Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#44758 closed enhancement (fixed)

The REST API is not respecting the user's locale properly.

Reported by: youknowriad's profile youknowriad Owned by: pento's profile pento
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch fixed-5.0
Focuses: rest-api Cc:

Description

When calling REST API endpoints such as /taxonomies?context=edit, the labels returned are always using the default locale of the site even if the authenticated user has changed its locale in his profile.

Interestingly, if you use WP_REST_Request to call these same API endpoints with the same user, it uses the user's locale in that case.

1- We should be consistent and always use the authenticated user's locale
2- If we want the default behavior to be using the site's locale then we should have a way to pass the desired locale as an argument to these endpoints because in Gutenberg (using the REST API), users expect the UI to respect their profile's locale.

Related Gutenberg issue https://github.com/WordPress/gutenberg/issues/8449

Attachments (5)

44758.diff (6.9 KB) - added by TimothyBlynJacobs 6 years ago.
44758.2.diff (5.1 KB) - added by TimothyBlynJacobs 6 years ago.
44758.3.diff (7.1 KB) - added by swissspidy 6 years ago.
Account for wp-login.php, add filter
44758.4.diff (7.4 KB) - added by flixos90 6 years ago.
44758.5.diff (8.2 KB) - added by danielbachhuber 6 years ago.

Download all attachments as: .zip

Change History (52)

#1 @TimothyBlynJacobs
6 years ago

  • Type changed from defect (bug) to enhancement

I personally think that this should be a global server-level flag, like _locale. Probably just accepting default/site or user for now ( as opposed to accepting any installed language code ).

The crux of the issue is that Gutenberg is an admin application where the user locale is meant to apply, but the REST API runs in a front-end context where the site locale should apply. Defaulting to the user locale for all requests, if they are authorized, would be changing that behavior which was specifically mentioned in the original user locale release post, https://make.wordpress.org/core/2016/11/07/user-admin-languages-and-locale-switching-in-4-7/, and would effect front-end uses of the REST API by returning user localized data when site localized data is expected.

I believe the reason for the inconsistency when using WP_REST_Request is that Gutenberg is making these requests in the admin context, so the user defined locale is applied. If, however, internal requests were made in a “front-end” context, then the site locale should apply.

When the server would check this flag is important. I wonder if it needs to apply before authentication is checked. If so, this happens in the global request life-cycle which doesn’t apply to internal requests. The utility of it happening at that point is confusing to me. If the user doesn’t get authenticated, then theoretically we don’t have a user object to fetch the locale for, but in practice, I believe the current user is set before the rest_authentication_errors and we could have an issue with an expired nonce, in which case we could return the error message in the user’s locale if desired.

I believe this is a parameter that should be respected when doing internal requests as well. So it would seem, at least to me, that the server would also have to check the locale flag inside of WP_REST_Server::dispatch() and not change the locale if it has already been switched. Then if not serving the request because it is internal, restore the locale at the end of ::dispatch() otherwise, we’d need to wait until right before the response is echoed in WP_REST_Server::serve_request() to restore the locale. Or perhaps even when the actual die happens, not sure.

#2 @youknowriad
6 years ago

I'm happy with any option that would make this work.

I just wanted to react to this sentence that seems wrong to me and shouldn't dictate solutions:

the REST API runs in a front-end context where the site locale should apply

The REST API doesn't run in the front-end and doesn't run in the backend. The REST API is context-agnostic and should be able to be used properly anywhere.

And from this, I think a query argument to override any default behavior the REST API chooses make sense.

#3 @TimothyBlynJacobs
6 years ago

I mean front-end in the technical context. In which case I believe the REST API definitely runs in a front-end context since it uses WP Rewrite. But yes it definitely should be usable from any context in practice.

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


6 years ago

#5 @mnelson4
6 years ago

First off, adding a _locale parameter, which would default to site, but could also be set to user sounds useful.

Here's some additional food for thought though:

In slack @TimothyBlynJacobs brought up that there's a standard Accept-Language header that might apply: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Language.
So far we've really only used request headers for authentication, though, which is why he suggested this be a query parameter. (BTW, even if it's a query parameter, it could still be used for PUT and POST requests too).

The main downside to the Acccept-Language header is it's supposed to receive a language string, whereas it would be slick if we could just provide user as a value, meaning whatever language the user wanted.

I think being able to specify the user's selected language will be more useful than being able to specify ANY arbitrary language, but I can see folks wanting any arbitrary language too. (Eg, if you set the language inside a mobile app, then you'd want to see everything in that language- although in that case, it would probably be better to update the user's locale, because it's doubtful they'd want to use a different language on their mobile app than in the web interface).

