Make WordPress Core

Opened 10 years ago

Closed 7 years ago

Last modified 14 months ago

#26511 closed feature request (fixed)

Introduce a locale-switching function

Reported by: johnbillion's profile johnbillion Owned by: swissspidy's profile swissspidy
Milestone: 4.7 Priority: high
Severity: normal Version:
Component: I18N Keywords: has-patch has-unit-tests commit has-dev-note
Focuses: Cc:

Description

When a site's content is displayed in a language that's different to that used in the admin area [1] by using the locale filter in get_locale(), the admin toolbar is shown in the language of the content, not the language of the admin area.

For example, if your content is in French (using fr_FR in the locale filter) but your admin area is in English (using en_US in the locale filter) then the admin toolbar on the front end will display in French rather than English.

The value of get_locale() is used when the translation files are loaded at the beginning of the page load, but there's no easy way to subsequently switch locale and load a different set of translation files later in the page load (ie. when we output the admin toolbar in the footer).


We should introduce a means of changing the locale at any point during the page load and automatically loading in the relevant translation files, overriding existing ones. This could then be used to switch the language before admin-related output on the front end (primarily the admin toolbar, but potentially any admin-related item).

Leaving this as a feature request for now because I don't have a solution in mind.


[1] Several plugins support this, such as WPML, Babble and Admin in English.

Attachments (31)

switch_to_locale.php (3.7 KB) - added by yoavf 10 years ago.
switch_to_locale__revisited.php (5.0 KB) - added by tfrommen 8 years ago.
A refreshed version of the WordPress.com functions - PHP 5.2 compatible, and object-oriented.
26511.patch (38.2 KB) - added by tfrommen 8 years ago.
switch_locale.patch (42.9 KB) - added by pbearne 8 years ago.
updated patch with Unit tests (start off)
26511.2.patch (8.4 KB) - added by ocean90 8 years ago.
26511.3.patch (8.8 KB) - added by tfrommen 8 years ago.
Refreshed patch for 4.7
26511.diff (10.9 KB) - added by swissspidy 8 years ago.
26511.2.diff (12.3 KB) - added by swissspidy 8 years ago.
26511.3.diff (12.2 KB) - added by swissspidy 8 years ago.
de_DE.zip (2.2 KB) - added by swissspidy 8 years ago.
5 de_DE translations to check number_format in unit tests
26511.4.diff (14.5 KB) - added by swissspidy 8 years ago.
26511.5.diff (14.9 KB) - added by swissspidy 7 years ago.
26511.6.diff (16.5 KB) - added by swissspidy 7 years ago.
WP_Post_Type_Labels.diff (14.4 KB) - added by swissspidy 7 years ago.
26511.7.diff (17.5 KB) - added by swissspidy 7 years ago.
26511.8.diff (27.3 KB) - added by swissspidy 7 years ago.
26511.9.diff (30.3 KB) - added by swissspidy 7 years ago.
26511.10.diff (30.3 KB) - added by swissspidy 7 years ago.
26511.11.diff (29.3 KB) - added by ocean90 7 years ago.
26511.12.diff (30.8 KB) - added by swissspidy 7 years ago.
26511.13.diff (30.8 KB) - added by swissspidy 7 years ago.
26511.14.diff (30.8 KB) - added by swissspidy 7 years ago.
26511.15.diff (22.6 KB) - added by ocean90 7 years ago.
26511.15.1.diff (30.2 KB) - added by ocean90 7 years ago.
26511.16.diff (31.5 KB) - added by ocean90 7 years ago.
26511.16.1.diff (22.6 KB) - added by ocean90 7 years ago.
languages.zip (13.5 KB) - added by ocean90 7 years ago.
26511.17.diff (35.6 KB) - added by swissspidy 7 years ago.
26511.18.diff (35.8 KB) - added by ocean90 7 years ago.
26511.18.1.diff (35.8 KB) - added by ocean90 7 years ago.
26511.19.diff (35.1 KB) - added by ocean90 7 years ago.

Download all attachments as: .zip

Change History (140)

#1 @nacin
10 years ago

A locale-switching function would be nice to have.

I think this will be something core will need to look into once we start having an in-dashboard language chooser, whereby the pack for that language can be downloaded automatically on selection. At that point, the next logical step would be to allow a user to choose their dashboard language independently from the site's locale.

In theory, this is easy to do. (See the Admin in English plugin.) In practice, it's akin to using the admin in SSL and the frontend using HTTP — core is surely doing something somewhere that causes whatever happens in the admin to trickle into the frontend (so, the equivalent of #15928), and we'll need to figure out those and patch things up.

Anyway, a locale-switching function could be implemented along the lines of switch_to_blog(). As of right now it'd need to be just two things: a filter on 'locale' (there's also a $locale global, though YMMV in the future) followed by a call to load_default_textdomain(). Un-switching is a matter of unhooking the filter and re-calling load_default_textdomain(). (YMMV due to this). That's expensive, though. For core to have this functionality, we should start storing loaded translations on a per-language basis, so as not to obliterate the object in memory, to allow for a switch-back when done without re-calling load_default_textdomain().

#3 @emzo
10 years ago

  • Cc wordpress@… added

#4 @dimadin
10 years ago

For core strings you can switch language for admin bar, I made a gist with example coincidentally exactly a year ago.

If you want admin bar in en_US and plugins add admin bar items at admin bar hooks, you could have admin bar in en_US by using gettext filters conditionally like in example above.

Problem for not being able to load translations later for plugins and themes is because their translation is loaded once and there is no information about that textdomain (location, type). If we want a language switcher, we need to store that information since I don't see other way to load plugins and themes translations later.

You could actually do this now by hooking to load_*_textdomain() functions but that is complicated.

Note that on wpcom you can set interface language on profile and that will be used on admin bar even if you browse sites in other languages, don't know how do they do that.

#5 @yoavf
10 years ago

