WordPress.org

Make WordPress Core

Opened 5 weeks ago

Closed 3 weeks ago

Last modified 10 days ago

#50999 closed defect (bug) (fixed)

Scripts loaded through concatenation don't print their translations.

Reported by: herregroen Owned by: ocean90
Milestone: 5.5.1 Priority: high
Severity: major Version: 5.0
Component: I18N Keywords: has-patch needs-testing needs-unit-tests commit
Focuses: accessibility, javascript Cc:

Description

If a script is loaded through load-scripts.php then it's translations aren't loaded as the translation logic is only called after the handle is added to the $concat variable has been appended and function returns.

To fix this the same exception should be made for translations as has been made for handles, namely that these are never included in load-scripts.php.

Attachments (2)

translations-no-concat.patch (1.5 KB) - added by herregroen 5 weeks ago.
50999.diff (1.8 KB) - added by desrosj 3 weeks ago.

Download all attachments as: .zip

Change History (32)

#1 @SergeyBiryukov
5 weeks ago

  • Milestone changed from Awaiting Review to 5.5.1

#2 @SergeyBiryukov
5 weeks ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#3 follow-up: @mikeyarce
5 weeks ago

Thanks for the report @herregroen !

Does your patch remove all the concatenation for all scripts though?

I wanted to note that a side-effect of the concatenation change in 5.5 is that now that media-editor.js is being concatenated (looks like it wasn't in 5.4 by default?), it's causing some issues when using another concat plugin in conjunction like: https://github.com/Automattic/nginx-http-concat/

The patch in this ticket does fix our problems though because it removes things like media-editor.js from being concatenated.

I'm curious about the reasoning to start concatenating media-editor.js and other scripts like that and if we're seeing other problems like the one I mentioned.

#4 @johnbillion
5 weeks ago

  • Version changed from trunk to 5.5

This ticket was mentioned in Slack in #core-editor by tacoverdo. View the logs.


5 weeks ago

This ticket was mentioned in Slack in #core-editor by bobbingwide. View the logs.


5 weeks ago

#7 follow-up: @bobbingwide
5 weeks ago

This patch also fixes #50993.

#8 @desrosj
5 weeks ago

#50993 was marked as a duplicate.

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


4 weeks ago

#10 in reply to: ↑ 3 @sageshilling
4 weeks ago

Last edited 4 weeks ago by sageshilling (previous) (diff)

#11 @audrasjb
4 weeks ago

  • Severity changed from normal to major

Hi,
I tested the proposed patch and it fixes the issue on all place I checked. Media modal, auto-updates screens, customizer, add new plugin screen…

In y opinion this is a major issue and I'm in favor of a quick 5.5.1 release because of that one.

#12 @audrasjb
4 weeks ago

  • Focuses accessibility added
  • Keywords has-patch added

Adding has-patch workflow keyword and accessibility focus as this is an accessibility issue as well. Indeed, those unexpected language changes can be considered as an accessibility issue.

#13 @audrasjb
4 weeks ago

  • Priority changed from normal to high

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


4 weeks ago

#15 in reply to: ↑ 7 @bobbingwide
4 weeks ago

Replying to bobbingwide:

This patch also fixes #50993.

Gutenberg 8.8.0 also resolves the issue reported in #50993.

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


4 weeks ago

This ticket was mentioned in Slack in #forums by otto42. View the logs.


4 weeks ago

#18 @desrosj
3 weeks ago

I'm still trying to determine exactly what caused this regression. The code being changed in translations-no-concat.patch has been pretty much untouched since it was merged into core in 5.0, so a change elsewhere is most likely the culprit.

I am currently trying to confirm a suspicion that I have that the changes associated with #20491 made in 5.5 are the reason this is happening.

When a script is localized to a handle that is concatenated, the localized script is printed to the page through $print_code in the WP_Scripts class. Because they are printed out in the page source, the script can still be loaded through load-scripts.php.

Switching the JavaScript strings to use wp.i18n would cause these translations to no longer be printed to the page through WP_Scripts->print_extra_scripts() because the new method of JS string localization manually prints out the translations after the script is concatenated.

If this thinking is correct, the patch I am attaching should allow scripts with JS translations to continue to be concatenated without the issue. Some testing/sanity checking would be much appreciated.

@desrosj
3 weeks ago

#19 @desrosj
3 weeks ago

Also, just for completeness, these are some changesets I believe may be causing this issue: [47884,48266,48270,48274,48285,48340,47347-47348,48375,48383-48385,48391-48392,48394-48396].

Last edited 3 weeks ago by desrosj (previous) (diff)

#20 @desrosj
3 weeks ago

Related: #51123.

I don't think a commonL10n back-compat shim would fix this specific issue, but definitely related.

Last edited 3 weeks ago by desrosj (previous) (diff)

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


3 weeks ago

#22 @JeffPaul
3 weeks ago

  • Keywords needs-testing added

#23 follow-up: @johnbillion
3 weeks ago

I'm struggling to reproduce this before I look into the cause.

Can someone provide steps to reliably reproduce? Maybe @herregroen ? Cheers!

#24 @ocean90
3 weeks ago

  • Keywords needs-unit-tests added
  • Owner changed from SergeyBiryukov to ocean90

#25 in reply to: ↑ 23 ; follow-ups: @ocean90
3 weeks ago

Replying to johnbillion:

Can someone provide steps to reliably reproduce? Maybe @herregroen ? Cheers!

Inspecting the source I see this without the plugin and this with the plugin activated.

50999.diff doesn't work for me, it seems to print raw JS on the page due to missing <script> tags. translations-no-concat.patch looks better.

Last edited 3 weeks ago by ocean90 (previous) (diff)

#26 in reply to: ↑ 25 @ocean90
3 weeks ago

Replying to ocean90:

Inspecting the source I see this without the plugin and this with the plugin activated.

The difference is caused by the wp-polyfill script which prevents concatenation due to the inline scripts. In core, it's always enqueued in the footer, thus no concatenation is enabled for most of the scripts. With Yoast SEO activated, wp-polyfill gets already loaded in the header and disables concatenation only for the header scripts, meaning concatenation is enabled for all footer scripts, making the issue visible.

#27 in reply to: ↑ 25 @ocean90
3 weeks ago

  • Keywords commit added
  • Version changed from 5.5 to 5.0

Replying to ocean90:

50999.diff doesn't work for me, it seems to print raw JS on the page due to missing <script> tags. translations-no-concat.patch looks better.

For 50999.diff you have to keep the part where $translations gets wrapped with the <script> tags at the current position because $print_code gets printed by _print_scripts() which already adds the <script> tags. But this still doesn't work as expected because the translation script requires wp.i18n to be defined which is not the case.

tldr; translations-no-concat.patch should be the way to go here. It's also kind of covered by the Tests_Dependencies_Scripts::test_wp_add_inline_script_before_after_concat_with_core_dependency() test.

We might want to explore in a separate ticket to re-enable concatenation after a script has been printed which previously prevented concatenation.

#28 @SergeyBiryukov
3 weeks ago

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

In 48897:

Script Loader: Disable concatenation for scripts with translations to ensure they are printed in the right order.

Props herregroen, ocean90, desrosj, mikeyarce, bobbingwide, audrasjb, johnbillion.
Fixes #50999.

#29 @SergeyBiryukov
3 weeks ago

In 48898:

Script Loader: Disable concatenation for scripts with translations to ensure they are printed in the right order.

Props herregroen, ocean90, desrosj, mikeyarce, bobbingwide, audrasjb, johnbillion.
Merges [48897] to the 5.5 branch.
Fixes #50999.

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


10 days ago

Note: See TracTickets for help on using tickets.