WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#38485 closed defect (bug) (fixed)

Some part of the UI can be in the wrong language (edge cases)

Reported by: afercia Owned by: ocean90
Milestone: 4.7 Priority: high
Severity: normal Version: 4.7
Component: I18N Keywords: has-screenshots has-unit-tests has-patch
Focuses: Cc:
PR Number:

Description

Hunting for edge cases in the admin, noticed a couple of things. To reproduce on trunk, set your user language and site language to different values, e.g.:

  • User profile > Language > English
  • Settings > Site Language > Deutsch

First edge case:

the admin is in English but the release video captions default language is German, the link to the video is:
https://videopress.com/embed/GbdhpGF3?hd=true&defaultLangCode=de

https://cldup.com/kj7jdlw9Iv.jpg

Second edge case:

on the settings page, the Timezone select shows the Cities in German (very noticeable for some cities, I've just highlighted a few):

https://cldup.com/-cX41IDClu.png

I'd expect both to be in English, since they're part of the admin interface.

Attachments (12)

38485.patch (711 bytes) - added by ocean90 3 years ago.
384851.1.diff (490 bytes) - added by yale01 3 years ago.
fix for wp_timezone_choice
384851.2.diff (722 bytes) - added by yale01 3 years ago.
rework on my previous patch
384851.3.diff (2.2 KB) - added by yale01 3 years ago.
added locale argument
38485.diff (1.9 KB) - added by swissspidy 3 years ago.
38485.2.diff (2.1 KB) - added by swissspidy 3 years ago.
38485-load-textdomain.diff (1.5 KB) - added by swissspidy 3 years ago.
38485-load-textdomain.2.diff (1.8 KB) - added by swissspidy 3 years ago.
Use user locale in _load_textdomain_just_in_time()
38485-load-textdomain.3.diff (1.8 KB) - added by ocean90 3 years ago.
38485-load-textdomain.4.diff (15.7 KB) - added by swissspidy 3 years ago.
38485-load-textdomain.5.diff (15.6 KB) - added by swissspidy 3 years ago.
adminbar-language.jpg (121.6 KB) - added by Presskopp 3 years ago.

Download all attachments as: .zip

Change History (52)

#1 @swissspidy
3 years ago

  • Keywords needs-patch added
  • Owner set to swissspidy
  • Status changed from new to assigned

Introduced in [38705] / #29783.

@ocean90
3 years ago

#2 @ocean90
3 years ago

38485.patch fixes about.php. The other one is wp_timezone_choice() which uses a static var to load the mo file on-the-fly and only once.

@yale01
3 years ago

fix for wp_timezone_choice

#3 @yale01
3 years ago

https://core.trac.wordpress.org/attachment/ticket/38485/384851.1.diff fixes determining user locale in wp_timezone_choice()

@yale01
3 years ago

rework on my previous patch

#4 @yale01
3 years ago

in 384851.2.diff, the value of current user locale is assigned to the static var $mo_loaded on first run, so when it changes, the new value of $locale is different from the $mo_loaded and the continents-cities get updated

#5 @swissspidy
3 years ago

  • Keywords has-patch added; needs-patch removed

#6 @ocean90
3 years ago

@yale01 Thanks for your patch! Calling load_textdomain() a second time for the same textdomain doesn't work at the moment, there needs to be a call to unload_textdomain() first.

For back-compat I think wp_timezone_choice() should get an optional $locale parameter which falls back to get_locale() if it's not set. When calling wp_timezone_choice() in the admin we should pass get_user_locale() to the function.

Please take also a look at our coding standards for PHP, there are some missing spaces in your patch.

#7 follow-up: @ocean90
3 years ago

In 38952:

About Page: Use get_user_language() for the video subtitles.

Rename $locale to $lang_code to avoid stomping of the global $locale.

See #38485.

@yale01
3 years ago

added locale argument

#8 @yale01
3 years ago

Thank you @ocean90.

New patch adds $locale argument to wp_timezone_choice() with default to get_locale() and unloads text domain if already loaded.
In admin options, wp_timezone_choice() is now called with get_user_locale() argument.

#9 in reply to: ↑ 7 @ocean90
3 years ago

Replying to ocean90:

In 38952:

About Page: Use get_user_language() for the video subtitles.

Rename $locale to $lang_code to avoid stomping of the global $locale.

See #38485.

s/get_user_language/get_user_locale/

@swissspidy
3 years ago

#10 @swissspidy
3 years ago

@yale01 Looks like your patch always loads the text domain from scratch, even when no locale is passed.

Uploaded a new patch, see 38485.diff. It would be great if y'all could give it a try. Works great on my install.

#11 @yale01
3 years ago

Hi @swissspidy, you're right, and I also realized later that the diff filename is wrong. Apologies :(

Your patch is clearly better, but it still re-loads the text domain when the $locale argument has the same not null value in different calls, and it can go worse if in the first call it has a value and the second one is null.

Maybe we could store the $locale value in a static var and compare it against the fresh argument ?

@swissspidy
3 years ago

#12 @swissspidy
3 years ago

Yeah a static var is probably the easiest solution, even though it makes the code even more untestable. See 38485.2.diff for a new patch.

#13 @davecpage
3 years ago

In testing i've noticed that plugins which use load_plugin_textdomain such as Akismet show in the site language e.g. Spanish, even though the user language is set to English. This appears to be because within the function it uses get_locale() https://core.trac.wordpress.org/browser/trunk/src/wp-includes/l10n.php#L796 rather than the newer get_user_locale().

However does this raise the problem of plugins which have a mix of content in the admin and the frontend?

#14 @swissspidy
3 years ago

  • Priority changed from normal to high

@davecpage Good catch, thanks! 38485-load-textdomain.diff addresses this.

However does this raise the problem of plugins which have a mix of content in the admin and the frontend?

Using that patch, a different translation will be loaded on the front end and the back end, so no problems there.

@swissspidy
3 years ago

Use user locale in _load_textdomain_just_in_time()

#15 @swissspidy
3 years ago

In 39068:

I18N: Show available timezones in the user's locale on the settings screen.

Adds a $locale parameter to wp_timezone_choice() to only reload translations when necessary.

Props yale01.
See #38485.

#16 @swissspidy
3 years ago

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

In 39069:

I18N: Use the user's locale when loading text domains in the admin.

Leverages get_user_locale() in load_*_textdomain() and _load_textdomain_just_in_time() to always load translations in the user's language when in the admin.

Fixes #38485.

#17 @johnbillion
3 years ago

@swissspidy How does this, and [39069] in particular, affect admin-ajax.php calls on the front end? Could it result in an Ajax call returning a response in the user's language when the front end of the site is shown in the site language?

#18 @eirikurn
3 years ago

Hey @swissspidy,

The change broke our vip-quickstart staging server (which is updating to svn master every hour).

It looks like the vip-scanner plugin is translating some strings when it's imported. This causes a call to get_user_locale, but wp_get_current_user has not been defined yet, as pluggable.php is imported later.

#19 @swissspidy
3 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

@johnbillion Good reminder. The whole user language feature actually affects Ajax calls. I'll have a closer look tomorrow.

@eirikurn Is the code of the plugin available somewhere so I can have a look?

#20 @eirikurn
3 years ago

VIP Scanner Plugin: https://github.com/Automattic/vip-scanner
VIP Quickstart environment (autoupdates to Wordpress trunk): https://github.com/Automattic/vip-quickstart

Thank you,

#21 @eirikurn
3 years ago

Here's the stack trace:

<b>Fatal error</b>: Uncaught Error: Call to undefined function wp_get_current_user() in /srv/www/wp/wp-includes/l10n.php:92
Stack trace:
#0 /srv/www/wp/wp-includes/l10n.php(868): get_user_locale()
#1 /srv/www/wp/wp-includes/l10n.php(896): _load_textdomain_just_in_time('vip-scanner')
#2 /srv/www/wp/wp-includes/l10n.php(122): get_translations_for_domain('vip-scanner')
#3 /srv/www/wp/wp-includes/l10n.php(202): translate('Name of theme', 'vip-scanner')
#4 /srv/www/wp-content/plugins/vip-scanner/vip-scanner/config-vip-scanner-wpcom-form.php(6): __('Name of theme', 'vip-scanner')
#5 /srv/www/wp-content/plugins/vip-scanner/vip-scanner/config-vip-scanner.php(4): include_once('/srv/www/wp-con...')
#6 /srv/www/wp-content/plugins/vip-scanner/vip-scanner.php(47): require_once('/srv/www/wp-con...')
#7 /srv/www/wp/wp-settings.php(263): include_once('/srv/www/wp-con...')
#8 /srv/www/wp-config.php(97): require_once('/srv/www/wp/wp-...')
#9 /srv/www/wp/wp-load.php(42): require_once('/srv/www/wp-con...')
#10 /srv/www/wp/wp-admin/admin.php(31): re in <b>/srv/www/wp/wp-includes/l10n.php</b> on line <b>92</b>

I just verified that this also broke the staging site of another of our VIP sites. So I would think that all vendors running vip-quickstart are unable to log into the admin after tonight.

#22 @swissspidy
3 years ago

  • Keywords needs-unit-tests added

#23 follow-up: @johnbillion
3 years ago

In 39070:

I18N: Revert [39069] as it needs some more work.

See #38485

#24 @AramZS
3 years ago

I can confirm this issue.

After some testing it is clear that some plugins are calling the translation functions before the user functions proposed to be added to the translation process in this ticket have been setup. As a result, anyone calling _x or load_plugin_textdomain at an earlier level than the intended action call of init (those functions may be available earlier than init, but that's what I tested with) described in the documentation will cause a fatal PHP error when the translation function attempts to call the user function before it has been set up.

Arguably these errors would only occur if the plugin's text domain is setup outside of the recommended methodology, but considering those setups worked perfectly before this patch and then cause total site failure after, perhaps we want some gentler transition or depreciation process?

#25 in reply to: ↑ 23 @ocean90
3 years ago

Replying to johnbillion:

In 39070:

I18N: Revert [39069] as it needs some more work.

See #38485

I think the "some more work" is adding a function_exists( 'wp_get_current_user' ) call to get_user_locale().

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


3 years ago

#27 @ocean90
3 years ago

  • Owner changed from swissspidy to ocean90
  • Status changed from reopened to accepted

#28 @ocean90
3 years ago

38485-load-textdomain.3.diff is 38485-load-textdomain.2.diff plus the use of function_exists( 'wp_get_current_user' ). Just like get_current_user_id() does.

#29 @swissspidy
3 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

38485-load-textdomain.4.diff adds a bunch of tests to ensure these functions all use get_user_locale() in the admin.

Anyone getting translated strings via Ajax will get strings in the user's locale when they are logged in (as admin-ajax.php is in the admin, after all). Developers should use switch_to_locale() if that's not desired or ideally leverage the REST API. We'll document this in the dev-note.

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


3 years ago

#31 @swissspidy
3 years ago

In 39125:

I18N: Move load_textdomain() tests to separate file.

See #38485.

#32 @swissspidy
3 years ago

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

In 39127:

I18N: Use the user's locale when loading text domains in the admin.

Leverages get_user_locale() in load_*_textdomain() and _load_textdomain_just_in_time() to always load translations in the user's language when in the admin.

This re-introduces [39069], but now with additional tests and a function_exists( 'wp_get_current_user' ) check in get_user_locale() in case it gets used early.

Props swissspidy, ocean90.
Fixes #38485.

#33 @westonruter
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

r39127 is causing a fatal error in Customize Posts plugin:

Uncaught Error: Call to undefined function get_user_by() in /srv/www/wordpress-develop/src/wp-includes/l10n.php:96
Stack trace:
#0 /srv/www/wordpress-develop/src/wp-includes/l10n.php(713): get_user_locale()
#1 /srv/www/wordpress-develop/src/wp-content/plugins/customize-posts/php/class-customize-posts-plugin.php(55): load_plugin_textdomain('customize-posts')
#2 /srv/www/wordpress-develop/src/wp-content/plugins/customize-posts/customize-posts.php(34): Customize_Posts_Plugin->__construct()
#3 /srv/www/wordpress-develop/src/wp-settings.php(298): include_once('/srv/www/wordpr...')
#4 /srv/www/wordpress-develop/src/wp-config.php(114): require_once('/srv/www/wordpr...')
#5 /srv/www/wordpress-develop/src/wp-load.php(37): require_once('/srv/www/wordpr...')
#6 /srv/www/wordpress-develop/src/wp-admin/admin.php(31): require_once('/srv/www/wordpr...')
#7 /srv/www/wordpress-develop/src/wp-admin/customize.php(13): require_once('/srv/www/wordpr...')
#8 {main}
  thrown in <b>/srv/www/wordpress-develop/src/wp-includes/l10n.php

#34 @ocean90
3 years ago

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

In 39134:

I18N: In get_user_locale() ensure that $user_id isn't a falsy value before calling get_user_by().

Fixes #38485.

#35 @Presskopp
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

When I set site language to english, but in user profile german (or the other way 'round) the top admin bar shows in the opposite language.

#36 @wajiharizvi29
3 years ago

  • Keywords has-patch added; needs-patch removed

#37 @swissspidy
3 years ago

@Presskopp Do you mean in the back end or the front end? Because on the front end that's currently expected.

#38 @Presskopp
3 years ago

I made a screenshot. I don't see this as expected, if it is. I can do the same when I turn around english/german. Should the adminbar not reflect the language setting in profile?

#39 @ocean90
3 years ago

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

@Presskopp It's currently expected because we don't use user's locale for the front end. See #38643 for changing that in a future release.

#40 @Presskopp
3 years ago

Perfect. Missed that one. Thank you.

Note: See TracTickets for help on using tickets.