Numerous console errors on Theme/Plugin File Editor screens and syntax highlighting not working

[ndiego]

In 6.0 Beta 3, I am seeing a number of console errors on the Theme/Plugin File Editor screens and the syntax highlighting is no longer working.

To test, make sure you are using 6.0 Beta 3, then navigate to either the Theme File Editor or Plugin File Editor screens.

[ndiego]

I just discovered, this issue is also impacting the JavaScript on the Add New User screen and prevents you from adding new users.

[SergeyBiryukov]

Hi there, welcome back to WordPress Trac!

Thanks for the ticket, moving to the milestone for visibility.

[ndiego]

After some more testing, I have narrowed down the issue. If you have Beta 3 and the latest version of Gutenberg active, you should see the errors. If you have just Beta 3, you should see no errors. If you have 5.9.3 and the latest version of Gutenberg active, you should see no errors.

So this appears to be an interaction between Gutenberg and Beta 3. If anyone else has an opportunity to test and confirm, that would be most appreciated.

[mukesh27]

@ndiego I reproduce the same error.

Error report

Error: When I use WP 6.0-beta3 and the latest 13.1.0 version of the Gutenberg plugin.
No Error: When I use WP 6.0-beta3 only.
No Error: When I use WP 5.9.3 and the latest 13.1.0 version of the Gutenberg plugin.

[costdev]

This can also be triggered on WP Core trunk by setting define( 'SCRIPT_DEBUG', false ); in wp-config.php with Gutenberg 13.1.0, or Gutenberg trunk, installed.

I'm also getting this on 6.0-beta1 and 6.0-beta2, but not on 5.9.3.

@ndiego, have you been able to track down any more information on this issue?

[costdev]

Introduced in [52937].

[costdev]

This patch is for testing. Do not commit.

[costdev]

55628.revert-52937-for-testing-do-not-commit.diff just reverts one change from [52937].

Testing steps:

1. Make sure you're on WP Core trunk.
2. Run npm run build:dev.
3. Add define( 'SCRIPT_DEBUG', false ); to wp-config.php.
4. Install and activate Gutenberg.
5. Navigate to Tools > Plugin File Editor. Notice errors in the console.
6. Apply 55628.revert-52937-for-testing-do-not-commit.diff.
7. Navigate to Tools > Plugin File Editor. Notice the errors are gone.

[ocean90]

Related: ​#meta6195 / ​https://github.com/WordPress/gutenberg/pull/39497

[ndiego]

@costdev no I haven't unfortunately. That said, I just tested your patch and it works as intended!

[jsnajdr]

I'm able to reproduce this on the wp-admin/plugin-editor.php page, with WP 6.0 beta 3 and Gutenberg 13.1.0.

With Gutenberg plugin deactivated, the page loads scripts in the following order:

1. load-scripts.php with jquery-core, jquery-migrate and others.
2. the wp-i18n script.
3. the wp-common script.

The wp-common script depends on wp-i18n, which is loaded before it and things are fine.

With Gutenberg activated, it changes to:

1. load-scripts.php with jquery-core etc and newly also common!
2. the wp-i18n script.

The wp-common load has been moved into load-scripts.php, but wp-i18n wasn't, so wp-common loads first and fails.

I don't know at this moment how scripts are or are not consolidated into the load-script.php loader and how the incorrect load is triggered.

[jsnajdr]

Looking at this line where the common script is registered:

​https://github.com/jsnajdr/wordpress-develop/blob/trunk/src/wp-includes/script-loader.php#L687

It doesn't declare its dependency on the wp-i18n script, although it should. Adding the missing dependency should fix the issue.

Another instance of this issue seems to be wp-util.js which uses Lodash (_.memoize) but _ is undefined. It declares underscore as dependency, which is loaded inside load-scripts.php. Something goes wrong there.

[jsnajdr]

Another instance of this issue seems to be wp-util.js which uses Lodash

Update: this script seems to be fine. _ is undefined only because an earlier load of load-scripts.php?load=common,underscore failed when executing the common code, and never got to execute and initialize underscore. That's the only reason why a later _.memoize call fails.

The only bug is the missing wp-i18n dependency in common.

[jsnajdr]

I think there is a bug in WP_Scripts::do_item. For every script to be printed, it does a $this->in_default_dir() check and if the script is in the default dir and satisfies a few other conditions, it will be concatenated. On encountering a script that can't be concatenated, the so-far concatenated scripts will be printed and the processing proceeds without concatenation.

Things go wrong when a script in default dir (like common) depends on a script that's not in the default dir. Then the dependency won't be printed correctly. And that's exactly the case with our wp-i18n dependency. With Gutenberg disabled, it's in default dir and everything works. But when Gutenberg registers its own version of wp-i18n, it's no longer in default dir and the printing becomes incorrect.

[chaion07]

Thanks @ndiego for reporting this. We reviewed this ticket during a recent bug-scrub session for WP 6.0 and based on the feedback received, we're positive about an additional patch before RC1.The regression that appears to have highlighted a possible problem in Core. The next step would be to get a patch on the ticket for discussion/review.

Props to @costdev

Cheers!

[costdev]

@jsnajdr What are your thoughts on a possible solution/workaround to this?

Here are some that come to mind:

• We modify WP_Scripts::do_item() to allow the registration of custom versions of dependencies.
• We add a check specifically for Gutenberg.
  • e.g. This removes the errors.

// src/wp-includes/class.wp-scripts.php:338
$is_gutenberg_script = str_contains( $srce, plugins_url( 'gutenberg/build/' ) );

if ( $is_gutenberg_script || ( $this->in_default_dir( $srce ) && ( $before_handle || $after_handle || $translations ) ) ) {
        $this->do_concat = false;

        // Have to print the so-far concatenated scripts right away to maintain the right order.
        _print_scripts();
        $this->reset();

• We revert one of the changes in [52937] - this is what 55628.revert-52937-for-testing-do-not-commit.diff does.

I'm sure there are other options available to us, but I'm also mindful that we're going into RC. Would it be prudent, or not, to commit 55628.revert-52937-for-testing-do-not-commit.diff until such time as we resolve the investigation and solution to the WP_Scripts::do_item() issue?

[hellofromTonya]

6.0 RC1 is happening within a few minutes. The issue reported here is a regression introduced in the 6.0 cycle. Unfortunately, there's not consensus yet on how to resolve it. But there is consensus to let this ticket go past RC1 point in the hopes that a resolution can be found in time for RC2.

[jsnajdr]

Copying a Slack message (​https://wordpress.slack.com/archives/C035NHLFUSY/p1651568132257809?thread_ts=1651562448.062809&cid=C035NHLFUSY):

I think reverting the translations patch is the best resolution right now. It’s merely covering up a real bug in WP_Scripts instead of fixing it, but given how fundamental WP component this is, and how late in the release cycle we are, doing a real and reliable fix right now is too risky.

I'm working on a proper fix, but for now, reverting to how things worked in 5.9.x would be most safe.

[jsnajdr]

I have a draft patch here: ​https://github.com/WordPress/wordpress-develop/pull/2667

I'll add some comments there about what it does change and how and what are the pros and cons.

Fixes https://core.trac.wordpress.org/ticket/55628 by avoiding an unwanted side effect of changes done in https://core.trac.wordpress.org/ticket/55250

That patch stopped printing translations inline scripts when these translations were empty, removing some bloat from the generated HTML. But that change also had another effect that turned out to be unwanted. When the print_translations method returns true for a script, the script won't be concatenated with load-scripts.php. But now print_translations started returning false, starting to concatenate scripts like common, which, during registration in wp_default_scripts, are marked as "having translations," which means "using wp-18n and translations from the default domain," without actually providing any translations:
{{{php
$scripts->add( 'common', "/wp-admin/js/common$suffix.js", array( 'jquery', 'hoverIntent', 'utils' ), false, 1 );
$scripts->set_translations( 'common' );
}}}
Then, when common got concatenated and wp-i18n, having been overridden by Gutenberg plugin, wasn't concatenated (because scripts from plugins are not), they were loaded in reversed order and the common script crashed with a reference error accessing wp.i18n.__.

The solution is to reinstate the concatenation opt-out condition with checking if $obj->textdomain is set. That way, we can eat our cake (avoid outputting redundant scripts) and keep it too (prevent concatenation).

[jsnajdr]

So, I have two solutions for this issue:

The first one is a very simple one, perfectly suitable to be safely shipped in 6.0 RC: ​https://github.com/WordPress/wordpress-develop/pull/2678. It's an amendment to [52937], re-introducing the concatenation opt-out condition.

The second one attempts to really fix a bug in WP_Scripts, where a script could get loaded before its own dependencies, leading to errors: ​https://github.com/WordPress/wordpress-develop/pull/2667. Unlike the simple fix, which works only for scripts depending on wp-i18n, this one works for all scripts. But it's a work-in-progress at this moment, not suitable for 6.0.

[peterwilsoncc]

Testing ​PR #2678 and the fix looks suitable to me. I've put a couple of notes on the pull request but they're both in the tests file.

Testing notes

Environment

WP: Trunk
Gutenberg: 13.1.0
Language: US English

Reproduction steps

Per comment #9 above.

Note: On a French install this didn't work, it must be US English.

With patch applied

• Unable to reproduce
• Code highlighting was displayed as expected.
• The patch does increase the number of JS files by four but that reverts to the 5.9 baseline for the number of JS files.

[Boniu91]

Working fine for me too

Env

WP: Trunk
Gutenberg: 13.1.0
Language: Polish

Steps to test

https://core.trac.wordpress.org/ticket/55628#comment:9

[ugyensupport]

I would also like to confirm on my end. It's looking good.

[audrasjb]

With the current patch, code highlighting displays well on my side as well 👍

[peterwilsoncc]

In 53360:

Script Loader: Fix i18n edge case breaking dependencies.

Prevent concatenation of scripts if the text domain is defined to ensure the dependency order is respected.

This accounts for an edge case in which replacing a core script via a plugin and a lack of translations (eg, for a US English site) could cause the JavaScript files to be ordered incorrectly.

Follow up to [52937].

Props audrasjb, boniu91, chaion07, costdev, hellofromtonya, jsnajdr, mukesh27, ndiego, ugyensupport.
Fixes #55628.

Merged in https://core.trac.wordpress.org/changeset/53360

[peterwilsoncc]

Reopening for merging to the 6.0 branch.

Second committer sign off has been given in comment #32

[SergeyBiryukov]

$translations_stop_concat = ! empty( $obj->textdomain );

I think this line could use a comment, as the relation between concatenation and text domain is not quite clear to me at a glance. Maybe something like 55628.diff​?

Otherwise, [53360] looks good to backport.

[peterwilsoncc]

In 53366:

Script Loader: Explain why i18n prevents concatenation.

Add inline comment to WP_Scripts::do_item() explaining why the definition of a text domain prevents concatenation.

Follow up to [53360].

Fixes #55628.
Props SergeyBiryukov.

[peterwilsoncc]

In 53367:

Script Loader: Fix i18n edge case breaking dependencies.

Prevent concatenation of scripts if the text domain is defined to ensure the dependency order is respected.

This accounts for an edge case in which replacing a core script via a plugin and a lack of translations (eg, for a US English site) could cause the JavaScript files to be ordered incorrectly.

Follow up to [52937].

Props audrasjb, boniu91, chaion07, costdev, hellofromtonya, jsnajdr, mukesh27, ndiego, ugyensupport, SergeyBiryukov.
Merges [53360,53366] to the 6.0 branch.
Fixes #55628.

[sergiomdgomes]

Hey folks!

I recently came across this issue when investigating why wp-i18n was being loaded in the <head> on a site. The cause seems to be the workaround for this bug (and #6195) that's ​still in place in Gutenberg.

wp-i18n isn't too large a script, but it does depend on the considerably larger wp-polyfill, which as a result also gets enqueued in <head>. From a performance point of view, it would be a nice win if we managed to fix this in a different way, since scripts in the <head> are render-blocking and can thus have an impact on first paint and all subsequent paint metrics.

I filed ​a bug in the Gutenberg repo to track this as well.