Opened 3 years ago
Closed 7 days ago
#26511 closed feature request (fixed)
Introduce a locale-switching function
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.7 | Priority: | high |
Severity: | normal | Version: | |
Component: | I18N | Keywords: | has-patch has-unit-tests needs-dev-note commit |
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)
Change History (134)
#2
@SergeyBiryukov
3 years ago
Previously: #1550
#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.
18 months ago
#8
@rmccue
18 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. :)
This ticket was mentioned in Slack in #core-i18n by ocean90. View the logs.
7 months ago
#11
@SergeyBiryukov
7 months ago
#32879 was marked as a duplicate.
#12
@SergeyBiryukov
7 months ago
- Milestone changed from Awaiting Review to 4.6
This ticket was mentioned in Slack in #core-i18n by ocean90. View the logs.
6 months ago
#14
follow-up:
↓ 15
@ocean90
6 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
6 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
6 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
6 months ago
A refreshed version of the WordPress.com functions - PHP 5.2 compatible, and object-oriented.
#17
@tfrommen
6 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.
6 months ago
This ticket was mentioned in Slack in #core-i18n by tfrommen. View the logs.
6 months ago
This ticket was mentioned in Slack in #core by tfrommen. View the logs.
5 months ago
#21
@pbearne
5 months ago
We have just been working on some unit test and found few problems / bugs with new switcher class
see new patch
#22
@tloureiro
5 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
5 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
5 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.
5 months ago
This ticket was mentioned in Slack in #core by ocean90. View the logs.
4 months ago
#29
@ocean90
4 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.
#30
@tfrommen
3 months 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.
2 months ago
#32
@SergeyBiryukov
2 months ago
- Milestone changed from Future Release to 4.7
Any way to avoid introducing a new global? See #37699.
#33
@pbearne
2 months 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
2 months 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.
@swissspidy
2 months ago
#35
@swissspidy
2 months 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
2 months 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:
↓ 39
@swissspidy
2 months 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
2 months ago
#38
@swissspidy
2 months 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
2 months 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
2 months 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:
↓ 46
↓ 48
@ocean90
2 months 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.)
#43
follow-up:
↓ 44
@johnjamesjacoby
8 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:
↓ 45
@tfrommen
8 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:
↓ 49
@johnjamesjacoby
8 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
8 weeks ago
#46
in reply to:
↑ 41
;
follow-up:
↓ 50
@swissspidy
8 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
8 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
8 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
8 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.
#50
in reply to:
↑ 46
@ocean90
8 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.
8 weeks ago
@swissspidy
8 weeks ago
#52
follow-up:
↓ 53
@swissspidy
8 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
8 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
6 weeks ago
Opened a new ticket to discuss naming convention: #38098.
#55
follow-up:
↓ 56
@ocean90
6 weeks 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:
↓ 57
@swissspidy
6 weeks 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
6 weeks 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
6 weeks ago
#58
follow-up:
↓ 60
@swissspidy
6 weeks 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.
6 weeks ago
#60
in reply to:
↑ 58
@ocean90
6 weeks 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:
↓ 62
↓ 63
@ocean90
6 weeks 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
6 weeks 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
6 weeks ago
#63
in reply to:
↑ 61
@swissspidy
6 weeks 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
5 weeks 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
5 weeks ago
@swissspidy
5 weeks ago
#65
@swissspidy
5 weeks 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.
5 weeks ago
#67
@ocean90
4 weeks 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
4 weeks 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
4 weeks 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.
4 weeks ago
#71
@swissspidy
3 weeks ago
- Owner set to swissspidy
- Status changed from reviewing to accepted
#72
@swissspidy
3 weeks ago
An is_locale_switched() function analog to ms_is_switched() might be worth adding.
@swissspidy
3 weeks ago
#73
@swissspidy
3 weeks 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.
3 weeks ago
This ticket was mentioned in Slack in #core by desrosj. View the logs.
2 weeks ago
#77
@swissspidy
2 weeks ago
- Keywords needs-dev-note added
#78
follow-up:
↓ 79
@ocean90
2 weeks 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
2 weeks ago
#79
in reply to:
↑ 78
;
follow-up:
↓ 80
@swissspidy
2 weeks 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:
↓ 81
@ocean90
2 weeks ago
The changes are looking good so far but some tests are failing: https://travis-ci.org/aaronjorbin/develop.wordpress/builds/168645925
@swissspidy
2 weeks ago
#81
in reply to:
↑ 80
@swissspidy
2 weeks 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.
#82
follow-up:
↓ 83
@ocean90
2 weeks 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:
↓ 84
@swissspidy
2 weeks 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
2 weeks 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:
↓ 86
@swissspidy
2 weeks 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:
↓ 87
@ocean90
11 days 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
11 days 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
10 days ago
#88
@swissspidy
10 days 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.
9 days ago
@swissspidy
8 days ago
#90
follow-up:
↓ 91
@swissspidy
8 days 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
8 days ago
#91
in reply to:
↑ 90
@swissspidy
8 days 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
#92
@ocean90
8 days 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
8 days ago
- $this->available_languages = get_available_languages(); + $this->available_languages = array_merge( array( 'en_US' ), get_available_languages() );
#94
@ocean90
8 days 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
8 days 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.
#96
@ocean90
8 days 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…
@swissspidy
8 days ago
#97
@swissspidy
8 days 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.
#99
@ocean90
7 days 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.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
7 days ago
#102
@ocean90
7 days ago
- Keywords commit added
26511.19.diff removes the switch from wp_new_blog_notification() (see comment:82) and uses get_user_locale().
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().