#39210 closed defect (bug) (fixed)
switch_to_locale() unloads all plugin and theme translations
Reported by: | gchtr | Owned by: | ocean90 |
---|---|---|---|
Milestone: | 6.1 | Priority: | normal |
Severity: | normal | Version: | 4.7 |
Component: | I18N | Keywords: | has-unit-tests has-patch needs-dev-note commit |
Focuses: | Cc: |
Description
When using switch_to_locale() in the backend, all translations loaded into the $l10n
global will be unloaded, except for the default one. This makes plugin and theme translations unavailable after using switch_to_locale().
In the load_translations
method of WP_Locale_Switcher
there are two functions called right after each other for each of the currently loaded domains:
unload_textdomain( $domain );
get_translations_for_domain( $domain );
unload_textdomain
loads all unloaded functions into the $l10n_unloaded
global. Later when _load_textdomain_just_in_time()
is called in get_translations_for_domain()
, all domains set in $10n_unloaded
will be short-circuited in the following statement
// Short-circuit if domain is 'default' which is reserved for core.
if ( 'default' === $domain || isset( $l10n_unloaded[ $domain ] ) ) {
return false;
}
This results in only the new translations for the domain default
being loaded. All plugin and theme translations will be lost. But even when || isset( $l10n_unloaded[ $domain ]
is commented out, it doesn’t work, because WordPress then tries to load a language file from the WP_LANG
folder, and not from either a theme or plugin directory.
In #26511 it was already mentioned by @rmccue that this might affect emails to be sent in the wrong language: https://core.trac.wordpress.org/ticket/26511#trac-change-8-1430202811399151.
I run into this problem when I want to send notification emails to subscribed email adresses whenever I publish a post.
Test case
I created a minimal theme with a translations for en_EN
and de_DE
that displays some debug output in the backend and the frontend.
Preparatory steps:
- Set site language to German (de_DE).
- Set admin user language to English (en_EN). If the language dropdown does not appear, temporarily set the site language to German so the translation is downloaded and then select the users language again.
- Install the test theme.
Now an admin notice should appear in the backend that shows debug output. There are string translations saved before and after switch_to_locale
. Whenever Untranslated default string
shows up, then a translation couldn’t be loaded.
---
Now if this is not intended behavior, I don’t really know how I would approach fixing this.
Attachments (10)
Change History (98)
#1
follow-up:
↓ 17
@
8 years ago
- Keywords needs-patch added
Hello @gchtr, welcome to WordPress Trac!
Thanks for your detailed report!
The language switcher was introduced in [38961]. The first iterations had support for caching translations during a switch, see 26511.diff:ticket:26511. But this was removed in favour of _load_textdomain_just_in_time()
. The committed version had also a change to the POMO library to store the current path of a translation file. But the part in the switcher didn't actually work and got removed in 39330. So yeah, you're right that the switcher can't handle translations by themes/plugins which don't have their translations in WP_LANG_DIR
which is used by _load_textdomain_just_in_time()
.
Sadly we didn't document that in the dev note. It's not really intended behavior, but it's the current behavior. As a workaround you can use the new change_locale action. It gets called when a locale is switched or restored. You can use it to call your custom load_(theme|plugin)_textdomain()
function.
This ticket was mentioned in Slack in #core-restapi by atimmer. View the logs.
8 years ago
#3
@
8 years ago
Thanks for the explanation @ocean90! I see that for performance reasons, it makes total sense to do it like that.
I now use the following workaround, which works fine:
<?php add_action( 'change_locale', 'force_load_theme_textdomain' ); // Switch to site locale $site_locale = get_locale(); $locale_switched = switch_to_locale( $site_locale );
<?php /** * Hotfix for WordPress 4.7 * * WordPress removes translations when switch_to_locale is used in backend. * Until this error is resolved, we circumvent it by adding a filter * to force the site locale for load_theme_textdomain() and then load the missing * text domain. * * @see https://core.trac.wordpress.org/ticket/39210 */ function force_load_theme_textdomain() { add_filter( 'theme_locale', 'force_site_locale_once' ); load_theme_textdomain( 'axa-blog', get_template_directory() . '/languages' ); } /** * Forces site locale once. * * Since 4.7 WordPress always returns the user locale in the backend. * This filters tells WordPress to use the site locale once when * load_theme_textdomain() is used. * * @return string $locale The locale to use. */ function force_site_locale_once() { // Remove filter to use it only once remove_filter( 'theme_locale', 'force_site_locale_once' ); // Return site locale return get_locale(); }
I have to use another filter before using load_theme_textdomain
to force-load the site’s text domain and not the user’s, which is the default for the backend.
Now is it really a bug or a matter of documentation (the special case of using switch_to_locale() in the backend)? Am I on an edge case here? Or should this be made more convenient for developers?
#4
@
8 years ago
39210.diff has a solution to this (very painful) problem. This works by storing all paths from `load_plugin|theme|muplugin' and attempting to load from there again.
#6
follow-up:
↓ 7
@
8 years ago
- Keywords needs-unit-tests added
- Milestone changed from Awaiting Review to 4.8
Can't $l10n_paths
be a property of WP_Locale_Switcher
to avoid another global?
I'd really like to change all the load_*_textdomain()
functions to behave more like a registration without actually loading the translation. Loading would be handled by _load_textdomain_just_in_time()
.
#7
in reply to:
↑ 6
@
8 years ago
Replying to ocean90:
Can't
$l10n_paths
be a property ofWP_Locale_Switcher
to avoid another global?
It probably can - I'll see about that
I'd really like to change all the
load_*_textdomain()
functions to behave more like a registration without actually loading the translation. Loading would be handled by_load_textdomain_just_in_time()
.
Would you like to explore that in this ticket, or separately?
needs-unit-test
What were you thinking in terms of additional unit tests?
This ticket was mentioned in Slack in #core by flixos90. View the logs.
7 years ago
#10
@
7 years ago
- Keywords needs-refresh added
The current patch needs a refresh, but overall seems to work.
I like the registration idea mentioned earlier and I think we can explore that in this ticket.
#11
@
7 years ago
- Keywords dev-feedback added; needs-refresh removed
Just spent some time on adding a new registry class for storing these paths.
The load_*_textdomain()
functions now don't actually load any translations. This only happens when calling translate()
.
Thus, things like accessing $l10n['default']->headers['Language']
just after switching locales doesn't work unless you call load_*_textdomain()
/ get_translations_for_domain()
between that. That's why the test_switch_to_different_site_locale_if_user_locale_is_set()
test is failing right now. If BC in this area is a big concern, $l10n
could be turned into an object with magic getters. When someone accesses $l10n['default']->headers
, we would manually load the translations for that.
If there's interest, I can put this up on GitHub as a PR for collaboration.
This ticket was mentioned in Slack in #core-i18n by swissspidy. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-i18n by clorith. View the logs.
7 years ago
#14
@
7 years ago
So I spent a good few hours debugging this to find this bug! The bottom line is you can unload translations, but you cant add them back unless they are the core ones. the
_load_textdomain_just_in_time
function needs || isset( $l10n_unloaded[ $domain ] )
removed. I was doing this as part of a multisite plugin activation routine with rewrite rules since the language switcher would NOT switch languages properly.
Here is a gist of some of the code in my (public) plugin that I had to use to get my use case fully working: https://gist.github.com/pcfreak30/12a0023518863ad75c2c05c76e8f53da
I think there is a design flaw in this that needs to be looked at again.
Thanks!
#15
@
6 years ago
@pcfreak30 same couple of hours lost for me too!
Please devs, at least put a ToDo/Warning/whatever alongside the code for switch_to_locale()
Basic use case of what currently fails:
add_action( 'after_setup_theme', function() {
load_theme_textdomain( 'co', get_template_directory() . '/languages' );
});
// we need to change locale. Eg, according to page template
add_action( 'wp', function() {
if (is_page() && strtolower(get_field('country_code')) == 'es' && FOOBAR) {
// Only if languages are installed for core (wp-cli language core install es_ES)
switch_to_locale("es_ES");
}, 10);
print(__("hello","co")); // expected "hola"
#17
in reply to:
↑ 1
;
follow-up:
↓ 20
@
6 years ago
Replying to ocean90:
So yeah, you're right that the switcher can't handle translations by themes/plugins which don't have their translations in
WP_LANG_DIR
which is used by_load_textdomain_just_in_time()
.
Just to add a note that this isn't quite correct (anymore), themes/plugins which have their translations in WP_LANG_DIR
and are using _load_textdomain_just_in_time()
are definitely affected by this, as [37113]/#37855 cause *any* unloaded textdomain (whether specifically requested, or not) to not be re-loadable.
One work-around is to simply disable the never reload a textdomain
functionality using this one-liner:
add_filter( 'change_locale', function() { $GLOBALS['l10n_unloaded'] = array(); } );
Another would be a core change like 39210.3.diff which adds a flag to unload_textdomain()
to allow the language switcher to indicate that the textdomain can be reloaded if needed by _load_textdomain_just_in_time()
.
39210.3.diff doesn't attempt to fix the scenario where a theme/plugin is loading it's own translations, just the WordPress.org-provided translations. @ocean90 what do you think about at least adding that?
This ticket was mentioned in Slack in #meta by dd32. View the logs.
6 years ago
#19
@
5 years ago
As eluded to above, I've found the problem to be directly correlated with _load_textdomain_just_in_time()
and how it checks the $l10n_unloaded
which gets manipulated during unload_textdomain()
during switching languages.
I'm experiencing this issue but in a slightly different context. My issue is when using switch_to_locale(), restore_previous_locale() and then switch_to_locale() again. The first time round works for me, but the second time the text isn't translated.
My use case is sending emails for bookings in different languages of the person making the booking.
As a simple test, install Events Manager from WordPress.org and make sure Spanish is installed in the wp-content/languages folder. Then add this:
<?php add_action('init', function(){ global $locale; $domain = 'events-manager'; $test_string = 'Quantity'; $test_output = 'Current "'.$test_string.'" Translation in <code>%s</code> : %s<br>'; echo sprintf($test_output, $locale, __($test_string, $domain)); switch_to_locale('es_ES'); echo sprintf($test_output, 'es_ES', __($test_string, $domain)); restore_previous_locale(); echo sprintf($test_output, $locale, __($test_string, $domain)); // uncomment the following line to fix the problem //global $l10n_unloaded; //if( !empty($l10n_unloaded[$domain]) ) unset($l10n_unloaded[$domain]); switch_to_locale('es_ES'); echo sprintf($test_output, 'es_ES', __($test_string, $domain)); die(); });
The last line gets translated only if I uncomment those lines near the bottom.
#20
in reply to:
↑ 17
@
4 years ago
- Milestone changed from Future Release to 5.6
Well, after four years I finally trapped into the same issue…
Replying to dd32:
Another would be a core change like 39210.3.diff which adds a flag to
unload_textdomain()
to allow the language switcher to indicate that the textdomain can be reloaded if needed by_load_textdomain_just_in_time()
.
39210.3.diff doesn't attempt to fix the scenario where a theme/plugin is loading it's own translations, just the WordPress.org-provided translations. @ocean90 what do you think about at least adding that?
Thanks for the additional research and pointing to #37113. To me 39210.3.diff looks like a good first step to fix this. Created a PR based on the patch and the current tests are still passing too.
@swissspidy Any thoughts on the proposed patch?
#21
follow-up:
↓ 22
@
4 years ago
It's been a long time indeed...
The PR looks good at first glance, but some tests would be great.
Would this $reloadable
flag still be needed if we ever implement a simple cache/registry like proposed in the other patches? FWIW such a class is working fine in Preferred Languages
We shouldn't neglect non-dotorg plugins/themes when adding this, that's why I worked on this more complex approach in the day. Any chance we could resurrect that?
#22
in reply to:
↑ 21
@
4 years ago
Replying to swissspidy:
Any chance we could resurrect that?
If you can refresh the patch and create a PR, sure why not.
I guess a good test to see if this is working is to update the existing test_plugin_translation_after_switching_locale_twice() test.
This ticket was mentioned in PR #613 on WordPress/wordpress-develop by ocean90.
4 years ago
#24
- Keywords has-unit-tests added; needs-unit-tests removed
Related: https://github.com/WordPress/wordpress-develop/pull/590
Applies 39210.2.diff on current trunk.
Trac ticket: https://core.trac.wordpress.org/ticket/39210
4 years ago
#25
Current failing tests:
There were 6 failures: 1) Tests_Locale_Switcher::test_switch_reloads_translations_outside_wplang Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'Das ist ein Dummy Plugin' +'This is a dummy plugin' /var/www/tests/phpunit/tests/l10n/localeSwitcher.php:419 2) Tests_Privacy_WpPrivacySendErasureFulfillmentNotification::test_should_send_fulfillment_email_in_user_locale_when_site_is_not_en_us Failed asserting that '[Test Blog] Erasure Request Fulfilled' contains "Löschauftrag ausgeführt". /var/www/tests/phpunit/tests/privacy/wpPrivacySendErasureFulfillmentNotification.php:377 3) Tests_Privacy_WpPrivacySendErasureFulfillmentNotification::test_should_send_fulfillment_email_in_user_locale_when_both_have_different_locales_than_site Failed asserting that '[Test Blog] Erasure Request Fulfilled' contains "Löschauftrag ausgeführt". /var/www/tests/phpunit/tests/privacy/wpPrivacySendErasureFulfillmentNotification.php:421 4) Tests_Privacy_WpPrivacySendPersonalDataExportEmail::test_should_send_personal_data_export_email_in_user_locale_when_site_is_not_en_us Failed asserting that '[Test Blog] Personal Data Export' contains "Export personenbezogener Daten". /var/www/tests/phpunit/tests/privacy/wpPrivacySendPersonalDataExportEmail.php:366 5) Tests_Privacy_WpPrivacySendPersonalDataExportEmail::test_should_send_personal_data_export_email_in_user_locale_when_both_have_different_locales_than_site Failed asserting that '[Test Blog] Personal Data Export' contains "Export personenbezogener Daten". /var/www/tests/phpunit/tests/privacy/wpPrivacySendPersonalDataExportEmail.php:412 6) WP_Test_REST_Themes_Controller::test_theme_tags Failed asserting that two arrays are identical. --- Expected +++ Actual @@ @@ Array &0 ( - 0 => 'holiday' + 0 => 'Holiday' 1 => 'custom-menu' ) /var/www/tests/phpunit/tests/rest-api/rest-themes-controller.php:386
The first one is passing when running as a single test but not with others. npm run test:php -- '--group 39210'
passes, npm run test:php -- '--group i18n'
does not.
4 years ago
#26
We're down to two remaining test failures:
1) Tests_L10n_loadTextdomainJustInTime::test_get_translations_for_domain_does_not_return_null_if_override_load_textdomain_is_used Failed asserting that false is true. /var/www/tests/phpunit/tests/l10n/loadTextdomainJustInTime.php:120 2) WP_Test_REST_Themes_Controller::test_theme_tags Failed asserting that Array &0 ( 0 => 'Holiday' 1 => 'custom-menu' ) is identical to Array &0 ( 0 => 'holiday' 1 => 'custom-menu' ).
The first one looks legit after I have reverted the change from the patch in https://github.com/WordPress/wordpress-develop/pull/613/commits/272136eaa97a6391a274616beceb5bcf9dba63ed.
swissspidy commented on PR #613:
4 years ago
#27
Need to update Tests_Theme_ThemeDir::test_theme_list
after 9bf0c6f55a4cf217d12d33251142b0f329637565 it seems
#28
follow-up:
↓ 29
@
4 years ago
- Keywords needs-testing added; dev-feedback removed
The patch from 39210.2.diff has been refreshed and moved to GitHub. It's now ready for additional review/testing.
@jsmoriss Since you just recently noticed the same issue, would you mind giving the patch a try to see if it fixes the issue you were having?
@dd32 Instead of adding the new flag to unload_textdomain()
the latest patch doesn't call unload_textdomain()
anymore, instead it uses unset( $l10n[ $domain ] );. Curious what you think about this.
#29
in reply to:
↑ 28
;
follow-up:
↓ 32
@
4 years ago
Replying to ocean90:
dd32 Instead of adding the new flag to
unload_textdomain()
the latest patch doesn't callunload_textdomain()
anymore, instead it uses unset( $l10n[ $domain ] );. Curious what you think about this.
It seems fine to me, it's cleaner than the extra parameter IMHO, although it should probably be documented in the code as to why it unsets the global rather than calling the API (unload_textdomain) as it may not seem obvious at first to a casual reader.
#30
@
4 years ago
- Owner set to ocean90
- Resolution set to fixed
- Status changed from new to closed
In 49236:
#32
in reply to:
↑ 29
@
4 years ago
Replying to dd32:
Replying to ocean90:
dd32 Instead of adding the new flag to
unload_textdomain()
the latest patch doesn't callunload_textdomain()
anymore, instead it uses unset( $l10n[ $domain ] );. Curious what you think about this.
It seems fine to me, it's cleaner than the extra parameter IMHO, although it should probably be documented in the code as to why it unsets the global rather than calling the API (unload_textdomain) as it may not seem obvious at first to a casual reader.
I went back and forth on this and decided to go with the extra parameter for unload_textdomain()
, just to be on the safe side because unload_textdomain()
has a filter and action which would no longer be called.
#33
@
4 years ago
It may be good to mention the new define( 'WP_PLUGIN_DIR', realpath( DIR_TESTDATA . '/plugins' ) );
in the dev notes.
Over in Jetpack, we're re-configuring our testing setup (https://github.com/Automattic/jetpack/pull/17570 for Travis and need to do another for our Docker testing setup) since one of our tests relied on a function chain that eventually calls get_plugins
.
This ticket was mentioned in Slack in #core by daisyo. View the logs.
4 years ago
#36
@
4 years ago
- Keywords needs-refresh added; needs-dev-note removed
- Milestone changed from 5.6 to 5.7
- Resolution fixed deleted
- Status changed from closed to reopened
#40
@
4 years ago
- Keywords needs-patch added; has-patch needs-refresh removed
As the implementation was reverted in changeset [49566] to "investigate alternative implementations.", reclassifying from needs-refresh
to needs-patch
.
#41
@
4 years ago
- Milestone changed from 5.7 to 5.8
We're too late in the release cycle for 5.7 to get this one through. Moving to 5.8.
This ticket was mentioned in Slack in #core by chaion07. View the logs.
3 years ago
#43
@
3 years ago
Thank to @gchtr for reporting this. We reviewed this ticket during a recent [bug-scrub session]https://wordpress.slack.com/archives/C02RQBWTW/p1622580506178900. Pinging @ocean90 to see if you are planning on an updated patch/PR for this by Beta 1(June 8). Otherwise it is likely that this ticket is getting punted to Future Release. Thanks!
Props to @jeffpaul https://wordpress.slack.com/archives/C02RQBWTW/p1622580736180400
#44
@
3 years ago
- Milestone changed from 5.8 to 5.9
This one has not received any attention during the 5.8 cycle, so I'm going to punt to 5.9 for now. If someone is able to give this the time it needs before the beta 1 cut off point later today, it can be moved back.
#45
@
3 years ago
- Milestone changed from 5.9 to Future Release
This hasn't received the needed attention this cycle, so going to punt. Putting in Future Release
for now, but when someone is able to pick it back up, it can be moved back to a numbered milestone.
This ticket was mentioned in PR #2941 on WordPress/wordpress-develop by swissspidy.
2 years ago
#47
- Keywords has-patch added; needs-patch removed
I18N: Introduce WP_Textdomain_Registry
to store text domains and their language directory paths.
Previously, when using switch_to_locale()
all current loaded text domains were unloaded and added to the $l10n_unloaded
global. This prevented the just-in-time loading for text domains after a switch. The just-in-time loading was also only possible if the translations were stored in WP_LANG_DIR
. Both issues have been fixed.
- Adds
WP_Textdomain_Registry
to keep track of the language directory paths for all plugins and themes. - Updates all
load_*_textdomain()
functions to store the path inWP_Textdomain_Registry
. - Adds
$locale
parameter toload_textdomain()
to specify the locale the translation file is for. - Adds
$reloadable
parameter tounload_textdomain()
to define whether a text domain can be loaded just-in-time again. This is used byWP_Locale_Switcher::load_translations()
. - Extends
_load_textdomain_just_in_time()
to also support text domains of plugins and themes with custom language directories. - Fixes the incorrect
test_plugin_translation_after_switching_locale_twice()
test which should have caught this issue earlier. - Adds a new test plugin/theme to test the loading of translations with a custom language directory.
- Deprecates the now unused and private
_get_path_to_translation()
and_get_path_to_translation_from_lang_dir()
functions.
Changes since last time:
This is a reiteration of https://core.trac.wordpress.org/changeset/49236 with the following changes:
- Adds
$locale
parameter toload_textdomain()
to specify the locale the translation file is for. This avoids having to guess the locale by parsing the MO file name, as was done in https://github.com/WordPress/wordpress-develop/pull/694.
Trac ticket: https://core.trac.wordpress.org/ticket/39210
swissspidy commented on PR #2941:
2 years ago
#48
The test_get_locale_is_called_only_once_per_textdomain
test is now failing, all others seem to pass.
spacedmonkey commented on PR #2941:
2 years ago
#49
@swissspidy This looks really interesting. Out of interest, how this got a performance benefit. I wonder if it does if we could get some support from the performance team.
swissspidy commented on PR #2941:
2 years ago
#50
This is not about performance benefits; it aims to fix a bug (of course trying not to make performance worse). See https://core.trac.wordpress.org/ticket/39210 for context.
spacedmonkey commented on PR #2941:
2 years ago
#51
This is not about performance benefits;
I get that. But this sounds like it would be a performance win. Fixing just-in-time?
swissspidy commented on PR #2941:
2 years ago
#52
We already have a cache in place for translations and just-in-time translation loading, but unfortunately it doesn't work for themes/plugins which don't have their translations in WP_LANG_DIR
when switching locales. This PR simply addresses that edge case. There's no performance angle, unless I am missing something (it's the first time I properly look into this ticket after 2 years, so that's possible)
swissspidy commented on PR #2941:
2 years ago
#54
Committed in https://core.trac.wordpress.org/changeset/53874
#57
@
2 years ago
https://core.trac.wordpress.org/changeset/53874#file28 introduces a substantial change to the PHPUnit bootstrap.php
which will break tests for any plugin or theme which defines a custom path for the WP_PLUGIN_DIR
constant which was never defined by core for tests before. This has already broken tests running on the nightly version of WordPress for Google Site Kit which is what brought this to my attention.
A quick search on GitHub shows many code matches for this constant in files under tests
, including WordPress repos (e.g. wordpress/performance), and Yoast extensions for WooCommerce and News.
What can't be quantified are all the other cases where the constant is preferred to be left undefined, using the default definition of wp-content/plugins
, for example a project running tests in a specific configuration, e.g. https://github.com/wp-phpunit/example-project
If possible, I would ask that this constant is defined conditionally, only if core tests are being run, rather than unconditionally as it is now to avoid such breaking changes.
This ticket was mentioned in PR #3098 on WordPress/wordpress-develop by aaemnnosttv.
2 years ago
#58
This PR modifies a change that was introduced in https://core.trac.wordpress.org/changeset/53874#file28 which unconditionally added a constant definition for WP_PLUGIN_DIR
. This definition is unnecessary to be made unconditionally and breaks tests for suites by extensions that also define this constant. More information about this in my comment here: https://core.trac.wordpress.org/ticket/39210#comment:57
Trac ticket: https://core.trac.wordpress.org/ticket/39210
#59
@
2 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening to consider the above modification to avoid breaking changes.
#60
follow-up:
↓ 61
@
2 years ago
@swissspidy @ocean90 I agree with the concern by @aaemnnosttv: The WP_PLUGIN_DIR
constant is very commonly defined in plugin test scripts, so defining it here unconditionally is a breaking change.
If it had a notable benefit, it may be worth it, but as far as I can tell, this is only needed for a single test. That doesn't mean we need to completely remove it. How about we define this constant only if it is not defined, or alternatively, only when running the WP core tests?
For plugin or theme tests setting this constant here does not help, and removing it for those test runs wouldn't break anything either.
#61
in reply to:
↑ 60
@
2 years ago
Replying to flixos90:
If it had a notable benefit, it may be worth it, but as far as I can tell, this is only needed for a single test. That doesn't mean we need to completely remove it. How about we define this constant only if it is not defined, or alternatively, only when running the WP core tests?
I think the latter makes sense, that's exactly what was done the last time this came up in [49269] / #51594, before being reverted in [49566]. Looks like PR #3098 goes that route too.
This ticket was mentioned in Slack in #core by sergey. View the logs.
2 years ago
SergeyBiryukov commented on PR #3098:
2 years ago
#64
Thanks for the PR! Merged in r53902.
#66
follow-up:
↓ 67
@
2 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Unfortunately, it appears that this change has introduced a major performance regression. A few days ago I conducted a performance analysis comparing 6.1-RC1 (and yesterday RC2) to the latest 6.0 release, finding a regression of ~10% slower WordPress execution on average.
Myself and several other folks have since looked at possible causes for the regression, and earlier today @spacedmonkey messaged me, suggesting that the changes for loading the textdomain from here are a major part of the problem.
I looked into it further and they indeed are:
- When using the en_US locale,
load_textdomain()
is (unnecessarily) called a ton of times (107 times in WP 6.1-RC2 vs 3 times in WP 6.0.3). - The regression makes a en_US site with WP 6.1-RC2 load ~9.4% slower than with WP 6.0.3 (151.3ms vs 138.2ms). This makes it likely the only notable cause for the regression.
- Using a hacked version of WP 6.1-RC2 with the relevant code bits replaced with the 6.0.3 code, performance for an en_US site is back to the levels from 6.0.3.
- The regression is only present on en_US sites. For sites that use a locale that typically has translation files present, performance remains basically as before.
- The problem is caused by the changes to
_load_textdomain_just_in_time()
, which uses the newWP_Textdomain_Registry
class.
Due to this notable regression, I propose to revert [53874] for now and resolve the underlying problem for WordPress 6.2. Since the problem only occurs for en_US sites, there's a good chance this issue should not present a major complexity to address. Of course, if there is a simple fix, we could try to get it into RC3 - the main point is that we either need to fix this right away or revert before the 6.1 stable launch.
I will also upload a screenshot of the spreadsheet where I collected the data that led to this conclusion.
@
2 years ago
Data showing how the 6.1 implementation for this ticket causes a performance regression
#67
in reply to:
↑ 66
;
follow-up:
↓ 70
@
2 years ago
Replying to flixos90:
- When using the en_US locale,
load_textdomain()
is (unnecessarily) called a ton of times (107 times in WP 6.1-RC2 vs 3 times in WP 6.0.3).
Assuming you have no other plugins active, could you explain what calls these are? And why there are unnecessary?
- The regression is only present on en_US sites. For sites that use a locale that typically has translation files present, performance remains basically as before.
That's actually a good point since the changes here will significantly improve the translation handling for the majority of our user base. If there's some tradeoff this should also be taken into consideration.
I think for the moment we should identify what the problem actually is and if it's not something that can be fixed before the release and/or in a follow-up release.
#68
@
2 years ago
I have provided some screenshots of useful data. As you can see, removing this commit from code, as a massive performance boost and fixes the issues. My test site, using 2016 theme with a full home page, no translations, plugins or other config and single site.
The issue, is every time a translated string is loaded, it does a call to is_readable
. In this theme with lots of data, it is 900+ times on a single page load. If you had more posts per page, then more the page slows down.
Unless this is an extremely simple fix, this commit should be reverted and put into another release. That could be 6.1.1 but it might have to wait until 6.2. I don't think the performance trade off is worth the translations benefits here.
#69
@
2 years ago
I checked the regression for a while.
After checking, I discovered that the _load_textdomain_just_in_time
method, which repeatedly calls load_textdomain
, is the problem.
#70
in reply to:
↑ 67
@
2 years ago
Replying to ocean90:
Replying to flixos90:
- When using the en_US locale,
load_textdomain()
is (unnecessarily) called a ton of times (107 times in WP 6.1-RC2 vs 3 times in WP 6.0.3).Assuming you have no other plugins active, could you explain what calls these are? And why there are unnecessary?
I used a vanilla WordPress site with no plugins and Twenty Twenty-One, with just the initial Hello World post. Only the default and twentytwentyone text domains are used in this context, but the commit here results in load_textdomain() to be called 107 times in one page load, even though all the site is en_US (no locale switching happening at all). So this is clearly a bug that needs to be addressed.
- The regression is only present on en_US sites. For sites that use a locale that typically has translation files present, performance remains basically as before.
That's actually a good point since the changes here will significantly improve the translation handling for the majority of our user base. If there's some tradeoff this should also be taken into consideration.
It makes sense that this is a trade off. I would like to see some data though under which conditions the commit improves translation handling and how (much).
I think for the moment we should identify what the problem actually is and if it's not something that can be fixed before the release and/or in a follow-up release.
As mentioned, I'm open to fixing this prior to release if feasible. But the data I shared proves the regression coming from the commit. Without having opposing data that would quantify any performance benefit for non-en_US sites, this either needs to be fixed or reverted prior to release.
#71
follow-up:
↓ 72
@
2 years ago
We have now already established that there's a bug here causing too many calls to load_textdomain()
. Simple as that. No need for detailed performance analysis for other sites or anything.
We have also already established that ideally we fix this or revert this if we can't fix it in the required time frame.
Good news is that I have a draft PR fixing this PR. It's like 99% complete.
Feel free to to check it out. I will continue looking into it myself when my next workday commences.
#72
in reply to:
↑ 71
;
follow-up:
↓ 75
@
2 years ago
Replying to swissspidy:
We have now already established that there's a bug here causing too many calls to
load_textdomain()
. Simple as that. No need for detailed performance analysis for other sites or anything.
I still think it would be interesting to see how the original commit here affects performance for the scenarios where it already works as expected, so I did a quick test similar to the previous ones where I compared 6.1-RC2 with 6.1-RC2 with the relevant code replaced with the 6.0.3 code, just with translation files present.
The difference is small, but consistently visible: With de_DE for example, with the commit here WordPress took a median of 0.1915s to load, while without it it was 0.1956s, so roughly 4ms difference. While there is always fluctuation, this seems to indeed be consistent, so it's good to see a positive impact of the overall concept.
Good news is that I have a draft PR fixing this PR. It's like 99% complete.
Feel free to to check it out. I will continue looking into it myself when my next workday commences.
I took a look at the PR. I am not too familiar with that code, but it seems to me that there is a much simpler solution to the problem than what the PR does: The real problem is in https://core.trac.wordpress.org/browser/trunk/src/wp-includes/l10n.php?rev=54654#L762, which I found out due to further testing: Different than my original assessment where I focused on the en_US locale, the problem goes actually beyond that: It occurs whenever translation files are not present locally, which is certainly possible for even locales that are not en_US (e.g. when a plugin or theme is not translated yet, or simply when the site has not downloaded the language packs yet).
Simply adding a $wp_textdomain_registry->set( $domain, $locale, false );
above the above line fixes the problem for me, by storing that there is no translation file in that situation, the same way it is already done when the translation file cannot be imported.
Potentially this may introduce another problem that I'm unfamiliar with, but at least the above change is where I would have started.
#73
follow-up:
↓ 78
@
2 years ago
The original solution here was not implemented because of performance, but because it fixes a bug with locale switching.
Your oroposed solution does not work as it breaks locale switching. Believe me, I have implemented this and am a component maintainer.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
2 years ago
#75
in reply to:
↑ 72
@
2 years ago
Replying to flixos90:
Simply adding a
$wp_textdomain_registry->set( $domain, $locale, false );
above the above line fixes the problem for me, by storing that there is no translation file in that situation, the same way it is already done when the translation file cannot be imported.
Potentially this may introduce another problem that I'm unfamiliar with, but at least the above change is where I would have started.
I'm not super familiar with this code either, but it looks like this was previously tried in #51678. While that would restore the performance, it seems to break what this ticket aimed to achieve.
This ticket was mentioned in PR #3508 on WordPress/wordpress-develop by @spacedmonkey.
2 years ago
#76
Trac ticket: https://core.trac.wordpress.org/ticket/39210
#77
@
2 years ago
Forgive me, but I put together a very simple patch, that caches, if the file exists or not. See #3508. This seems to fix much of the problem, as I am seeing page speed improve.
If this is not the solution, then I would love to understand why not. Because I don't see a reason not to cache is_readable, it seems it is not a value that is going to change in a page request.
#78
in reply to:
↑ 73
@
2 years ago
Replying to swissspidy:
Your oroposed solution does not work as it breaks locale switching. Believe me, I have implemented this and am a component maintainer.
Makes sense. Like I said, I'm not very familiar with the code here. Given that your PR looks more complex than what I would anticipate for this late in the release cycle though, I think it would be crucial to get someone else with l10n experience to review it (looking at you @ocean90).
Realistically, this probably needs to be committed by Monday or reverted so that we can have it in the final RC (Tuesday).
Replying to spacedmonkey:
Forgive me, but I put together a very simple patch, that caches, if the file exists or not. See #3508. This seems to fix much of the problem, as I am seeing page speed improve.
If this is not the solution, then I would love to understand why not. Because I don't see a reason not to cache is_readable, it seems it is not a value that is going to change in a page request.
This doesn't address the underlying bug, since even with this caching, load_textdomain()
still is called >100 times within one page load - that is the root problem here. I trust @swissspidy is working into the right direction (although I'm too unfamiliar with the underlying code to make for a good reviewer on this).
This ticket was mentioned in PR #3506 on WordPress/wordpress-develop by @swissspidy.
2 years ago
#79
L10N: Change how WP_Textdomain_Registry
stores the default languages path.
WP_Textdomain_Registry
was introduced in [53874] to store text domains and their language directory paths, addressing issues with just-in-time loading of textdomains when using locale switching and when usingload_*_textdomain()
functions.
Said change has inadvertently caused a performance regression exactly when usingload_*_textdomain()
, which still often is the case, where the cached information was not further used or even overridden.
This change addresses that issue by storing the default languages paths in a separate way, while at the same time making WP_Textdomain_Registry
easier to maintain and adding new tests to catch future regressions.
Trac ticket: https://core.trac.wordpress.org/ticket/39210
#80
@
2 years ago
This doesn't address the underlying bug, since even with this caching,
load_textdomain()
still is called >100 times within one page load - that is the root problem here.
Precisely.
Please check out https://github.com/WordPress/wordpress-develop/pull/3506 for a proper fix.
I know this is time-critical, which is why I have spent yesterday and today looking into it, including consulting with @ocean90.
@spacedmonkey commented on PR #3506:
2 years ago
#81
@SergeyBiryukov commented on PR #3506:
2 years ago
#82
Thanks for working on this! Left a couple of comments, looks good to me otherwise 👍
#83
@
2 years ago
I spent some time testing the PR. It has a net posative result on performance. Tested on
Single / Multisite, 2016, with and without object caching. Also tested with and without translation files present.
Shaved around 200ms off the page load. Great work @swissspidy.
#84
@
2 years ago
- Keywords commit added
The linked pull request has approvals from two committers since the most recent /src
change: @ocean90 @SergeyBiryukov. @flixos90 approved earlier.
Marking for commit.
@swissspidy commented on PR #3506:
2 years ago
#86
Commited in https://core.trac.wordpress.org/changeset/54669
Test theme to show behavior