Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 4 months ago

#55628 closed defect (bug) (fixed)

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

Reported by: ndiego's profile ndiego Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.0 Priority: normal
Severity: normal Version: 6.0
Component: Script Loader Keywords: has-patch has-unit-tests dev-reviewed fixed-major
Focuses: javascript Cc:

Description

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.

Attachments (3)

console-errors.jpg (431.9 KB) - added by ndiego 3 years ago.
55628.revert-52937-for-testing-do-not-commit.diff (578 bytes) - added by costdev 3 years ago.
This patch is for testing. Do not commit.
55628.diff (547 bytes) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (43)

@ndiego
3 years ago

#1 @ndiego
3 years ago

  • Summary changed from Numerous console errors on Theme/Plugin File Editor screens to Numerous console errors on Theme/Plugin File Editor screens and syntax highlighting not working

#2 @ndiego
3 years ago

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

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


3 years ago

#4 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 6.0

Hi there, welcome back to WordPress Trac!

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

#5 @ndiego
3 years ago

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.

#6 @mukesh27
3 years ago

  • Keywords needs-patch added

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

#7 @costdev
3 years ago

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?

Last edited 3 years ago by costdev (previous) (diff)

#8 @costdev
3 years ago

Introduced in [52937].

@costdev
3 years ago

This patch is for testing. Do not commit.

#9 @costdev
3 years ago

  • Keywords needs-testing added
  • Version set to trunk

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. Run npm run build:dev.
  8. Navigate to Tools > Plugin File Editor. Notice the errors are gone.
Version 0, edited 3 years ago by costdev (next)

#10 @ocean90
3 years ago

  • Component changed from General to Script Loader

#11 @ndiego
3 years ago

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

#12 @jsnajdr
3 years ago

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.

#13 @jsnajdr
3 years ago

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.

#14 @jsnajdr
3 years ago

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.

#15 @jsnajdr
3 years ago

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.

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


3 years ago

#17 @chaion07
3 years ago

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!

#18 @costdev
3 years ago

@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();

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?

Last edited 3 years ago by costdev (previous) (diff)

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


3 years ago

#20 @hellofromTonya
3 years ago

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.

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


3 years ago

#22 @jsnajdr
3 years ago

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.

#23 @jsnajdr
3 years ago

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.

This ticket was mentioned in PR #2678 on WordPress/wordpress-develop by jsnajdr.


3 years ago
#24

  • Keywords has-patch has-unit-tests added; needs-patch removed

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

#25 @jsnajdr
3 years ago

  • Keywords needs-patch added; has-patch has-unit-tests removed

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.

#26 @jsnajdr
3 years ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-testing removed

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


3 years ago

#28 @peterwilsoncc
3 years ago

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.

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


3 years ago

#30 @Boniu91
3 years ago

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

#31 @ugyensupport
3 years ago

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

#32 @audrasjb
3 years ago

  • Keywords dev-reviewed added

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

#33 @peterwilsoncc
3 years ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

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.

#35 @peterwilsoncc
3 years ago

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

Reopening for merging to the 6.0 branch.

Second committer sign off has been given in comment #32

@SergeyBiryukov
3 years ago

#36 @SergeyBiryukov
3 years ago

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

#37 @peterwilsoncc
3 years ago

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

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.

#38 @peterwilsoncc
3 years ago

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.

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


3 years ago

#40 @sergiomdgomes
4 months ago

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.

Note: See TracTickets for help on using tickets.