-
Notifications
You must be signed in to change notification settings - Fork 107
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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(); | |||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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 ); |
There was a problem hiding this comment.
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).
b659941
to
6f5aee7
Compare
src/modules/slugs/early_hooks.php
Outdated
if ( empty( $url_info['lang_url'] ) ){ | ||
return qtranxf_convertURL( $url_orig, $q_config['default_language'],false,true ); | ||
} else { | ||
return $url_orig; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More changes with 8c9fc82
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