-
Notifications
You must be signed in to change notification settings - Fork 103
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?
Changes from 2 commits
171e2be
6f5aee7
046b8c6
8c9fc82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,4 +39,24 @@ public static function load_active_modules(): void { | |
} | ||
} | ||
} | ||
|
||
/** | ||
* Loads early hooks from modules previously activated in the options. | ||
* | ||
* Attention! This assumes the current states stored in the options are valid. | ||
* This doesn't perform any check, neither on the plugin conditions nor the folder structure. | ||
* In the worst case the state can be refreshed by reactivating the plugin. | ||
* | ||
* Note also the modules should be loaded before "qtranslate_init_language" is triggered. | ||
*/ | ||
public static function load_active_modules_early_hooks(): void { | ||
$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 commentThe reason will be displayed to describe this comment to others. Learn more. Except for |
||
if ( $state === QTX_MODULE_STATE_ACTIVE && file_exists( $target ) ) { | ||
require_once( $target ); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 commentThe 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). |
||
|
||
/** | ||
* Allows url redirection due to language auto detection only if site url has been requested | ||
* | ||
* @see qtranxf_check_url_maybe_redirect | ||
* @param string $url_lang proposed target URL for the active language to redirect to. | ||
* @param string $url_orig original URL supplied to browser, which needs to be standardized. | ||
* @param array $url_info a hash of various information parsed from original URL, cookies and other site configuration. The key names should be self-explanatory. | ||
* | ||
* @return string resulting redirection url | ||
*/ | ||
function qtranxf_slugs_language_detect_redirect($url_lang, $url_orig, $url_info): string { | ||
global $q_config; | ||
if (site_url().'/' === $url_orig) | ||
return $url_lang; | ||
else { | ||
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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. More changes with 8c9fc82 |
||
} | ||
} | ||
herrvigg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
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.