Make WordPress Core

Opened 6 years ago

Closed 7 months ago

Last modified 7 months ago

#39210 closed defect (bug) (fixed)

switch_to_locale() unloads all plugin and theme translations

Reported by: gchtr's profile gchtr Owned by: ocean90's profile 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:

  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.

Attachments (10)

test_theme.zip (4.6 KB) - added by gchtr 6 years ago.
Test theme to show behavior
39210.diff (12.5 KB) - added by yoavf 6 years ago.
39210.2.diff (33.2 KB) - added by swissspidy 6 years ago.
39210.3.diff (3.3 KB) - added by dd32 4 years ago.
39210-wp61-regression.png (137.1 KB) - added by flixos90 8 months ago.
Data showing how the 6.1 implementation for this ticket causes a performance regression
Screenshot 2022-10-20 at 08.56.52.png (176.2 KB) - added by spacedmonkey 8 months ago.
Trace of the path
Screenshot 2022-10-19 at 23.51.11.png (72.4 KB) - added by spacedmonkey 8 months ago.
Screenshot 2022-10-19 at 23.48.49.png (25.5 KB) - added by spacedmonkey 8 months ago.
Screenshot 2022-10-19 at 23.48.42.png (25.8 KB) - added by spacedmonkey 8 months ago.
Screenshot 2022-10-21 at 23.47.16.png (282.1 KB) - added by spacedmonkey 7 months ago.
Results of testing patch in different environments.

Download all attachments as: .zip

Change History (98)

@gchtr
6 years ago

Test theme to show behavior

#1 follow-up: @ocean90
6 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.


6 years ago

#3 @gchtr
6 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( '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?

Last edited 6 years ago by gchtr (previous) (diff)

#4 @yoavf
6 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.

@yoavf
6 years ago

#5 @yoavf
6 years ago

  • Keywords has-patch added; needs-patch removed

#6 follow-up: @ocean90
6 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 @yoavf
6 years ago

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?

This ticket was mentioned in Slack in #core by flixos90. View the logs.


6 years ago

#9 @ocean90
6 years ago

  • Milestone changed from 4.8 to Future Release

#10 @swissspidy
6 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.

@swissspidy
6 years ago

#11 @swissspidy
6 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.


6 years ago

This ticket was mentioned in Slack in #core-i18n by clorith. View the logs.


5 years ago

#14 @pcfreak30
5 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 @drzraf
5 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"

#16 @ocean90
4 years ago

#46230 was marked as a duplicate.

@dd32
4 years ago

#17 in reply to: ↑ 1 ; follow-up: @dd32
4 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?

Last edited 4 years ago by dd32 (previous) (diff)

This ticket was mentioned in Slack in #meta by dd32. View the logs.


4 years ago

#19 @msykes
4 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.

Last edited 4 years ago by msykes (previous) (diff)

#20 in reply to: ↑ 17 @ocean90
3 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: @swissspidy
3 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 @ocean90
3 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.

#23 @ocean90
3 years ago

#51539 was marked as a duplicate.

This ticket was mentioned in PR #613 on WordPress/wordpress-develop by ocean90.


3 years ago
#24

  • Keywords has-unit-tests added; needs-unit-tests removed

ocean90 commented on PR #613:


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

ocean90 commented on PR #613:


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

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

swissspidy commented on PR #613:


3 years ago
#27

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

#28 follow-up: @ocean90
3 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: @dd32
3 years ago

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.

#30 @ocean90
3 years ago

  • Owner set to ocean90
  • Resolution set to fixed
  • Status changed from new to closed

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.

#31 @ocean90
3 years ago

  • Keywords needs-dev-note added; needs-testing removed

#32 in reply to: ↑ 29 @ocean90
3 years ago

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.

#33 @kraftbj
3 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.


3 years ago

#35 @ocean90
3 years ago

In 49566:

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

See #39210, #51678, #26511.

#36 @ocean90
3 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

#37 @ocean90
3 years ago

#51678 was marked as a duplicate.

#38 @SergeyBiryukov
3 years ago

In 49569:

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

See #39210.

#39 @SergeyBiryukov
3 years ago

In 49570:

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

See #39210.

#40 @hellofromTonya
2 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 @lukecarbis
2 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.


2 years ago

#43 @chaion07
2 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 @desrosj
2 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 @desrosj
19 months 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.

#46 @SergeyBiryukov
13 months ago

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.

This ticket was mentioned in PR #2941 on WordPress/wordpress-develop by swissspidy.


11 months 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 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:

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

swissspidy commented on PR #2941:


11 months 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:


11 months 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:


11 months 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:


11 months 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:


11 months 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)

#53 @swissspidy
10 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#55 @swissspidy
10 months ago

  • Keywords add-to-field-guide added
  • Milestone changed from Future Release to 6.1

#56 @swissspidy
10 months ago

  • Keywords needs-dev-note added; add-to-field-guide removed

#57 @aaemnnosttv
10 months 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.

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 ticket was mentioned in PR #3098 on WordPress/wordpress-develop by aaemnnosttv.


10 months 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 @aaemnnosttv
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to consider the above modification to avoid breaking changes.

#60 follow-up: @flixos90
10 months 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 @SergeyBiryukov
10 months 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.


10 months ago

#63 @SergeyBiryukov
10 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

SergeyBiryukov commented on PR #3098:


10 months ago
#64

Thanks for the PR! Merged in r53902.

#65 @desrosj
9 months ago

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.

#66 follow-up: @flixos90
8 months 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 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
8 months ago

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

#67 in reply to: ↑ 66 ; follow-up: @ocean90
8 months 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.

@spacedmonkey
8 months ago

Trace of the path

#68 @spacedmonkey
8 months 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 @mukesh27
7 months 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 @flixos90
7 months 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: @swissspidy
7 months 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: @flixos90
7 months 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: @swissspidy
7 months 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.


7 months ago

#75 in reply to: ↑ 72 @SergeyBiryukov
7 months 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.

#77 @spacedmonkey
7 months 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 @flixos90
7 months 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.


7 months 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 @swissspidy
7 months 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:


7 months ago
#81

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

https://i0.wp.com/user-images.githubusercontent.com/237508/197207279-00c091f5-8406-45c2-9545-0efa321961cd.png

@SergeyBiryukov commented on PR #3506:


7 months ago
#82

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

#83 @spacedmonkey
7 months 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.

@spacedmonkey
7 months ago

Results of testing patch in different environments.

#84 @peterwilsoncc
7 months 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.

#85 @swissspidy
7 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#87 @ocean90
7 months ago

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.

This ticket was mentioned in Slack in #core by sergey. View the logs.


7 months ago

Note: See TracTickets for help on using tickets.