On WP.com, we already have something like this that works in a similar way to switch_to_blog() and lets you restore the previous language(s) when you want.

I'm going to look into adapting it for core - it won't work as is because of the way we rely on a single textdomain.

#6 @yoavf
10 years ago

switch_to_locale.php is mostly how we do it on wp.com - I just added incomplete support for additional textdomains. It's incomplete because I can't see a way to know where to get the mo files for plugins and themes when switching the locale.

Thinking aloud: maybe we introduce something like register_textdomain() that stores the location of mo files somewhere for later use. Haven't really thought this through.

This ticket was mentioned in Slack in #polyglots by sergeybiryukov. View the logs.


9 years ago

#8 @rmccue
9 years ago

Related: if you have a multilingual network (i.e. sites have different locales), then things that loop over the sites can break.

<?php
get_blog_option( 1, 'WPLANG' ) == '';
get_blog_option( 2, 'WPLANG' ) == 'de_DE';

// The site you start on will have the correct translations
get_current_blog_id() == 1;
get_locale() == 'en_US';
__( 'Hello' ) == 'Hello';

// Switch to a different site, and things start not making sense
switch_to_blog( 2 );
get_locale() == 'de_DE';
__( 'Hello' ) == 'Hello'; // Should be translated "Hallo"

This is causing a bug for us, as we loop over all sites to send a newsletter on each site. When we switch to the site, we generate an email for the site then send it. Turns out we're actually sending out a bunch of emails in the wrong language right now. :)

#9 @ocean90
8 years ago

Related: #32879

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


8 years ago

#11 @SergeyBiryukov
8 years ago

#32879 was marked as a duplicate.

#12 @SergeyBiryukov
8 years ago

  • Milestone changed from Awaiting Review to 4.6

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


8 years ago

#14 follow-up: @ocean90
8 years ago

  • Keywords needs-patch added
  • Summary changed from Separate locale for the admin toolbar to Introduce a locale-switching function

@yoavf, @dd32: Is switch_to_locale.php still the same function which is used on wp.com?

#15 in reply to: ↑ 14 @yoavf
8 years ago

Replying to ocean90:

@yoavf, @dd32: Is switch_to_locale.php still the same function which is used on wp.com?

Yes - this version was just edited to remove wp.com specific references and to add some ( incomplete ) support for multiple text domains.

#16 @pbearne
8 years ago

I took a different approach in #36859 and added a parameter to the constructor to wp_local maybe the efforts can be merged.

The current patch here doesn't fetch a missing language from WP.org where I have got that working in my patch.

@tfrommen
8 years ago

A refreshed version of the WordPress.com functions - PHP 5.2 compatible, and object-oriented.

#17 @tfrommen
8 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

I just attached a refreshed version of the WordPress.com functions.

I took a (PHP 5.2 compatible) object-oriented approach.

The only thing left is getting the according MO file for a textdomain. This should be easy to fix, though, if we were to attach it (the file path) to the MO object.

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


8 years ago

@tfrommen
8 years ago

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


8 years ago

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


8 years ago

#21 @pbearne
8 years ago

We have just been working on some unit test and found few problems / bugs with new switcher class

see new patch

@pbearne
8 years ago

updated patch with Unit tests (start off)

#22 @tloureiro
8 years ago

  • In our unit test, when running switch_to_locale(), the global $l10n was not loaded yet. So we had to run a dummy load_text_domain() before making use of the switcher class.
  • switch_to_locale now returns the locale if it runs successfully or false otherwise

#23 @pbearne
8 years ago

  • Keywords has-unit-tests added

The public var $month_genitive was/is not declared in new WP_Locale this is also missing in the old WP_Locale

<?php
        /**
         * Stores the translated strings for the full month names.
         * array( 'siječnja', 'veljače', 'ožujka', 'travnja', 'svibnja', 'lipnja', 'srpnja', 'kolovoza', 'rujna', 'listopada', 'studenoga', 'prosinca' );
         *
         * @since 4.6.0
         * @var array
         */
        public $month_genitive;

#24 @pbearne
8 years ago

The current function allows you to switch a random locale e.g. xx_XX

Is this a good idea?

If not I can add a test to catch that. e.g.

<?php
if ( ! in_array( $locale, get_available_languages() ) ) {
        return false;
}

But this test doesn't test that we have we have the language file (*.mo) installed so we could add call to the wp_get_available_translations() function ( a file system call - not cheap), But all this tells us is install or not. If it is not installed we shouldn't switch otherwise will have a broken set global locales.

We could get into loading the file from WP.org but this doesn't work on all sites and is slow.

I have come to the conclusion that we should just add the languages files for ALL languages returned via get_available_languages() to WP so we know we can switch and know that the data we need to load the locale is installed.

It's a lot of files but they are small.

Thoughts

Paul

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


8 years ago

#26 @ocean90
8 years ago

  • Owner set to ocean90
  • Status changed from new to reviewing

#27 @ocean90
8 years ago

In 37889:

I18N: Move the WP_Locale class to its own file.

The new class-wp-locale.php file is loaded in locale.php for backward compatibility purposes.

See #26511.
Fixes #37209.

@ocean90
8 years ago

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


8 years ago

#29 @ocean90
8 years ago

  • Milestone changed from 4.6 to Future Release

26511.2.patch is a refresh of 26511.patch after [37889].

Punting, since it didn't get much traction.

@tfrommen
8 years ago

Refreshed patch for 4.7

#30 @tfrommen
8 years ago

I just added a refreshed patch for 4.7 (including some minor tweaks).

So, how do we get this thing rolling now? :)

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


8 years ago

#32 @SergeyBiryukov
8 years ago

  • Milestone changed from Future Release to 4.7

Any way to avoid introducing a new global? See #37699.

#33 @pbearne
8 years ago

I still have a problem that you can change the locale to one that we don't have the translation files installed for.

I believe that we need to include a set of .mo file with the locale settings for all supported languages or return and error if an attempt to switch to locale that the lang .mo are not installed