IMO it would be good to use Accept-Language, but also permit the value user, to mean "use whatever language the user has set" (and not setting the Accept-Language header means use the site's language). But @kadamwhite said in slack that felt weird. Other thoughts on using the Accept-Language header? (And either sticking to the standard, or slightly tweaking the standard to also support user?)

#6 @TimothyBlynJacobs
6 years ago

I don't think Accept-Language is feasible. This header looks to be added automatically be a number of clients, most notably Google Chrome. For example: https://github.com/postmanlabs/postman-app-support/issues/3271.

#7 @mnelson4
6 years ago

Ah! Important insight, thanks @TimothyBlynJacobs. And that would be a problem because we'd like the default to be to the site's default language, whereas if API clients are automatically sending the Accept-Language header which specifies which language to use, then they'll default to seeing the site in their language instead of the site's language.

So in order to use the Accept-Language header in REST API requests, we should have been using it in normal web requests too. (Because then a French visitor would visit a site that's normally English, and instead see a French version, and when their browser makes requests to the REST API, the responses will also be in French; whereas if only the REST API uses Accept-Language, but normal web requests don't, then the page would load in English, but content loaded over the REST API would be in French).

Ok, so it seems if WordPress were to support Accept-Language header, it would be a separate initiative that would probably include using it in normal web requests too.

So, a _locale header with options of site or user would fit better into the WordPress way.

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


6 years ago

#9 @mnelson4
6 years ago

  • Keywords 2nd-opinion added

In today's meeting, @TimothyBlynJacobs and I discussed whether the check for the locale parameter would be best in serve_request() or dispatch().

PROS to serve_request():

  • if we check it before issuing error messages, those error messages could be translated
  • locale becomes a "global" lifecycle parameter, affecting all internal REST API requests (so even if you are going to make internal requests, the locale would be set once and reused, which is probably what we usually want)

PROs to dispatch():

  • internal requests can also set locale, which they'd probably expect
  • locale becomes a "local" lifecycle parameter, affecting only one internal REST API request (so it needs to be set on each request)

@TimothyBlynJacobs and I preferred to do it in serve_request(). He said

Global requests to /wp-json/ go through serve_request while internal requests only are dispatched. Historically, server level parameters ( prefixed with an underscore ) are only applied on the server level requests... So if we have another server parameter, at this point, I’m not sure where it is intended to be handled.

FWIW handling just in serve_request would be much simpler.

but

I worry about the inconsistency and I’m not sure where we stand there on the API design side of things

So, we'd like confirmation from component maintainer that setting the locale during serve_request is a good idea.

#10 @TimothyBlynJacobs
6 years ago

  • Keywords has-patch has-unit-tests dev-feedback added

Uploaded a patch based off what we discussed in the last REST API meeting: https://wordpress.slack.com/archives/C02RQC26G/p1535649512000100

This will switch the locale if the _locale query argument is set to user. The switching happens after auth and is restored after echoing the content. If it is set to site I've opted not to perform any switching whatsoever, the thinking being that if the locale has been switched at some point for the front-end context, then we probably shouldn't adjust it.

AFAICT, more testing welcomed, this works well for Core. However, there is a bit of an issue with CPTs. When the locale is switched, WordPress fires a change_locale action. Core hooks into this to re-register the built in post types and taxonomies which have their labels set statically during init. This was discussed during the initial implementation ticket.

For the current use cases of switch_to_locale() in core, this doesn't seem to be an issue too often. However, AFAIK, the function is mostly used for mail where post type labels probably aren't being fetched. The inclusion of Gutenberg is going to make this issue a lot more noticeable. We could say that users have to "fix" their code to listen for the change_locale hook combined with supporting the Post Type Labels ticket.

Alternately, we could try checking for the _locale wayyy earlier in the cycle. Which I believe is effectively in the _get_path_to_translation_from_lang_dir(), I'm really not sure, the is_admin() ? user : site pattern is all over the place. This would make the whole request occur in the user's language.

However, it seems like a quite hacky solution.

  • We'd now be loading the current user "early" not just in the admin and customizer, but the front-end as well.
  • We'd probably need to do checking ourselves if this is a REST API request because the WP global wouldn't be setup yet.
  • We'd need to worry about the rest_authentication_errors filter. If that filter returns an error, but a user has already been set on determine_current_user What localization should we use? See the earlier discussion in the ticket. ( This would happen in particular if a user is cookie authed, but has an invalid nonce ).

Side Note: I posted a question in Slack about changes to the test fixures, https://wordpress.slack.com/archives/C04KRK0KA/p1536119118000100. If anyone knows if just modifying the mo/po files directly is ok that'd be awesome!

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


6 years ago

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


6 years ago

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


6 years ago

#14 @mnelson4
6 years ago

🎸 rock on!

However, there is a bit of an issue with CPTs. When the locale is switched, WordPress fires a change_locale action. Core hooks into this to re-register the built in post types and taxonomies which have their labels set statically during init.

I'm not clear on why this is a problem. Is it just inefficient?

Alternately, we could try checking for the _locale wayyy earlier in the cycle.... We'd now be loading the current user "early" not just in the admin and customizer, but the front-end as well.

This sounds very likely to break plugins that expect the current user to be set at its current time, so should probably be avoided IMO. Although having the entire request be in that user's locale wouldn't be horrible, (that's probably want they assumed would happen).

#15 @TimothyBlynJacobs
6 years ago

The issue is that a custom post type will still have incorrect labels. For instance:

add_action( 'init', function() {
  register_post_type( 'my-cpt', [
    'label' => __( 'My CPTs' ),
  ] );
} );

That label will be translated on init and set to a static string in the post type object. When the locale is switched it will still be "My CPTs" in the site locale as opposed to the desired locale. Core works around this by hooking into change_locale and re-registering the post type, but most people don't listen for that action now. So their strings will be untranslated.

This would apply to any static strings setup, not just CPTs, though CPTs are the most obvious example.

Static as opposed to a function callback that returns the string.

This ticket was mentioned in Slack in #core-restapi by mnelson4. View the logs.


6 years ago

#17 @mnelson4
6 years ago

The issue is that a custom post type will still have incorrect labels

Ahhh ok that makes sense then.

This is probably tangent, but it might be helpful here if all the occurences of is_admin() ? get_user_locale() : get_locale() were replaced with a method something like

function determine_locale() {
  return apply_filters('determine_locale', is_admin() ? get_user_locale() : get_locale());
}

Then we'd have at least one filter we could use to change this behaviour, rather than dozens. Plus, repeating is_admin() ? get_user_locale() : get_locale() all over is essentially hard-coding that admin requests will always be in the user's locale, and all other requests will be in the site's locale (which we'd like to modify on this ticket)

#18 follow-up: @mnelson4
6 years ago

Summary of the issue

Calling switch_locale() during WP_REST_Server::serve_request() would change the site's locale much later than it normally is (an exception being when emailing users, locale switching happens very late in those cases). By that point, many strings (including CPT strings) were already translated and so will be in the wrong language.

So: I think REST API design says we'd prefer to switch the locale during WP_REST_Server::serve_request(), but all the rest of WP's code expects the locale to be set much earlier and not change (with a few exceptions). So what do we do?

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


6 years ago

#20 @swissspidy
6 years ago

How big of an issue are the untranslated post type labels for this? Happy to work on #38218 in the next few weeks. I had a PoC patch lying around somewhere a while ago...

Also, how much trouble would it cause to set the locale way earlier than now to circumvent this?

This is probably tangent, but it might be helpful here if all the occurences of is_admin() ? get_user_locale() : get_locale() were replaced with a method something like

Sounds interesting. This is already used quite a bit in WordPress, so I'm sure we could DRY things up a bit. Not sure if we really need yet another filter for this... Essentially you only need the locale filter and the user meta filter for the locale. I wouldn't want to change this.

#21 @TimothyBlynJacobs
6 years ago

How big of an issue are the untranslated post type labels for this? Happy to work on #38218 in the next few weeks. I had a PoC patch lying around somewhere a while ago...

Fairly important. Gutenberg uses the labels of post types and taxonomies to populate important parts of the editor interface. The current path works fine for the built-ins because they are re-registered, but if someone was using Gutenberg on a custom post type, or with custom taxonomies, they'd see this inconsistently translated interface.

Also, how much trouble would it cause to set the locale way earlier than now to circumvent this?

We had trouble identifying where, exactly, the translations for the request get set. load_*_textdomain() seemed like critical functions, as did _get_path_to_translation_from_lang_dir().

Sounds interesting. This is already used quite a bit in WordPress, so I'm sure we could DRY things up a bit. Not sure if we really need yet another filter for this... Essentially you only need the locale filter and the user meta filter for the locale. I wouldn't want to change this.

If we replaced those instances with a determine_locale() like function, it wouldn't necessarily need to be filtered. We could inline the REST API locale detection code.

Assuming that part is feasible, the difficulties would be two fold.

  1. We would need to do some trickery to determine whether this is a REST API request before parse_request.
  2. On rest requests, we would be loading the current user earlier than expected. Previously this [early loading](https://core.trac.wordpress.org/ticket/29783#comment:50) would only happen on admin requests and the customizer. What potential fallout this might cause, I'm not sure.

Something like this:

function determine_locale() {
	if ( ( is_multisite() && get_blog_option( null, 'permalink_structure' ) ) || get_option( 'permalink_structure' ) ) {
		// Not actually accurate, needs to handle index permalinks and paths in the home url, etc...
		$is_rest = strpos( $_SERVER['REQUEST_URI'], '/' . rest_get_url_prefix() ) === 0;
	} else {
		$is_rest = isset( $_GET['rest_route'] );
	}
	
	if ( is_admin() ) {
		return get_user_locale();
	}
	
	if ( $is_rest && isset( $_GET['_locale'] ) && 'user' === $_GET['_locale'] && is_user_logged_in() ) {
		return get_user_locale();
	}

	return get_locale();
}

I suppose this could actually happen with the existing locale filter. But we were confused whether that was the right solution or not. Particularly because a REST API locale seems very similar to how a different WP-Admin locale works, and that was implemented using the is_admin() ? get_user_locale() : get_locale() switch instead of inside get_locale() or a locale filter. History in #29783.

#22 in reply to: ↑ 18 @swissspidy
6 years ago

Summary of the issue

Calling switch_locale() during WP_REST_Server::serve_request() would change the site's locale much later than it normally is (an exception being when emailing users, locale switching happens very late in those cases). By that point, many strings (including CPT strings) were already translated and so will be in the wrong language.

So: I think REST API design says we'd prefer to switch the locale during WP_REST_Server::serve_request(), but all the rest of WP's code expects the locale to be set much earlier and not change (with a few exceptions). So what do we do?

Doesn't sound like an issue to me. If we fix the post type label ticket first, this is the way to go IMO. That's a perfect use case for switch_to_locale().

As mentioned yesterday, I'll take another stab at #38218.

#23 @mnelson4
6 years ago

Thanks for the direction, @swissspidy.

Doesn't sound like an issue to me.

So you're saying that, once you've committed #38218, we should be fine calling switch_to_locale() during WP_REST_Server::serve_request() which happens during the parse_request action, right? Is the reason for that because, besides CPT strings, there are very few strings translated before that?

#24 @swissspidy
6 years ago

@mnelson4 Yes, exactly. But now that I think about it, #41305 came back into my mind which should solve this issue for _any_ string. So that's probably the way to go

#25 @mnelson4
6 years ago

But now that I think about it, #41305 came back into my mind which should solve this issue for _any_ string. So that's probably the way to go

I agree, I'd love for that to be the solution. I'm not holding my breath though, as that'll be a significant change.

This ticket was mentioned in Slack in #core-restapi by mnelson4. View the logs.


6 years ago

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


6 years ago

#28 @kadamwhite
6 years ago

@schlessera has uploaded a new patch to #41305; if you can assist with testing that, we can use it as a basis for the rest of this work. Thank you Alain for prioritizing that patch refresh!

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


6 years ago

#30 @danielbachhuber
6 years ago

  • Focuses rest-api added
  • Milestone changed from Awaiting Review to 5.0

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


6 years ago

#32 @danielbachhuber
6 years ago

  • Owner set to pento
  • Status changed from new to assigned

@TimothyBlynJacobs I love how deviously simple 44758.2.diff is. As far as naming goes, I'd prefer get_current_locale() (similar to get_current_screen()), but mine is not a strongly held opinion.

@flixos90 @swissspidy @ocean90 would love your thoughts on 44758.2.diff if you have them.

#33 @danielbachhuber
6 years ago

Also, just so it's stated, Gutenberg will also need to be updated to use ?_locale= in API requests.

#34 @mnelson4
6 years ago

I took this for a spin and it works provided you remember to set the Accept header to application/json. I normally don't bother doing that, so it's a gotcha IMO. It would be nice if that weren't necessary, but I'm not sure if that's possible.

Also, I just realized the new method, regardless of its name, should probably have a wp_ prefix on it to avoid possible collisions, right?

#35 @swissspidy
6 years ago

I don't think a wp_ prefix is absolutely needed. Not many functions in WordPress have that, especially not the i18n functions. A quick search through the plugin repository revealed only a single plugin that defined a determine_locale() function. It's a closed one with no real installs: https://wordpress.org/plugins/cartrabbit/

As for the patch, it feels a bit odd to have that REST API specific code in such a low-level function. Perhaps the function could be made filterable instead and the REST API specific code is added via that filter?

Also, there is some similar code in wp-login.php where the wp_lang GET param is checked to switch the locale to one of the user's choice (similar to how https://login.wordpress.org does it). It currently uses switch_to_locale() to do so, but with a filterable determine_locale function that could be leveraged instead.

Of course these two parts don't necessarily have to be split up into their own functions and added via a filter, but a filter in general could still be beneficial.

Happy to iterate on the patch if these suggestions sounds reasonable.

#36 @pento
6 years ago

I like the idea of 44758.2.diff. @swissspidy, if you have time today to iterate on it, that'd cool. 🙂

@swissspidy
6 years ago

Account for wp-login.php, add filter

#37 follow-up: @swissspidy
6 years ago

I added support for locale switching on the login page in 44758.3.diff. I also added a filter that perhaps needs a better name.

Not sure if the wp_lang part needs some additional sanitization by checking if the locale is part of get_available_languages() (or equals en_US)

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


6 years ago

#39 @danielbachhuber
6 years ago

I like the looks of 44758.3.diff. We can land it after beta1 with some testing, and then someone could submit a pull request for https://github.com/WordPress/gutenberg/issues/10811

@flixos90
6 years ago

#40 @flixos90
6 years ago

I like what the current approach looks like, I just made two minor modifications in 44758.4.diff:

  • I changed the name of the filter to determine_locale. I think it's a more common approach to use the function name (with the verb) instead of what you're actually filtering. It confused me when I saw it was called determined_locale, different than the function and without any action verb in the name.
  • I think it would be beneficial to have a short-circuiting filter here, thus added pre_determine_locale. Some of the default logic has side effects (see #43869), so a short-circuit filter would allow working around these. I can personally see more use-cases for that than for the default one that runs after the default way of determining the current locale, however I think both have reasons to exist, so I'd say we should have both in.

It's good for merge in my opinion.

#41 @danielbachhuber
6 years ago

  • Keywords 2nd-opinion has-unit-tests dev-feedback removed

44758.5.diff catches one more instance of is_admin() ? get_user_locale() : get_locale() in wp_get_jed_locale_data().

#42 @danielbachhuber
6 years ago

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

In 43776:

REST API: Render response in user locale with ?_locale=user.

Introduces new determine_locale() function for deciding the proper locale to use for a response. Default value is get_user_locale() in the admin, and get_locale() on the frontend. Because REST API requests are considered frontend requests, ?_locale=user can be used to render the response in the user's locale.

Also updates wp-login.php?wp_lang implementation to benefit from this abstraction.

Props flixos90, mnelson4, swissspidy, TimothyBlynJacobs.
Fixes #44758.

#43 in reply to: ↑ 37 @danielbachhuber
6 years ago

  • Keywords fixed-5.0 added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to swissspidy:

Not sure if the wp_lang part needs some additional sanitization by checking if the locale is part of get_available_languages() (or equals en_US)

To close the loop on this, wp-login.php doesn't currently validate $_GET['wp_lang'] against a whitelist, so I don't think it's necessary to in this case. WP_Locale_Switcher->switch_to_locale() is a no-op if $locale isn't available in $this->available_languages.

Reopening for merge to trunk.

#44 @danielbachhuber
6 years ago

In 43846:

l10n: Avoid calling is_user_logged_in() in determine_locale().

is_user_logged_in() is a pluggable function, and loaded after plugins are loaded. If a plugin calls __() too early, is_user_logged_in() is missing and WordPress will fatal. get_user_locale() already handles this scenario for us, so it's safe to rely on exclusively.

See #44758.
Fixes #45235.

#45 @jeremyfelt
6 years ago

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

In 44134:

REST API: Render response in user locale with ?_locale=user.

Introduces new determine_locale() function for deciding the proper locale to use for a response. Default value is get_user_locale() in the admin, and get_locale() on the frontend. Because REST API requests are considered frontend requests, ?_locale=user can be used to render the response in the user's locale.

Also updates wp-login.php?wp_lang implementation to benefit from this abstraction.

Merges [43776] from the 5.0 branch to trunk.

Props flixos90, mnelson4, swissspidy, TimothyBlynJacobs.
Fixes #44758.

#46 @atimmer
6 years ago

In 44181:

l10n: Avoid calling is_user_logged_in() in determine_locale().

is_user_logged_in() is a pluggable function, and loaded after plugins are loaded. If a plugin calls () too early, is_user_logged_in() is missing and WordPress will fatal. get_user_locale() already handles this scenario for us, so it's safe to rely on exclusively.

Props danielbachhuber.
Merges [43846] to trunk.
See #44758.
Fixes #45235.

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.