WordPress.org

Make WordPress Core

Opened 18 months ago

Last modified 6 weeks ago

#39210 new defect (bug)

switch_to_locale() unloads all plugin and theme translations

Reported by: gchtr Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: I18N Keywords: has-patch needs-unit-tests dev-feedback
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 (3)

test_theme.zip (4.6 KB) - added by gchtr 18 months ago.
Test theme to show behavior
39210.diff (12.5 KB) - added by yoavf 17 months ago.
39210.2.diff (33.2 KB) - added by swissspidy 8 months ago.

Download all attachments as: .zip

Change History (17)

@gchtr
18 months ago

Test theme to show behavior

#1 @ocean90
18 months 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.


18 months ago

#3 @gchtr
18 months 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 12 months ago by gchtr (previous) (diff)

#4 @yoavf
17 months 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
17 months ago

#5 @yoavf
17 months ago

  • Keywords has-patch added; needs-patch removed

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


12 months ago

#9 @ocean90
12 months ago

  • Milestone changed from 4.8 to Future Release

#10 @swissspidy
9 months 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
8 months ago

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


8 months ago

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


4 months ago

#14 @pcfreak30
6 weeks 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!

Note: See TracTickets for help on using tickets.