Site Health actions that use the REST API receive an untranslated response

[oglekler]

All Site Health checks which use REST API are getting reply without translation.
Screenshot from a local site with the Russian language chosen like a site language and language for the current user:
​https://yadi.sk/i/Td2OwFh2YH2zZw
Russian version 5.6 translated 100% and strings are present in the PO file.

[kebbet]

Testing with 5.6-RC1-49687 and sv_SE. All strings translated in the translation-repo, but one response shows up in English.

[TimothyBlynJacobs]

Great catch @oglekler! This is because we aren't passing _locale=user to the REST API as a query parameter. The apiFetch package automatically applies this as a middleware, but the simpler apiRequest package does not.

Patch coming up.

[Clorith]

I don't think that's the issue here, a quick dump of the locale inside the function shows the correct one is recognized, even when using apiRequest (if you change the site language for example, not just profile language), it still returns English text even if get_locale() shows nb_NO in my test case.

It's the same behavior for both apiRequest and apiFetch (that said, it should certainly use apiFetch instead, to honor the users profile option at least).

Trac ticket: https://core.trac.wordpress.org/ticket/51871

[TimothyBlynJacobs]

Yeah I misread the ticket the first time. We still need to fix that issue, and I've attached a patch that will handle that part.

I believe this specific issue is because the strings are defined in the WP_Site_Health class which is in wp-admin so part of the admin language pack. But the REST API executes in a front-end context, so those admin translations aren't loaded by load_default_textdomain. I'm not sure what should be done about that.

Cc: @ocean90, @swissspidy, @SergeyBiryukov.

[ocean90]

Looks like we have two options (maybe only one):

• Move the class to wp-includes if it's used in the front end (which includes the REST API). I'm not sure why this file is actually in wp-admin since it's already loaded by wp-settings.php.
• Try to adjust the file mappings in ​https://meta.trac.wordpress.org/browser/sites/trunk/wordpress.org/public_html/wp-content/plugins/wporg-gp-customizations/inc/cli/class-make-core-pot.php?rev=10063#L86

[TimothyBlynJacobs]

Thanks @ocean90. I added moving the classes to the PR. The issue still doesn't appear fixed because the strings are still in the different package.

[ocean90]

@TimothyBlynJacobs That sounds correct because this requires an update to the ​POT files and an import of those on translate.wordpress.org. This all done automatically once the changes have been committed to core. Once imported, new generated language packs will include the strings of the moved class in the front-end PO/MO files.

[TimothyBlynJacobs]

Gotcha @ocean90. Are we good to commit? Is there something I need to do during commit to keep the history of the site health files?

[TimothyBlynJacobs]

Also noting @adamsilverstein is going to review the JS changes.

[ocean90]

I'm currently not able to test it but from reading the changes it seems to be good.

To keep the history you have to use svn copy for the files first. Then you can do the changes and commit everything at once. Note that a patch doesn't show this so you have to do this locally on commit.

Thanks for the review @adamsilverstein! Pushed a commit to address that issue.

[SergeyBiryukov]

For reference, this appears to be a result of moving async tests to REST API endpoints in [49154] / #48105.

I can confirm the PR works as expected and the strings are displayed in Russian after I manually copy them from the wp-content/languages/admin-ru_RU.po file to ru_RU.po and rebuild the ru_RU.mo file with Poedit. As noted above, after the change is committed to core, the strings will be moved automatically.

I do, however, have a couple of other concerns:

• With this change, the front-end package will include ~250 more strings that are primarily used in the admin. This will increase memory footprint and might have performance implications similar to those previously raised in #19852. Granted, 250 is only 6% of all current front-end strings (4149), but still a non-trivial number. If there was a way to only move the async test strings we need and leave the rest in the admin, I think that would be better.
• More importantly, I'm concerned with moving ~250 strings from the admin language project to the front-end while being in string freeze. I don't think translate.wordpress.org will move the translations automatically as well, so translators will have to manually copy and approve each string. Granted, the strings should already be in translation memory, but copying, checking, and approving them is still a lot of work.

As an alternative approach, could we load the admin language file on these requests instead, similar to how it's done in load_default_textdomain()?

load_textdomain( 'default', WP_LANG_DIR . "/admin-$locale.mo" );

That seems to also work as expected in my testing.

[Clorith]

I'm also leaning towards the suggestion from @SergeyBiryukov here (and is honestly what I thought would have happened behind the scenes, I was not aware the wp-admin had a separate translations file).

The ideal scenario moving forward is to allow all Site Health checks to be retrieved through the REST API endpoint, not just the asynchronous ones (so that they can be used in more flexible ways by for example a host), so finding the best solution here that doesn't involve overburdening the front-end is optimal.

[TimothyBlynJacobs]

That's a good point. We'd be loading unnecessary strings for that site health request, but that probably isn't a big deal.

I wonder if for 5.7 we should look at making a REST API strings package?

[TimothyBlynJacobs]

I've updated the patch, loading the text domain manually also works in my testing, could you give it a second look @Clorith @SergeyBiryukov?

[SergeyBiryukov]

Replying to TimothyBlynJacobs:

I've updated the patch, loading the text domain manually also works in my testing, could you give it a second look?

Looks good to me and works as expected :) Thanks!

[TimothyBlynJacobs]

In 49716:

Site Health, App Passwords: Ensure REST API responses are properly translated.

The REST API requests in Site Health and App Passwords now include _locale=user in the request URL to ensure the user's locale is used instead of the site locale. Additionally, the apiRequest library now sends a JSON Accept header which is required by determine_locale() to respect the _locale query parameter.

The Site Health REST API controllers now manually load the default admin textdomain if not is_admin(). This allows for the Site Health tests to be translated even though the translations are part of the administration project and the REST API is not.

Props oglekler, kebbet, Clorith, TimothyBlynJacobs, ocean90, SergeyBiryukov, adamsilverstein.
Fixes #51871.

[TimothyBlynJacobs]

Reopening for backport to the 5.6 branch.

[SergeyBiryukov]

[49716] looks good to me to backport.

It might also be a good idea to only call load_textdomain() once (for example, having a flag in a static variable) and not several times in a row. That could be addressed in a follow-up commit, or could be a future enhancement.

[TimothyBlynJacobs]

Ah yeah, I think that'd be a good idea too. Each REST API request is separate, so we don't necessarily need to fix this for 5.6 if we don't want to.

[SergeyBiryukov]

Replying to TimothyBlynJacobs:

Each REST API request is separate, so we don't necessarily need to fix this for 5.6 if we don't want to.

Ah, right, I missed that. Should be good to go as is then, I don't think we have a way to save the state between requests without involving transients, etc.

[SergeyBiryukov]

In 49724:

Site Health, App Passwords: Ensure REST API responses are properly translated.

The REST API requests in Site Health and App Passwords now include _locale=user in the request URL to ensure the user's locale is used instead of the site locale. Additionally, the apiRequest library now sends a JSON Accept header which is required by determine_locale() to respect the _locale query parameter.

The Site Health REST API controllers now manually load the default admin textdomain if not is_admin(). This allows for the Site Health tests to be translated even though the translations are part of the administration project and the REST API is not.

Props oglekler, kebbet, Clorith, TimothyBlynJacobs, ocean90, SergeyBiryukov, adamsilverstein.
Reviewed by TimothyBlynJacobs, SergeyBiryukov.
Merges [49716] to the 5.6 branch.
Fixes #51871.