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.

Sign up for GitHub

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

Jump to bottom

Raw merge of qtranslate-slug 1.1.18 as qtranslate-xt module #1060

Merged
merged 6 commits into from
Feb 13, 2022
Merged

Raw merge of qtranslate-slug 1.1.18 as qtranslate-xt module #1060

merged 6 commits into from
Feb 13, 2022

Conversation

spleen1981
Copy link
Contributor

Initial merge with minimal modifications to both qtranslate-xt core and qtranslate-slug to incorporate the latter as a module.
Module can be enabled through a checkbox in qtranslate-xt advanced settings.
Heavy code cleanup and harmonization yet to be performed.
Settings page also to be incorporated in qtranslate-xt page

@spleen1981 spleen1981 mentioned this pull request Aug 31, 2021
Closed
@spleen1981
Raw merge of qtranslate-slug 1.1.8 as qtranslate-xt module
b1bf3aa 
Initial merge with minimal modifications to both qtranslate-xt core and qtranslate-slug to incorporate the latter as a module.
Module can be enabled through a checkbox in qtranslate-xt advanced settings.
Heavy code cleanup and harmonization yet to be performed.
Settings page also to be incorporated in qtranslate-xt page
@herrvigg herrvigg added core Core functionalities, including the admin section enhancement New feature or request plugin: others Concerns integration with other plugins labels Aug 31, 2021
@herrvigg
Copy link
Collaborator

Need more time to check and polish, but it's in the plan to integrate this. It will be a major release.

herrvigg
herrvigg reviewed Feb 13, 2022
View reviewed changes
Copy link
Collaborator

@herrvigg herrvigg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job!! Looks good overall!
A few remarks with a few changes to come before merge.
We should transition fully from plugin to module. It is still in an intermediate state. But the idea is to merge and continue refactoring this on master.

modules/qtx_modules_handler.php
array(
'id' => 'slugs',
'name' => 'Slugs',
'plugin' => true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should rather enable that if the user checks "enable experimental slugs", so the other way around.
But I see why it is easier like this for now. The module has still the legacy code of the plugin, so the init sequence should be refactored.

qtranslate_core.php Show resolved Hide resolved
modules/slugs/includes/class-qtranslate-slug.php Outdated Show resolved Hide resolved
modules/slugs/includes/class-qtranslate-slug.php Outdated Show resolved Hide resolved
modules/slugs/includes/class-qtranslate-slug.php Show resolved Hide resolved
modules/qtx_modules_handler.php Outdated Show resolved Hide resolved
admin/qtx_admin_settings.php Outdated Show resolved Hide resolved
@herrvigg herrvigg merged commit 9d1a8d8 into qtranslate:master Feb 13, 2022
@herrvigg herrvigg added module: slugs and removed plugin: others Concerns integration with other plugins labels Feb 19, 2022
@herrvigg
Copy link
Collaborator

For the last updates and post-merge cleanup see #671.

@spleen1981 spleen1981 deleted the slugs-module branch February 19, 2022 15:55
@bagaweb
Copy link

bagaweb commented Apr 28, 2022

Hello maybe this is not the correct place to post this error, let me know if I should open a new thread...

Anyway... I noticed an issue with Russian language, if the slugs are in Cyrillic (like the titles), front-end links in Russian language give 404 error.

If I rewrite the slugs in Latin characters (for example apartamenty instead of апартаменты), there are no issues.

Anyone else noticed this bug?

@spleen1981
Copy link
Contributor Author

@bagaweb opening a new issue would be better for proper follow-up and visibility.

@spleen1981 spleen1981 changed the title Raw merge of qtranslate-slug 1.1.8 as qtranslate-xt module Raw merge of qtranslate-slug 1.1.18 as qtranslate-xt module Apr 28, 2022
@spleen1981
Copy link
Contributor Author

Based on original plugin README, this seems to be a known issue.

@bagaweb
Copy link

bagaweb commented Apr 28, 2022 •
edited
Loading

Based on original plugin README, this seems to be a known issue.

Ok but this issue was about taxonomies and custom post types, not about pages.

On an another project which is using QTXT & QTS 1.18, pages work fine even with Cyrillic slugs.

@bagaweb
Copy link

bagaweb commented Apr 28, 2022 •
edited
Loading

@bagaweb opening a new issue would be better for proper follow-up and visibility.

BTW I opened a new issue here: #1156

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@herrvigg herrvigg herrvigg left review comments

Assignees
No one assigned
Labels
core Core functionalities, including the admin section enhancement New feature or request module: slugs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@spleen1981 @herrvigg @bagaweb