WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 5 months ago

#51871 closed defect (bug) (fixed)

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

Reported by: oglekler Owned by: TimothyBlynJacobs
Milestone: 5.6 Priority: high
Severity: normal Version: 5.6
Component: Site Health Keywords: commit dev-reviewed
Focuses: rest-api Cc:

Description

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.

Attachments (1)

51871-sv_SE.png (28.5 KB) - added by kebbet 5 months ago.

Download all attachments as: .zip

Change History (26)

@kebbet
5 months ago

#1 @kebbet
5 months ago

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

#2 @TimothyBlynJacobs
5 months ago

  • Milestone changed from Awaiting Review to 5.6
  • Owner set to TimothyBlynJacobs
  • Status changed from new to assigned

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.

#3 @Clorith
5 months ago

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).

This ticket was mentioned in PR #773 on WordPress/wordpress-develop by TimothyBJacobs.


5 months ago

  • Keywords has-patch added

#5 @TimothyBlynJacobs
5 months ago

  • Keywords has-patch removed
  • Priority changed from normal to high

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.

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


5 months ago

#7 @ocean90
5 months ago

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

#8 @TimothyBlynJacobs
5 months ago

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.

#9 @ocean90
5 months ago

@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.

#10 @TimothyBlynJacobs
5 months ago

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?

#11 @TimothyBlynJacobs
5 months ago

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

#12 @ocean90
5 months ago

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.

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


5 months ago

#14 @prbot
5 months ago

TimothyBJacobs commented on PR #773:

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

#15 @SergeyBiryukov
5 months ago

  • Keywords 2nd-opinion added

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.

Last edited 5 months ago by SergeyBiryukov (previous) (diff)

#16 @Clorith
5 months ago

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.

#17 @TimothyBlynJacobs
5 months ago

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?

#18 follow-up: @TimothyBlynJacobs
5 months ago

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

#19 in reply to: ↑ 18 @SergeyBiryukov
5 months ago

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!

#20 @TimothyBlynJacobs
5 months ago

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

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.

#21 @TimothyBlynJacobs
5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to the 5.6 branch.

#22 @SergeyBiryukov
5 months ago

  • Keywords commit dev-reviewed added; 2nd-opinion removed

[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.

#23 follow-up: @TimothyBlynJacobs
5 months ago

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.

#24 in reply to: ↑ 23 @SergeyBiryukov
5 months ago

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.

#25 @SergeyBiryukov
5 months ago

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

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.

Note: See TracTickets for help on using tickets.