Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QTS: revision and cleanup of add/edit terms slugs input code #1126

Merged
merged 6 commits into from
Mar 21, 2022

Conversation

spleen1981
Copy link
Contributor

@spleen1981 spleen1981 commented Mar 13, 2022

No description provided.

@herrvigg
Copy link
Collaborator

I have some issues when saving a term, but I don't think it's coming just from this patch.

Warning: Undefined array key "es" in .../includes/class-qtranslate-slug.php on line 1346.

public function validate_term_slug( $slug, $term, $lang ) {
        global $q_config; //TODO: q_config  : term_name
        $term_key = qtranxf_split( $term->name );
        // after split we will get array (with language code as a key )
        $term_key = $term_key[ $this->default_language ];
        $name_in_lang = $q_config['term_name'][ $term_key ][ $lang ];

Here is category I'm testing
image

The $term_key becomes Actualités which is already quite weird, taking the name for the default language as a key.
The values are just set for FR, EN, DE in database, they are empty for IT and ES but here the form fields take the default language slug.

It's possible it doesn't handle well existing terms which are missing the slugs for every language.

@herrvigg
Copy link
Collaborator

Also, another consideration: why don't we use the Language Switch Buttons (LSB) instead of duplicating all the fields for every language? With the LSB enabled we should use those and keep a single "slug" field to stick the general UI offered by QTX.

@herrvigg
Copy link
Collaborator

Another minor bug, when we switch language with the LSB, the slug field (read-only) is not updated in the table.

image

It should change to news for english and so forth.

@herrvigg
Copy link
Collaborator

I don't expect all of this to be solved in this Pull Request, but I leave it for info.

Let me know what you think about these 3 last remarks and see how we proceed. We can first merge this patch if you like.

@herrvigg
Copy link
Collaborator

herrvigg commented Mar 20, 2022

Point 1 (bug) - can be worth looked at in this PR.
Point 2 (LSB edit update) - require certainly much more changes with JS code. Can be treated as a feature request.
Point 3 (read-only update) - could be related to point 2, as the read-only updates are also done in JS.

@herrvigg
Copy link
Collaborator

Once this patch is merged, we should split this huge class-translate-slug.php file. It's way too big (1870 lines). The terms part could be taken out, among others.

@spleen1981
Copy link
Contributor Author

Point 1 should be covered by 1d2653c, let me know if it works. I've removed unneeded complications which really did not seem to add value (besides the wrong implementation itself, with the missing check if key existed).
Point 2 & 3 need some reflection, as I think that slug metadata structure would need to be changed to use a single string same as translated content (e.g. [:en]slug1[:it]slug2[:]) rather than current mutiple strings, which is a big change in the module structure. Changing only javascript will only fix the LSB, but would not work in raw editor for example. However this makes sense and can be done, also for consistency with main plugin, but I would consider it as an enhancement and keep the matter on hold until the module is clean and stable. To formally fix the column bug for now, we may name the column "Slug (default language)".

@spleen1981 spleen1981 changed the title QTS: fixed duplicate slugs fields in admin edit terms and unneeded co… QTS: revision and cleanup of add/edit terms slugs input code Mar 21, 2022
@herrvigg
Copy link
Collaborator

Point 1 should be covered by 1d2653c, let me know if it works. I've removed unneeded complications which really did not seem to add value (besides the wrong implementation itself, with the missing check if key existed).

Yes it works, awesome!

@herrvigg
Copy link
Collaborator

Points 2 and 3: it will require more work, indeed. We can create separate tickets to keep track of it.

@herrvigg herrvigg merged commit c3e0374 into qtranslate:master Mar 21, 2022
@spleen1981 spleen1981 deleted the class_review_005 branch March 21, 2022 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants