#55628 closed defect (bug) (fixed)
Numerous console errors on Theme/Plugin File Editor screens and syntax highlighting not working
Reported by: | ndiego | Owned by: | 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)
Change History (43)
#1
@
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
This ticket was mentioned in Slack in #core by ndiego. View the logs.
3 years ago
#4
@
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
@
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
@
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
@
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?
#9
@
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:
- Make sure you're on WP Core
trunk
. - Run
npm run build:dev
. - Add
define( 'SCRIPT_DEBUG', false );
towp-config.php
. - Install and activate Gutenberg.
- Navigate to
Tools > Plugin File Editor
. Notice errors in the console. - Apply 55628.revert-52937-for-testing-do-not-commit.diff.
- Run
npm run build:dev
. - Navigate to
Tools > Plugin File Editor
. Notice the errors are gone.
#11
@
3 years ago
@costdev no I haven't unfortunately. That said, I just tested your patch and it works as intended!
#12
@
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:
load-scripts.php
withjquery-core
,jquery-migrate
and others.- the
wp-i18n
script. - 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:
load-scripts.php
withjquery-core
etc and newly alsocommon
!- 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
@
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
@
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
@
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
@
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
@
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();
- 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?
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
3 years ago
#20
@
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
@
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
@
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
@
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.
This ticket was mentioned in Slack in #core by costdev. View the logs.
3 years ago
#28
@
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
@
3 years ago
Working fine for me too
Env
WP: Trunk
Gutenberg: 13.1.0
Language: Polish
Steps to test
#32
@
3 years ago
- Keywords dev-reviewed added
With the current patch, code highlighting displays well on my side as well 👍
#33
@
3 years ago
- Owner set to peterwilsoncc
- Resolution set to fixed
- Status changed from new to closed
In 53360:
peterwilsoncc commented on PR #2678:
3 years ago
#34
#35
@
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
#36
@
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.
This ticket was mentioned in Slack in #core by sergey. View the logs.
3 years ago
#40
@
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.
I just discovered, this issue is also impacting the JavaScript on the Add New User screen and prevents you from adding new users.