Thoughts?

I do what to see this in WP but we need to make is safe.

Paul

#34 @swissspidy
8 years ago

@pbearne Yes, switching to non-existing locales should be avoided. See also suggestions in #29783.

I'll try to wrap my head around the patch & tests now.

Edit: What if there's a locale available for a plugin, but not for core? That shouldn't prevent switching.

Last edited 8 years ago by swissspidy (previous) (diff)

@swissspidy
8 years ago

#35 @swissspidy
8 years ago

26511.diff applies cleanly against trunk again.

Changes to the previous patch:

  • Actually includes some unit tests to get us started. Not included are Multisite-related tests (e.g. switch_to_blog() + switch_to_locale()) and tests with plugin/theme translations.
  • Removes the need for a WP_Locale_Storage class. One simple filter is enough.
  • Adds switch_locale and restore_locale action hooks.

As you can see, this takes into account some of the feedback from #29783. This ticket should actually lay the foundation for that one so we can kill two birds with one stone

@SergeyBiryukov: Regarding the $wp_locale_switcher global, we could change it when #37699 lands or perhaps make it a property of the WP class.

There's actually a bunch of i18n globals like $l10n, $l10n_unloaded, $locale, $wp_locale, $wp_local_package. We could combine all of these in 1 central WP_i18n class (which could also be a property of the WP class), though that's probably out of scope for this ticket.

#36 @tfrommen
8 years ago

Hey, cool this is moving forward!

Thanks, @swissspidy - and brilliant how you got rid of the extra class. Also, great to see you were able to take most of my patch as is for your version.

Some small improvements:

In WP_Locale_Switcher::switch_to_locale(), instead of this:

<?php
if ( null === $l10n ) { 
    $l10n = array(); 
} 

if ( empty( $l10n ) ) { 
    load_default_textdomain(); 
}

we could to that (and save an unnecessary extra check):

<?php
if ( ! $l10n ) { 
    $l10n = array(); 

    load_default_textdomain(); 
}

Also, in WP_Locale_Switcher::restore_locale(), move the initialization of $previous_locale to after the check, so we have this instead:

<?php
if ( ! array_pop( $this->locales ) ) { 
    // The stack is empty, bail. 
    return false; 
}

$previous_locale = end( $this->locales ); 

If you would like, I can provide a new patch for this.

#37 follow-up: @swissspidy
8 years ago

@tfrommen Agree, we can get rid of that extra check, although I usually like more strict comparisons.

Regarding $previous_locale, this won't work because that would make $previous_locale equal to $locale. I'm writing a test right now to demonstrate that $previous_locale is really the previous locale.

@swissspidy
8 years ago

#38 @swissspidy
8 years ago

26511.2.diff includes the previous suggestions including a test_restore_locale_action_passes_previous_locale test.

There's also an incomplete test_switch_to_locale_changes_wp_locale_global test to assert the $wp_locale object contains the correct variables. It's incomplete because the translation files in the test directory (introduced in #35284) do not contain the necessary translations. The easiest way to solve this would be to add translation files for an RTL language.

#39 in reply to: ↑ 37 @tfrommen
8 years ago

Replying to swissspidy:

Agree, we can get rid of that extra check, although I usually like more strict comparisons.

In case I'm checking for presence, I'm all strict, too. In this case, however, we check for absence, and furthermore can spare an unnecessary check. That's why I suggested to combine the two checks - which you already implemented. :)

Regarding $previous_locale, this won't work because that would make $previous_locale equal to $locale. I'm writing a test right now to demonstrate that $previous_locale is really the previous locale.

Well, forget my comment on this. It doesn't make any sense. I got distracted while reading your patch and then again while writing the comment here. :) I thought I saw some initialization where it wasn't (yet) needed, but obviously this was not the correct place.

#40 @swissspidy
8 years ago

No worries.

Just realized I forgot to properly add remove_action() in test_restore_locale_action_passes_previous_locale(). Will do that in the next iteration of the patch, but first it'd be great to get some feedback from someone like @ocean90 :-)

#41 follow-ups: @ocean90
8 years ago

  • Keywords dev-feedback removed

Nice job!

Feedback for 26511.2.diff:

  • The property $available_languages has no @since tag.
  • The pomo library is an external library, it shouldn't get @since tags for a WP version.
  • get_available_languages() returns only core languages, something to keep in mind.
  • has_translations_for_locale() is an odd method. It's only used to check $this->translations[ $locale ]. I think we can remove it or should think about a getter/setter method for $this->translations.
  • The actions restore_locale and switch_locale are running _after_ the locale has been switched. Should they be named locale_restored and locale_switched?
  • The function/method name restore_locale() is ambiguous. Which locale is restored? For multisite we have restore_current_blog(). The DocBlock for restore_locale() mentions "previous locale". Maybe name it restore_previous_locale()?
  • When calling load_default_textdomain() we can pass the locale, this would avoid one get_locale() call.
  • While looking at #26511: It's likely that we'll have a new function get_user_locale() which should probably have it's own filter. Do we need context - site vs. user - support for the switcher? What would be required to make this possible?
  • I'm wondering if we really have to cache the translations now that we have _load_textdomain_just_in_time(). Probably only because not all plugins/themes are using language packs, yet…
  • Do we have to care about unloaded text domains? See unload_textdomain() and the global $l10n_unloaded.
  • This really should be tested in combination with #29783 (needs-refresh). How does this work performance-wise for a regular site with 1 theme and maybe 10 plugins? (This should include one of the big ones like Jetpack or WooCommerce.)

#42 @ocean90
8 years ago

  • Owner ocean90 deleted

#43 follow-up: @johnjamesjacoby
8 years ago

Is it too late to recommend a WP_Switcher base class that sites can use, too?

Related: #19572 <3

#44 in reply to: ↑ 43 ; follow-up: @tfrommen
8 years ago

Replying to johnjamesjacoby:

Is it too late to recommend a WP_Switcher base class that sites can use, too?

Related: #19572 <3

As the implementation would be completely different in any case, there's no use for a base class. So ... please let's make it an interface - and shock the world. :D

#45 in reply to: ↑ 44 ; follow-up: @johnjamesjacoby
8 years ago

Replying to tfrommen:

Replying to johnjamesjacoby:

Is it too late to recommend a WP_Switcher base class that sites can use, too?

Related: #19572 <3

As the implementation would be completely different in any case, there's no use for a base class.

Consider that the implementation between admin area list-tables is different for each type of object being listed, but them sharing a base class is (I think) pretty helpful for defining the mechanics of how a list-table works regardless of it's type.

Similarly, the mechanics of stashing the current *thing*, switching to a new *thing*, and restoring the original *thing* are pretty common (consider $wp_query or wp_reset_postdata() for existing examples).

@swissspidy
8 years ago

#46 in reply to: ↑ 41 ; follow-up: @swissspidy
8 years ago

Replying to ocean90:

  • The property $available_languages has no @since tag.
  • The pomo library is an external library, it shouldn't get @since tags for a WP version.
  • get_available_languages() returns only core languages, something to keep in mind.
  • has_translations_for_locale() is an odd method. It's only used to check $this->translations[ $locale ]. I think we can remove it or should think about a getter/setter method for $this->translations.
  • The actions restore_locale and switch_locale are running _after_ the locale has been switched. Should they be named locale_restored and locale_switched?
  • The function/method name restore_locale() is ambiguous. Which locale is restored? For multisite we have restore_current_blog(). The DocBlock for restore_locale() mentions "previous locale". Maybe name it restore_previous_locale()?
  • When calling load_default_textdomain() we can pass the locale, this would avoid one get_locale() call.

Fixed in the latest patch. Renamed the method to restore_previous_locale(). The hooks run before setting the global, like in switch_to_blog() for example.

  • While looking at #29783: It's likely that we'll have a new function get_user_locale() which should probably have it's own filter. Do we need context - site vs. user - support for the switcher? What would be required to make this possible?

Do you have a use case for temporarily switching the user locale? If yes, a context switch could make sense and is fairly easy. Just needs a second stack.

Otherwise I was more thinking that developers could use it like this:

<?php
$switched = switch_to_locale( get_user_locale() );
// Do stuff
if ( $switched ) {
  restore_previous_locale();
}
  • I'm wondering if we really have to cache the translations now that we have _load_textdomain_just_in_time(). Probably only because not all plugins/themes are using language packs, yet…
  • Do we have to care about unloaded text domains? See unload_textdomain() and the global $l10n_unloaded.

It'd be awesome if we don't have to cache translations. This would allow us to basically remove all complexity from this class. Otherwise we'd really have to check the $l10n_unloaded global.

Citing @ocean90 from #29783:

We should avoid loading multiple languages into memory, only one at the same time.

  • This really should be tested in combination with #29783. How does this work performance-wise for a regular site with 1 theme and maybe 10 plugins? (This should include one of the big ones like Jetpack or WooCommerce.)

This shouldn't really affect #29783 right now. Regarding testing, I'll first add some unit tests and afterwards we can do some benchmarks.

As these two tickets are so tied to each other, should we perhaps merge the patches?

Replying to johnjamesjacoby:

Is it too late to recommend a WP_Switcher base class that sites can use, too?

Related: #19572 <3

You're right, there are lots of similar cases in core and some kind of abstraction could certainly help. If we don't mess up badly now, we can also do this later I think.

#47 @swissspidy
8 years ago

Oh, and regarding

  • get_available_languages() returns only core languages, something to keep in mind.

One shouldn't be able to switch to a non-existent locale. Are there many situations where a core language isn't available but a plugin has a translation for it? Shouldn't happen with language packs I think.

#48 in reply to: ↑ 41 @SergeyBiryukov
8 years ago

Replying to ocean90:

The function/method name restore_locale() is ambiguous. Which locale is restored? For multisite we have restore_current_blog(). The DocBlock for restore_locale() mentions "previous locale". Maybe name it restore_previous_locale()?

If the functions are modeled after switch_to_blog()/restore_current_blog(), shouldn't it be called restore_current_locale() to avoid confusion?

The discrepancy could throw someone off. (Is it restore_previous_locale() or restore_current_locale()? restore_current_blog() or restore_previous_blog()?)

#49 in reply to: ↑ 45 @GaryJ
8 years ago

Replying to johnjamesjacoby:

Similarly, the mechanics of stashing the current *thing*, switching to a new *thing*, and restoring the original *thing* are pretty common (consider $wp_query or wp_reset_postdata() for existing examples).

Right, so that's behaviour, which can be defined in an interface so that the common actions of stash(), switch() and restore() (to use your parlance as an example) are consistent, regardless of what the *thing* is.

@swissspidy
8 years ago

5 de_DE translations to check number_format in unit tests

#50 in reply to: ↑ 46 @ocean90
8 years ago

Replying to swissspidy:

Do you have a use case for temporarily switching the user locale? If yes, a context switch could make sense and is fairly easy. Just needs a second stack.

Otherwise I was more thinking that developers could use it like this:

<?php
$switched = switch_to_locale( get_user_locale() );
// Do stuff
if ( $switched ) {
  restore_previous_locale();
}

Yes: An author should get a comment moderation email in their language. But switch_to_locale( get_user_locale() ); should work, I think.

As these two tickets are so tied to each other, should we perhaps merge the patches?

No, but we should maybe move the other one to 4.7 too. :)

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


8 years ago

@swissspidy
8 years ago

#52 follow-up: @swissspidy
8 years ago

26511.4.diff is a new patch without the caching (at least for now) and a few new tests related to _load_textdomain_just_in_time(), which helped fix some bugs. Also contains some docs improvements etc. For the tests to work, one needs to add the files from de_DE.zip to the tests/phpunit/data/languages directory.

@SergeyBiryukov: restore_current_blog() is confusing because it means "go back to the blog I was on before". Or for locales, "switch to a locale, and then go back".

@GaryJ Looking at #25293, an interface would make sense, even though it's probably a bit too much for now for only like 2-3 public methods.

The patch & naming on #25293 is interesting by the way. Think WP_Locale_State. Also, it has a is_switched() method, which maybe could be helpful in our case as well.

#53 in reply to: ↑ 52 @GaryJ
8 years ago

Replying to swissspidy:

@GaryJ Looking at #25293, an interface would make sense, even though it's probably a bit too much for now for only like 2-3 public methods.

Even if there was just one public method, having an interface would contractually agree that that one method was present. Defensive coding should outweigh an extra file potentially being loaded IMHO.

#54 @swissspidy
7 years ago

Opened a new ticket to discuss naming convention: #38098.

Last edited 7 years ago by swissspidy (previous) (diff)

#55 follow-up: @ocean90
7 years ago

26511.4.diff looks good so far. Note that is uses get_user_locale() from #29783 which shouldn't be part of the initial commit. (Depends on which ticket lands first.)

  • Is there a reason why $GLOBALS['wp_locale'] = new WP_Locale(); happens after the actions?
  • The docs for WP_Locale_Switcher can be improved:
    • 'Class for switching locales.' => 'Core class used for switching locales.'
    • 'Locale switcher object.' => 'Locale API: WP_Locale_Switcher class'
  • Is the get_translations_for_domain() call in WP_Locale_Switcher::load_translations() required? Tests are still passing when removing the line.

#56 in reply to: ↑ 55 ; follow-up: @swissspidy
7 years ago

Replying to ocean90:

26511.4.diff looks good so far. Note that is uses get_user_locale() from #29783 which shouldn't be part of the initial commit. (Depends on which ticket lands first.)

Good catch. Should

  • Is there a reason why $GLOBALS['wp_locale'] = new WP_Locale(); happens after the actions?

I think I did this after your previous comment:

The actions restore_locale and switch_locale are running _after_ the locale has been switched. Should they be named locale_restored and locale_switched?

Instead of removing the actions I moved them up, similar to switch_to_blog().

  • The docs for WP_Locale_Switcher can be improved:
    • 'Class for switching locales.' => 'Core class used for switching locales.'
    • 'Locale switcher object.' => 'Locale API: WP_Locale_Switcher class'

Agreed, though the whole class name might still change. See #38098.

  • Is the get_translations_for_domain() call in WP_Locale_Switcher::load_translations() required? Tests are still passing when removing the line.

I think it can be removed as it's a leftover from the caching in the previous patches. Again, needs some benchmarking to decide if caching should be considered.

#57 in reply to: ↑ 56 @ocean90
7 years ago

Replying to swissspidy:

  • Is there a reason why $GLOBALS['wp_locale'] = new WP_Locale(); happens after the actions?

I think I did this after your previous comment:

The actions restore_locale and switch_locale are running _after_ the locale has been switched. Should they be named locale_restored and locale_switched?

Instead of removing the actions I moved them up, similar to switch_to_blog().

I see, but isn't loading the new text domain also part of switching the locale?
Also, "Fires when the locale is switched" isn't accurate right now. If I'd hook into switch_locale and use a function like is_rtl() I'd expect that it returns the data of the new locale. Currently it doesn't.

  • The docs for WP_Locale_Switcher can be improved:
    • 'Class for switching locales.' => 'Core class used for switching locales.'
    • 'Locale switcher object.' => 'Locale API: WP_Locale_Switcher class'

Agreed, though the whole class name might still change. See #38098.

You're saying that this ticket now depends on introducing an interface? I'm not sure I like that. _If_ there would be an interface in the future we still can use it even if the name is different. IMO WP_Locale_Switcher is self-explaining, WP_Locale_State not.

  • Is the get_translations_for_domain() call in WP_Locale_Switcher::load_translations() required? Tests are still passing when removing the line.

I think it can be removed as it's a leftover from the caching in the previous patches. Again, needs some benchmarking to decide if caching should be considered.

Okay, I'll try to do some benchmarking.

@swissspidy
7 years ago

#58 follow-up: @swissspidy
7 years ago

I see, but isn't loading the new text domain also part of switching the locale?
Also, "Fires when the locale is switched" isn't accurate right now. If I'd hook into switch_locale and use a function like is_rtl() I'd expect that it returns the data of the new locale. Currently it doesn't.

True. I moved the actions further down now.

You're saying that this ticket now depends on introducing an interface? I'm not sure I like that. _If_ there would be an interface in the future we still can use it even if the name is different.

No. I mainly created that ticket to keep discussions off from this one, but it's good to know where discussion is heading to make this class more future proof. The only thing I would rename at the moment is restore_previous_locale() to restore_current_locale() because of restore_current_blog().

In 26511.5.diff I removed the get_user_locale() check (which had no test btw!) and improved the docs as well.

I feel like the add_filter() call should be moved outside the constructor. Any ideas?

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


7 years ago

#60 in reply to: ↑ 58 @ocean90
7 years ago

Replying to swissspidy:

No. I mainly created that ticket to keep discussions off from this one, but it's good to know where discussion is heading to make this class more future proof. The only thing I would rename at the moment is restore_previous_locale() to restore_current_locale() because of restore_current_blog().

Not sure if we really have to be consistent with a naming which is semantically incorrect. But maybe we can provide both functions? restore_current_locale() would empty the stack and set the locale to $original_locale.

In 26511.5.diff I removed the get_user_locale() check (which had no test btw!) and improved the docs as well.

I feel like the add_filter() call should be moved outside the constructor. Any ideas?

Can't we move it to default-filters.php as add_filter( 'locale', array( $GLOBALS['wp_locale_switcher'], 'filter_locale' );? Or a simple init() method which gets called in wp-settings.php.

#61 follow-ups: @ocean90
7 years ago

Something I missed to mention earlier: There is an issue with functions like get_post_type_labels()/_get_custom_object_labels(). Post type labels are initially registered via register_post_type() which means they don't change when calling switch_to_locale(). You can see that with the example plugin in the Add New menu for the Post and Page items.

#62 in reply to: ↑ 61 @tfrommen
7 years ago

Replying to ocean90:

Something I missed to mention earlier: There is an issue with functions like get_post_type_labels()/_get_custom_object_labels(). Post type labels are initially registered via register_post_type() which means they don't change when calling switch_to_locale(). You can see that with the example plugin in the Add New menu for the Post and Page items.

There are even more issues, for example, when you have a Custom Post Type or Custom Taxonomy registered by using a translated slug. When you switch to a different locale, the rewrite rules (and thus the generated permalinks) do not get adapted.

The question is if coping with these things should be handled by the locale switcher, or not.

@swissspidy
7 years ago

#63 in reply to: ↑ 61 @swissspidy
7 years ago

Replying to ocean90:

Not sure if we really have to be consistent with a naming which is semantically incorrect. But maybe we can provide both functions? restore_current_locale() would empty the stack and set the locale to $original_locale.

Added in the latest patch.

Can't we move it to default-filters.php as add_filter( 'locale', array( $GLOBALS['wp_locale_switcher'], 'filter_locale' );? Or a simple init() method which gets called in wp-settings.php.

default-filters.php is loaded in wp-settings.php on line 113, while $GLOBALS['wp_locale_switcher'] is initialized on line 395, so that doesn't really work. Going with the init() method for now.

Replying to ocean90:

Something I missed to mention earlier: There is an issue with functions like get_post_type_labels()/_get_custom_object_labels(). Post type labels are initially registered via register_post_type() which means they don't change when calling switch_to_locale(). You can see that with the example plugin in the Add New menu for the Post and Page items.

Ugh, I experienced that once when testing but thought it was fixed eventually. Since post type labels are cached it's a bit hacky to reload them. The latest patch tries it anyway.

Replying to tfrommen:

There are even more issues, for example, when you have a Custom Post Type or Custom Taxonomy registered by using a translated slug. When you switch to a different locale, the rewrite rules (and thus the generated permalinks) do not get adapted.

The question is if coping with these things should be handled by the locale switcher, or not.

We definitely can't accommodate for rewrite rules (especially as long as they are stored in the database, see #36292) as we'd have no idea which part was translated.

#64 @swissspidy
7 years ago

Actually, 26511.6.diff won't work for post type labels (though I want to keep the new methods).

_get_custom_object_labels() has some logic to add specific labels, like name_admin_bar. If that label isn't set during post type registration, it falls back to singular_name. However, it is set for all the built-in post types. This means it gets lost when simply calling $post_type->labels = get_post_type_labels( $post_type );

@swissspidy
7 years ago

#65 @swissspidy
7 years ago

Today I've chatted about this with @ocean90. The easiest way to support re-initialization like of post type labels would be to support passing a callback function for labels instead of an array — for post types, post status and taxonomies. That way, post type labels would always get translated again.

Passing callbacks for the built-in objects would result in plenty of new functions added to core just to provide these labels. A class could make sense there. As you can see, this would add more complexity and needs some more consideration.

For now, in 26511.7.diff I just re-register the built-in post types and taxonomies upon changing the locale.

Alternatively, WP_Post_Type_Labels.diff is a WIP of what I've toyed with today. It essentially replaces the post type labels object with a new class that returns the labels on the fly (note: includes tests from #38157). The downside is that labels are never stored in an array but always translated upon request.

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


7 years ago

#67 @ocean90
7 years ago

  • Priority changed from normal to high

#29783 is landed so this issue needs to be resolved.

26511.7.diff looks like it has enough abstraction to allow core and plugins to run extra actions when a locale is switched. I'm currently not sure what impact calling create_initial_post_types()/create_initial_taxonomies() multiple times has, this needs some research (and tests).

WP_Post_Type_Labels.diff should be in its own ticket to gather more feedback.

The main use case for this function in core will be to send emails in the language of the site or a user. We'd need a patch for that.

#68 @swissspidy
7 years ago

Pressure's on I guess :-)

Just noticed that add_action( 'change_local', 'create_initial_taxonomies' ); needs to be add_action( 'change_locale', 'create_initial_taxonomies' ); in the latest patch. I think the impact of that approach is neglectable as it's reduced to a minimum.

I opened #38218 for suggesting a WP_Post_Type_Labels class though as it would be more maintainable.

Anyway, it's only a problem when doing something like switching the locale for the admin bar on the front end.

The main use case for this function in core will be to send emails in the language of the site or a user. We'd need a patch for that.

Happy to provide one. Post type labels should be irrelevant for that, so it should be easy.

#69 @swissspidy
7 years ago

At first glance it looks like the following functions and files need to be modified:

  • update_option_new_admin_email()
  • send_confirmation_on_profile_email()
  • wp_new_blog_notification()
  • wp-admin/user-new.php
  • wp-admin/ms-delete-site.php
  • wp_notify_postauthor()
  • wp_notify_moderator()
  • wp_new_user_notification()
  • wp_update_user()
  • wpmu_signup_blog_notification()
  • wpmu_signup_user_notification()
  • wpmu_welcome_notification()
  • wpmu_welcome_user_notification()

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


7 years ago

#71 @swissspidy
7 years ago

  • Owner set to swissspidy
  • Status changed from reviewing to accepted

#72 @swissspidy
7 years ago

An is_locale_switched() function analog to ms_is_switched() might be worth adding.

@swissspidy
7 years ago

#73 @swissspidy
7 years ago

In 26511.8.diff:

  • Introduces is_locale_switched() including tests.
  • Switch locale in the files / functions mentioned above. Exceptions: wpmu_signup_blog_notification() and wpmu_signup_user_notification() (no user exists yet) as well as wp_notify_postauthor(), wp_notify_moderator() (need to be refactored). No tests included yet.

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


7 years ago

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


7 years ago

#76 @desrosj
7 years ago

This needs some testing, and some more work on the last patch.

#77 @swissspidy
7 years ago

  • Keywords needs-dev-note added

#78 follow-up: @ocean90
7 years ago

Some quick feedback for 26511.8.diff:

  • wp_new_user_notification(): The email for admin_email should be sent using the site locale (get_locale()).
  • wp_update_user(): When $user['locale'] isn't set get_locale() should be used for the switch_to_locale() call.
  • ms-delete-site.php: Why is this using get_user_locale()? I think it should be get_locale().
  • wpmu_signup_blog_notification(): The function is only used on wp-signup.php, the front end. switch_to_locale() would only be needed if it's used in the admin. Maybe there are plugins which are using the function in the admin?
  • wpmu_signup_user_notification(): Unlike wpmu_signup_blog_notification(), wpmu_signup_user_notification() is also used in the admin on wp-admin/user-new.php for multisite. If the user already exists it should use their locale, otherwise get_locale().
  • wp_notify_moderator(): Well, that's a mess. But we should at least make sure that it's always using the site locale.

@swissspidy
7 years ago

#79 in reply to: ↑ 78 ; follow-up: @swissspidy
7 years ago

  • wp_new_user_notification(): The email for admin_email should be sent using the site locale (get_locale()).

Makes sense. I somehow didn't think of that scenario (is_admin() being true and thus the locale "wrong") while working on the latest patch. Fixed in 26511.9.diff.

  • wp_update_user(): When $user['locale'] isn't set get_locale() should be used for the switch_to_locale() call.
  • ms-delete-site.php: Why is this using get_user_locale()? I think it should be get_locale().
  • wpmu_signup_user_notification(): Unlike wpmu_signup_blog_notification(), wpmu_signup_user_notification() is also used in the admin on wp-admin/user-new.php for multisite. If the user already exists it should use their locale, otherwise get_locale().

Fixed in the latest patch.

  • wpmu_signup_blog_notification(): The function is only used on wp-signup.php, the front end. switch_to_locale() would only be needed if it's used in the admin. Maybe there are plugins which are using the function in the admin?

Better to be safe than sorry.

  • wp_notify_moderator(): Well, that's a mess. But we should at least make sure that it's always using the site locale.

Did that now, but also added that in wp_notify_postauthor().

#80 in reply to: ↑ 79 ; follow-up: @ocean90
7 years ago

The changes are looking good so far but some tests are failing: https://travis-ci.org/aaronjorbin/develop.wordpress/builds/168645925

@swissspidy
7 years ago

#81 in reply to: ↑ 80 @swissspidy
7 years ago

Replying to ocean90:

The changes are looking good so far but some tests are failing: https://travis-ci.org/aaronjorbin/develop.wordpress/builds/168645925

26511.10.diff addresses the undefined variable notices and the failing test_restore_current_locale_without_switching() test. Also uses assertTrue( ! … ) everywhere instead of assertFalse() (might not be available on PHP 5.2).

Note: to make the other tests pass, upload de_DE.zip with the po & mo files to your PR.

@ocean90
7 years ago

#82 follow-up: @ocean90
7 years ago

26511.11.diff renames $switched to $switched_locale.

I also added a missing restore_previous_locale() in wp_new_blog_notification(). But I think we can remove the locale switch because the function is only used for a new install see wp_install().

#83 in reply to: ↑ 82 ; follow-up: @swissspidy
7 years ago

Replying to ocean90:

I think we can remove the locale switch because the function is only used for a new install see wp_install().

Agreed. Any other objections on the patch? Does the PR work with the latest patch + the files from the ZIP file applied?

#84 in reply to: ↑ 83 @ocean90
7 years ago

Replying to swissspidy:

Any other objections on the patch? Does the PR work with the latest patch + the files from the ZIP file applied?

For some reasons the build failed twice: https://travis-ci.org/aaronjorbin/develop.wordpress/builds/168847074

I've to review some of the switch_to_locale( $user->locale ) again to see how an empty value affects the language of an email.

#85 follow-up: @swissspidy
7 years ago

The PR still doesn't seem to contain the updated patch, or am I missing something?

Just noticed that the in_array() check in the class isn't strict yet.

#86 in reply to: ↑ 85 ; follow-up: @ocean90
7 years ago

Replying to swissspidy:

The PR still doesn't seem to contain the updated patch, or am I missing something?

I made a new one and it's still failing for PHP < 7.0, see https://travis-ci.org/aaronjorbin/develop.wordpress/builds/169711596

Looks like it gets stuck at Starting test 'Tests_L10n_loadTextdomainJustInTime::test_theme_translation_after_switching_locale':

...
Starting test 'Tests_L10n_loadTextdomainJustInTime::test_theme_translation_should_be_translated_without_calling_load_theme_textdomain'.
.
Starting test 'Tests_L10n_loadTextdomainJustInTime::test_get_translations_for_domain_does_not_return_null_if_override_load_textdomain_is_used'.
.
Starting test 'Tests_L10n_loadTextdomainJustInTime::test_should_allow_unloading_of_text_domain'.
.
Starting test 'Tests_L10n_loadTextdomainJustInTime::test_plugin_translation_after_switching_locale'.
. 3904 / 7369 ( 52%)

Starting test 'Tests_L10n_loadTextdomainJustInTime::test_theme_translation_after_switching_locale'.

#87 in reply to: ↑ 86 @ocean90
7 years ago

Replying to ocean90:

There is an infinite loop in the WP_Locale_Switcher::load_translations() foreach part.
To test this you can add fwrite( STDERR, print_r( $domain, true ) ); before unload_textdomain( $domain ); and then run test_plugin_translation_after_switching_locale() and test_theme_translation_after_switching_locale() in a group. The output will look like

internationalized-plugin
internationalized-theme
internationalized-plugin
internationalized-theme
internationalized-plugin
internationalized-theme
....

@swissspidy
7 years ago

#88 @swissspidy
7 years ago

In 26511.12.diff:

  • Make in_array() check strict.
  • Fix filter in default-filters.php.

@ocean90 Thanks for investigating the infinite loop. Unfortunately I don't have an older PHP version handy right now, but it sounds like #37997 would have an effect on this.

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


7 years ago

@swissspidy
7 years ago

#90 follow-up: @swissspidy
7 years ago

The $l10n array should obviously not be modified while looping over it. 26511.13.diff fixes this by copying the array first.
If copying the whole array takes too long, we could also just loop over array_keys( $l10n ) and then use $l10n[ $domain ]->get_filename().

Travis CI build: https://travis-ci.org/aaronjorbin/develop.wordpress/builds/170410107

@swissspidy
7 years ago

#91 in reply to: ↑ 90 @swissspidy
7 years ago

Replying to swissspidy:

If copying the whole array takes too long, we could also just loop over array_keys( $l10n ) and then use $l10n[ $domain ]->get_filename().

See 26511.14.diff

@ocean90
7 years ago

#92 @ocean90
7 years ago

Using array_keys() like in 26511.14.diff is fine.

26511.15.diff adds a missing switch to wp_notify_postauthor() and adds test_switch_to_locale_en_US() which is currently failing. get_available_languages() doesn't include en_US since there are usually no language files for en_US.

#93 @ocean90
7 years ago

26511.15.1.diff:

- $this->available_languages = get_available_languages();
+ $this->available_languages = array_merge( array( 'en_US' ), get_available_languages() );

@ocean90
7 years ago

#94 @ocean90
7 years ago

Um, found another bigger issue: switch_to_locale( get_locale() ) doesn't really work because of the get_locale() === $locale check in WP_Locale_Switcher::switch_to_locale().

Steps to reproduce:

  • Site language is en_US
  • User language is de_DE
  • Reply to a comment on wp-admin/edit-comments.php

Expected: Language of the mail is en_US
Actual: The mail was sent using user's language

#95 @swissspidy
7 years ago

Sounds like the get_locale() === $locale check should be removed. Make it the caller's responsibility to check for this and/or doing any ( is_admin() && get_user_locale() === $locale ) checks.

@ocean90
7 years ago

#96 @ocean90
7 years ago

26511.16.diff: Changes the check to use get_user_locale() in admin.

$current_locale = is_admin() ? get_user_locale() : get_locale(); 
if ( $current_locale === $locale ) { 
    return false; 
}

There is a new test test_switch_to_site_locale_if_user_locale_is_set(). It's currently failing because restore_current_locale() doesn't restore $l10n. Hmm, that makes actually sense since the value was empty before because of en_US. Need to test this with another locale…

@ocean90
7 years ago

@ocean90
7 years ago

@swissspidy
7 years ago

#97 @swissspidy
7 years ago

26511.17.diff is the latest round of iteration. Changes how the locale switcher class treats the user locale in the admin and adds another test case there.

Will try to add a new test for multiple site locale & user locale switches early in the morning.

#98 @ocean90
7 years ago

In 38930:

Tests: Update language files to include a Language header.

See #26511.

@ocean90
7 years ago

#99 @ocean90
7 years ago

26511.18.diff includes two more tests for site locale & user locale switches. It also fixes test pollution by missing resets of the $l10n and $l10n_unload globals and missing set_current_screen( 'front' ); calls.

@ocean90
7 years ago

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


7 years ago

#101 @ocean90
7 years ago

In 38955:

I18N: Add $user_id argument to get_user_locale().

This allows to retrieve the locale of any user with the additional fallback to the site locale.

Fixes #38512.
See #29783, #26511.

@ocean90
7 years ago

#102 @ocean90
7 years ago

  • Keywords commit added

26511.19.diff removes the switch from wp_new_blog_notification() (see comment:82) and uses get_user_locale().

#103 @ocean90
7 years ago

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

In 38961:

I18N: Introduce a locale-switching function.

With the introduction of user-specific languages in [38705] it's necessary to be able to switch translations on the fly. For example emails should be sent in the language of the recipient and not the one of the current user.

This introduces a new WP_Locale_Switcher class which is used for switching locales and translations. It holds the stack of locales whenever switch_to_locale( $locale ) is called. With restore_previous_locale() you can restore the previous locale. restore_current_locale() empties the stack and sets the locale back to the initial value.

switch_to_locale() is added to most of core's email functions, either with the value of get_locale() (site language) or get_user_locale() (user language with fallback to site language).

Props yoavf, tfrommen, swissspidy, pbearne, ocean90.
See #29783.
Fixes #26511.

#104 @isuke01
4 years ago

Almost working ...
If user langauge is same as language of page or user lang is default (so it is same as page language)
it will work.

But if User langauge is different than page language, it does not working, actually it produces fatal error of memory if you try to debug it (loadtext_domain produce this error), I have tried to allocate 5gb of memory, still issue.

#105 @desrosj
4 years ago

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

#106 @ocean90
3 years ago

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.

#107 @ocean90
3 years ago

In 49566:

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

See #39210, #51678, #26511.

#108 @swissspidy
20 months ago

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.

#109 @swissspidy
14 months ago

In 55161:

I18N: Introduce switch_to_user_locale().

This new function makes it easier to switch to a specific user’s locale by reducing duplicate code and storing the user’s ID as additional context for plugins to consume. Existing usage of switch_to_locale() in core has been replaced with switch_to_user_locale() where appropriate.

Also, this change ensures WP_Locale_Switcher properly filters determine_locale so that anyyone using the determine_locale() function will get the correct locale information when switching is in effect.

Props costdev.
Fixes #57123.
See #26511.

Note: See TracTickets for help on using tickets.