Make WordPress Core

Opened 4 years ago

Last modified 4 years ago

#49263 new enhancement

Switching blog doesn't switch locale

Reported by: iandunn's profile iandunn Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: I18N Keywords: 2nd-opinion
Focuses: multisite Cc:

Description

#26511 introduced switch_to_locale(), but didn't take into account the Multisite use case that @rmccue mentioned in ticket:26511#comment:8.

It seems reasonable to expect that switching to a site with a different locale would switch the locale.

You can see that it currently doesn't by setting up a network with the following conditions and code, and then loading each site.

  • The locale of site 1 is en_US
  • The locale of site 2 is es_MX
  • The locale of site 3 is fr_FR
add_action( 'admin_init', function() {
        echo get_locale() . '<br>'; // loaded site
        _e( 'Howdy, %s' );
        echo '<hr>';

        switch_to_blog( 2 ); // spanish
        echo get_locale() . '<br>';
        _e( 'Howdy, %s' );
        restore_current_blog();
        echo '<hr>';

        switch_to_blog( 3 ); // french
        echo get_locale() . '<br>';
        _e( 'Howdy, %s' );
        restore_current_blog();
        echo '<hr>';

        switch_to_blog( 1 ); // english
        echo get_locale() . '<br>';
        _e( 'Howdy, %s' );
        restore_current_blog();
        echo '<hr>';

        echo get_locale() . '<br>'; // back to loaded site
        _e( 'Howdy, %s' );

        wp_die();
} );

The strings are always translated using the loaded site's locale, rather than the switched site.

A rudimentary way to see the desired effect would be something like this:

function switch_to_blog_locale() {
        $locale = get_option( 'WPLANG', 'en_US' ); // bypass get_locale() b/c early return is stuck on the starting site.

        switch_to_locale( $locale );
}
add_action( 'switch_blog', 'switch_to_blog_locale' );

...although that doesn't take user locales into account, doesn't restore previous locales, etc.

Related: #44844

Attachments (4)

49263.1.diff (6.6 KB) - added by iandunn 4 years ago.
rough WIP exploring approaches
49263.2.diff (6.1 KB) - added by iandunn 4 years ago.
Modularized get_locale() into _get_raw_site_locale()
49263.3.diff (7.8 KB) - added by iandunn 4 years ago.
iterating on tests
49263.4.diff (9.1 KB) - added by iandunn 4 years ago.
Minor notes

Download all attachments as: .zip

Change History (12)

#1 @johnjamesjacoby
4 years ago

  • Keywords 2nd-opinion reporter-feedback added

Also related to #39210 (which I found to have more conversation & context.)

At a cursory, this probably would use _load_textdomain_just_in_time() but then would suffer from the same relative performance issues.

It makes me (once again) wish we had a base Switcher class, along with a way to register what happens when something is switched where. There are places where switching between sites (or users/posts/locales eventually) needs to be really speedy (like the top toolbar menu getting Site Names) but then other places where switching would expect for things like translations to be properly loaded up.

Today, I'm inclined to suggest that the best solution is also the worst one, which is to wait until a multisite specific bug appears (like an email getting delivered in the wrong locale or something) and intentionally hooking in the locale change as needed instead of doing it always and unhooking for performance benefits.

If/when locale switching can be cached nicely and super speedy (and it's viable for more broad implementation across core) then we could revisit this specific issue and give it another try?

Thoughts?

#2 @iandunn
4 years ago

  • Keywords reporter-feedback removed

🤔

My first thought would be to cache expensive operations like those in a transient, but I'm guessing you're thinking of some other examples where that wouldn't work well? Or is that a concern even with the site names in the toolbar?

Another idea would be to add a new parameter to switch_to_blog() which would let the caller explicitly define what should be switched. e.g.,

switch_to_blog( 4, null, array(
    'blog_id' => true,
    'table_prefix' => true,
    'object_cache' => true,
    'locale' => true,
) );

The default args could be set to the current behavior, so locale would be false.

#3 @johnjamesjacoby
4 years ago

🤔

Another idea would be to add a new parameter to switch_to_blog() which would let the caller explicitly define what should be switched. e.g.,

I initially did not like that idea, but it's actually pretty good. Could pass those into the related filters then too.

@iandunn
4 years ago

rough WIP exploring approaches

#4 @iandunn
4 years ago

49263.1.diff is a rough start. switch_to_blog() needs a way to get the new site's locale, but get_locale() caches the old one. I added a param to bypass that, so that we can reuse the logic there in a DRY way.

That updates $GLOBALS['locale'], though, which causes switch_to_locale() to return early. It looks like modularizing the inner logic out of get_locale() and into something like _get_raw_locale() might be the best way, but I'm still playing around with things.

Let me know if anyone has any feedback.

We'll also need to account for user locales, and work out how to restore the previous locale and other flags in restore_current_blog().

---

I'm also making progress on a workaround for sites that need to fix it before this lands, which shows some of what may be necessary here.

@iandunn
4 years ago

Modularized get_locale() into _get_raw_site_locale()

#5 @iandunn
4 years ago

49263.2.diff takes the approach of modularizing get_locale() so that the inner logic is reusable without any side-effects on globals/filters.

That's working at first glance, but still needs lots of testing and refinement, as well as clarity around what's needed to properly restore a locale. Also opinions on whether or not the new _get_raw_site_locale() should be labeled internal or not.

@iandunn
4 years ago

iterating on tests

This ticket was mentioned in Slack in #core-multisite by iandunn. View the logs.


4 years ago

#7 @iandunn
4 years ago

switch_to_blog()'s 2nd parameter has been deprecated since r21485, but I'm wondering if we could reuse it, instead of adding a new 3rd param? That would make the devex a bit smoother.

Any thoughts?


The old $validate param was a bool, so we could maintain back-compat by calling _deprecated_argument() if the value is anything other than an array. After calling it, the value could be overridden with array(), so that $default_options would be used, and the current behavior (not switching the locale) would be maintained).

I scanned the directory for plugins that might be affected by that:

> ag "switch_to_blog\(\s?(.*)," --php ./plugins/ | tee scans/switch-to-blog.txt

Most are false-positives from plugins that include a library that defines its own `switch_to_blog()` function, but that function doesn't ever call Core's switch_to_blog() with a 2nd argument.

So so if we ignore those:

> cat scans/switch-to-blog.txt |grep -v "freemius" > scans/switch-to-blog-not-freemius.txt

...then we get:

> ./summarize-scan.php scans/switch-to-blog-not-freemius.txt
42 matching plugins
Matches  Plugin                                     Active installs
=======  ======                                     ===============
      1  ibexrentacar                                           10+
      1  radio-buttons-for-taxonomies                       10,000+
      1  wp-survey                                              10+
      2  linkedin-lite                                         300+
      1  static-html-output-plugin                          10,000+
      1  administrative-shortcodes                              70+
      2  threewp-broadcast                                   1,000+
      1  network-posts-extended                                600+
      1  blackbaud-sphere                                        0+
      1  redactor                                               10+
      1  wp-appkit                                           1,000+
      1  blogrollsync                                           10+
      1  export-readers                                          0+
      2  avatar-manager                                     10,000+
      1  multilingual-press                                    300+
      2  social-networks-auto-poster-facebook-twitter-g    100,000+
      1  pollme                                                 10+
      2  mu-widgets                                             10+
      1  rss-injection                                          10+
      1  wp-headfoot                                            40+
      3  lepress-20                                             10+
      1  wp-postdate                                            10+
      1  checkbox-for-taxonomies                               100+
      1  buddypress                                        200,000+
      1  wp-boilerplate                                         10+
      1  members-blog                                           10+
      1  quizme                                                 10+
      1  wp-submission                                          10+
      4  multisite-language-switcher                         7,000+
      3  woocommerce-billink                                   200+
      7  wp-optimize                                       900,000+
      1  jetpack                                         5,000,000+
      2  lcimedia-broadcast                                 REMOVED
      1  paymill                                               400+
      1  presstest                                          REMOVED
      2  broadcast-mu                                       REMOVED
      1  pressroom-by-newswire                                  60+
      1  single-posts-ext                                        0+
      1  slimjetpack                                         8,000+
      1  wp-contactme                                          100+
      1  blognetworking                                         10+
      1  blog-linkit                                            10+

_deprecated_argument() isn't currently being called for the 2nd param, so adding it would introduce new notices for those plugins. We could mitigate that by emailing them, and mentioning it in a dev-note on make/Core.

@iandunn
4 years ago

Minor notes

#8 @iandunn
4 years ago

Related #50228

Note: See TracTickets for help on using tickets.