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

[SLUGS] fix language autodetection #1364

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

spleen1981
Copy link
Contributor

Uses the qtranslate_language_detect_redirect filter to change default QTX redirection behaviour, redirecting user url only if site_url() is requested and not redirecting otherwise. In the latter case, if $url_info['lang_url'] is empty default language is assumed.
To use qtranslate_language_detect_redirect filter, placed in QTX source before modules are loaded, additional modules "early hooks" mechanism has been added to modules handling.

Fixes #1328
Fixes #1348
May help with #1273

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.

I need to think how to design the init flow better to support both this and mu-plugins. I may do it in several steps.

@@ -82,6 +82,8 @@ function qtranxf_init_language(): void {
$url_info['language'] = qtranxf_detect_language( $url_info );
$q_config['language'] = apply_filters( 'qtranslate_language', $url_info['language'], $url_info );

QTX_Module_Loader::load_active_modules_early_hooks();

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is changing the loading sequence logic, as we currently load the code of the modules later.
I don't think we should make it more complex by having "early hooks" and "later hooks". The flow is already complicated and I've been trying to simplify it with consistent loaders.
Also there's some rework to do for #1325 with mu-plugins.
But I understand the need for some plugins that have effect on redirection step that comes quite early on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For now no requested change, but I keep it on hold a bit. I still may merge the patch as it is and refactor the loading sequence completely from this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

At this point I'm not convinced we should generalize this pattern to all modules.
The Slugs module is quite special because it touches some core functionalities.
For now I suggest to have a special case for slugs in that section. I'll try to push another patch.

$modules_state = get_option( QTX_OPTIONS_MODULES_STATE, array() );

foreach ( $modules_state as $module_id => $state ) {
$target = QTRANSLATE_DIR . '/src/modules/' . $module_id . '/' . 'early_hooks.php';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Except for early_hooks.php this is not different than load_active_modules for the main loader.php.
We need a better design with a unique entry point.

@@ -0,0 +1,27 @@
<?php

add_filter( 'qtranslate_language_detect_redirect', 'qtranxf_slugs_language_detect_redirect', 600, 3 );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to avoid using a filter for an internal mechanism, that should be called explicitly to have full control on the flow. I don't think any other plugin should have a possibility to alter this behavior between core and the slugs module (once it's enabled).

if ( empty( $url_info['lang_url'] ) ){
return qtranxf_convertURL( $url_orig, $q_config['default_language'],false,true );
} else {
return $url_orig;
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a regression with this patch. If I set manually a URL with just the default language to force a language switch this behavior is now broken.
Example if it is the default language, setting /it/ should redirect to home page after setting the cookie. With this patch I get page not found (404).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@herrvigg it should be fixed with 046b8c6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More changes with 8c9fc82

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants