WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 43 hours ago

#26511 reviewing feature request

Introduce a locale-switching function

Reported by: johnbillion Owned by:
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: I18N Keywords: has-patch has-unit-tests
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 (15)

switch_to_locale.php (3.7 KB) - added by yoavf 2 years ago.
switch_to_locale__revisited.php (5.0 KB) - added by tfrommen 4 months ago.
A refreshed version of the WordPress.com functions - PHP 5.2 compatible, and object-oriented.
26511.patch (38.2 KB) - added by tfrommen 4 months ago.
switch_locale.patch (42.9 KB) - added by pbearne 4 months ago.
updated patch with Unit tests (start off)
26511.2.patch (8.4 KB) - added by ocean90 3 months ago.
26511.3.patch (8.8 KB) - added by tfrommen 6 weeks ago.
Refreshed patch for 4.7
26511.diff (10.9 KB) - added by swissspidy 4 weeks ago.
26511.2.diff (12.3 KB) - added by swissspidy 3 weeks ago.
26511.3.diff (12.2 KB) - added by swissspidy 3 weeks ago.
de_DE.zip (2.2 KB) - added by swissspidy 3 weeks ago.
5 de_DE translations to check number_format in unit tests
26511.4.diff (14.5 KB) - added by swissspidy 3 weeks ago.
26511.5.diff (14.9 KB) - added by swissspidy 7 days ago.
26511.6.diff (16.5 KB) - added by swissspidy 4 days ago.
WP_Post_Type_Labels.diff (14.4 KB) - added by swissspidy 43 hours ago.
26511.7.diff (17.5 KB) - added by swissspidy 43 hours ago.

Download all attachments as: .zip

Change History (80)

#1 @nacin
3 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().

#2 @SergeyBiryukov
3 years ago

Previously: #1550

#3 @emzo
3 years ago

  • Cc wordpress@… added

#4 @dimadin
3 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
2 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
2 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.


17 months ago

#8 @rmccue
17 months 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
6 months ago

Related: #32879

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


5 months ago

#11 @SergeyBiryukov
5 months ago

#32879 was marked as a duplicate.

#12 @SergeyBiryukov
5 months ago

  • Milestone changed from Awaiting Review to 4.6

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


5 months ago

#14 follow-up: @ocean90
5 months 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
5 months 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
4 months 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
4 months ago

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

#17 @tfrommen
4 months 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.


4 months ago

@tfrommen
4 months ago

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


4 months ago

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


4 months ago

#21 @pbearne
4 months ago

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

see new patch

@pbearne
4 months ago

updated patch with Unit tests (start off)

#22 @tloureiro
4 months 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
4 months 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
4 months 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.


4 months ago

#26 @ocean90
4 months ago

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

#27 @ocean90
3 months 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
3 months ago

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


3 months ago

#29 @ocean90
3 months 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
6 weeks ago

Refreshed patch for 4.7

#30 @tfrommen
6 weeks 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.


4 weeks ago

#32 @SergeyBiryukov
4 weeks ago

  • Milestone changed from Future Release to 4.7

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

#33 @pbearne
4 weeks 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
4 weeks 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 4 weeks ago by swissspidy (previous) (diff)

@swissspidy
4 weeks ago

#35 @swissspidy
4 weeks 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
3 weeks 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
3 weeks 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
3 weeks ago

#38 @swissspidy
3 weeks 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
3 weeks 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
3 weeks 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
3 weeks 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
3 weeks ago

  • Owner ocean90 deleted

#43 follow-up: @johnjamesjacoby
3 weeks 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
3 weeks 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
3 weeks 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
3 weeks ago

#46 in reply to: ↑ 41 ; follow-up: @swissspidy
3 weeks 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
3 weeks 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
3 weeks 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
3 weeks 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
3 weeks ago

5 de_DE translations to check number_format in unit tests

#50 in reply to: ↑ 46 @ocean90
3 weeks 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.


3 weeks ago

@swissspidy
3 weeks ago

#52 follow-up: @swissspidy
3 weeks 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
3 weeks 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
9 days ago

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

Last edited 9 days ago by swissspidy (previous) (diff)

#55 follow-up: @ocean90
7 days 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 days 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 days 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 days ago

#58 follow-up: @swissspidy
7 days 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 days ago

#60 in reply to: ↑ 58 @ocean90
4 days 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
4 days 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
4 days 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
4 days ago

#63 in reply to: ↑ 61 @swissspidy
4 days 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
3 days 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
43 hours ago

#65 @swissspidy
43 hours 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.

Note: See TracTickets for help on using tickets.