switch_to_locale() unloads all plugin and theme translations

[gchtr]

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:

1. Set site language to German (de_DE).
2. 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.
3. 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.

[gchtr]

Test theme to show behavior

[ocean90]

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.

[gchtr]

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( 'my-text-domain', 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?

[yoavf]

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.

[ocean90]

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().

[yoavf]

Replying to ocean90:

Can't $l10n_paths be a property of WP_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?

[swissspidy]

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.

[swissspidy]

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.

[pcfreak30]

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!

[drzraf]

@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"

[ocean90]

#46230 was marked as a duplicate.

[dd32]

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?

[msykes]

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.

[ocean90]

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?

[swissspidy]

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?

[ocean90]

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.

[ocean90]

#51539 was marked as a duplicate.

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

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.

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.

The test:
​https://github.com/WordPress/wordpress-develop/blob/30965710474260b25031951df31eec901e716e28/tests/phpunit/tests/l10n/loadTextdomainJustInTime.php#L109-L117

Need to update Tests_Theme_ThemeDir::test_theme_list after 9bf0c6f55a4cf217d12d33251142b0f329637565 it seems

[ocean90]

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.

[dd32]

Replying to ocean90:

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.

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.

[ocean90]

In 49236:

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 in WP_Textdomain_Registry.
• Adds $reloadable parameter to unload_textdomain() to define whether a text domain can be loaded just-in-time again. This is used by WP_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 catch 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.

Props yoavf, swissspidy, dd32, ocean90.
See #26511.
Fixes #39210.

[ocean90]

Replying to dd32:

Replying to ocean90:

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.

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.

[kraftbj]

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.

[ocean90]

In 49566:

I18N: Revert [49236] for now to investigate alternative implementations.

See #39210, #51678, #26511.

[ocean90]

#51678 was marked as a duplicate.

[SergeyBiryukov]

In 49569:

Build/Tests Tools: Restore [49535], accidentally reverted in [49566].

See #39210.

[SergeyBiryukov]

In 49570:

Build/Tests Tools: Restore [49491], accidentally reverted in [49566].

See #39210.

[hellofromTonya]

As the implementation was reverted in changeset [49566] to "investigate alternative implementations.", reclassifying from needs-refresh to needs-patch.

[lukecarbis]

We're too late in the release cycle for 5.7 to get this one through. Moving to 5.8.

[chaion07]

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

[desrosj]

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.

[desrosj]

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.

[SergeyBiryukov]

I was curious what was the issue with the original implementation in [49236] that caused it to be reverted in [49566]. Based on a quick discussion with @swissspidy, addressing the performance regression reported in #51678 would help move this ticket forward.

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 in WP_Textdomain_Registry.
• Adds $locale parameter to load_textdomain() to specify the locale the translation file is for.
• Adds $reloadable parameter to unload_textdomain() to define whether a text domain can be loaded just-in-time again. This is used by WP_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 to load_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

The test_get_locale_is_called_only_once_per_textdomain test is now failing, all others seem to pass.

@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.

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.

This is not about performance benefits;

I get that. But this sounds like it would be a performance win. Fixing just-in-time?

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]

In 53874:

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 in WP_Textdomain_Registry.
• Adds $locale parameter to load_textdomain() to specify the locale the translation file is for.
• Adds $reloadable parameter to unload_textdomain() to define whether a text domain can be loaded just-in-time again. This is used by WP_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 and 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.

Previously added in [49236] and reverted in [49236] to investigate concerns which are now addressed here.

Props yoavf, swissspidy, dd32, ocean90.
See #26511.
Fixes #39210.

Committed in https://core.trac.wordpress.org/changeset/53874

[aaemnnosttv]

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.

​https://github.com/search?q=%22define%28+%27WP_PLUGIN_DIR%27%22+path%3Atests+language%3APHP+language%3APHP&type=Code&ref=advsearch&l=PHP&l=PHP

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 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

[aaemnnosttv]

Reopening to consider the above modification to avoid breaking changes.

[flixos90]

@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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

In 53902:

Build/Test Tools: Only define WP_PLUGIN_DIR when running core tests.

This takes into account plugin or theme test suites that rely on WP_PLUGIN_DIR being set to a custom path.

Follow-up to [49236], [49269], [49566], [53874].

Props aaemnnosttv, flixos90.
Fixes #39210.

Thanks for the PR! Merged in r53902.

[desrosj]

In 54210:

Coding Standards: Various alignment fixes from composer format.

Follow up to [53874], [54097], [54110], [54155], [54162], [54184].

See #39210, #55443, #56288, #56092, #56408, #56467, #55881.

[flixos90]

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 new WP_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.

[flixos90]

Data showing how the 6.1 implementation for this ticket causes a performance regression

[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?

• 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.

[spacedmonkey]

Trace of the path

[spacedmonkey]

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.

[mukesh27]

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.

[flixos90]

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.

[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.

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.

[flixos90]

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.

[swissspidy]

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.

[SergeyBiryukov]

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.

Trac ticket: https://core.trac.wordpress.org/ticket/39210

[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.

[flixos90]

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).

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

[swissspidy]

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.

I haven't reviewed the code yet. But a quick check of the performance shows some amazing results.

Thanks for working on this! Left a couple of comments, looks good to me otherwise 👍

[spacedmonkey]

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.

[spacedmonkey]

Results of testing patch in different environments.

[peterwilsoncc]

The ​linked pull request has approvals from two committers since the most recent /src change: @ocean90 @SergeyBiryukov. @flixos90 approved earlier.

Marking for commit.

[swissspidy]

In 54669:

I18N: 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.

Props flixos90, spacedmonkey, ocean90, SergeyBiryukov, costdev.
Fixes #39210.

Commited in https://core.trac.wordpress.org/changeset/54669

[ocean90]

In 54682:

I18N: 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.

Props flixos90, spacedmonkey, ocean90, SergeyBiryukov, costdev.
Merges [54669] to the 6.1 branch.
See #39210.