Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 4 years ago

#36392 closed defect (bug) (fixed)

wp_add_inline_script() breaks script dependency order when using script loader (no SCRIPT_DEBUG)

Reported by: westonruter's profile westonruter Owned by: swissspidy's profile swissspidy
Milestone: 4.5 Priority: high
Severity: major Version: 4.6
Component: Script Loader Keywords: has-unit-tests has-patch commit
Focuses: Cc:

Description (last modified by westonruter)

I've found that if an inline script is added via wp_add_inline_script() for a JS file registered in a plugin, if this script has a dependency on a core script that gets concatenated via load-scripts.php, the script dependency and the inline script will erroneously get printed before the the script tag referencing load-scripts.php and the script it depends on. This error only occurs when SCRIPT_DEBUG is false, since this is when load-scripts.php is used.

Here is a minimal test case: https://gist.github.com/westonruter/c8968edc125fd31ae2da4171ced8c62b

wp_add_inline_script() introduced in #14853.

Attachments (15)

inline-script-without-dependency-breakage-when-script-debug.png (151.1 KB) - added by westonruter 9 years ago.
lack-of-inline-script-dependency-order-preserved.png (226.6 KB) - added by westonruter 9 years ago.
inline-script-causes-dependency-breakage.png (253.4 KB) - added by westonruter 9 years ago.
36392.patch (1.2 KB) - added by ocean90 9 years ago.
36392.2.patch (2.6 KB) - added by ocean90 9 years ago.
36392.3.patch (4.5 KB) - added by ocean90 9 years ago.
test_wp_add_inline_script_before_concat_with_core_dependency() is failing
36392.4.patch (10.4 KB) - added by gitlost 9 years ago.
Patch to make before_handle behave similar to after_handle.
36392.batch.patch (15.8 KB) - added by gitlost 9 years ago.
Demo code to "batch" stuff.
36392.batch.2.patch (21.8 KB) - added by gitlost 9 years ago.
A more convincing version of the "batch" demo, plus extra tests.
36392.5.patch (12.4 KB) - added by jeremyfelt 9 years ago.
36392.6.patch (12.0 KB) - added by gitlost 9 years ago.
Attempt to document the limitation with the 36392.4.patch as simply as possible.
36392.7.patch (12.7 KB) - added by azaozz 9 years ago.
36392.diff (12.5 KB) - added by swissspidy 9 years ago.
36392.2.diff (12.7 KB) - added by ocean90 9 years ago.
36392.3.diff (13.9 KB) - added by jeremyfelt 9 years ago.

Download all attachments as: .zip

Change History (55)

#1 @westonruter
9 years ago

  • Description modified (diff)

#2 @xyulex
9 years ago

I want to start contributing so I might be looking at this :)

@ocean90
9 years ago

#3 @ocean90
9 years ago

Attached a test in 36392.patch.

I had mentioned a similar case in ticket:14853:41 but seems like we missed the case with another dependency.

cc: @swissspidy

#4 @kirasong
9 years ago

  • Keywords has-patch added; needs-patch removed

#5 @westonruter
9 years ago

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

@ocean90
9 years ago

#6 @ocean90
9 years ago

  • Keywords has-patch added; needs-patch removed

@westonruter Did you miss to define a dependency in https://gist.github.com/westonruter/c8968edc125fd31ae2da4171ced8c62b#file-script-loader-test-case-php-L10?

36392.2.patch fixes the unit test but not sure if it fixes your issue too.

#7 @westonruter
9 years ago

@ocean90 you're absolutely right. I neglected to include the dependency when registering. Nevertheless, when adding the dependency the issue was incidentally still present. But, when I applied 36392.2.patch then the order gets fixed! :+1:

@ocean90
9 years ago

test_wp_add_inline_script_before_concat_with_core_dependency() is failing

#8 @ocean90
9 years ago

  • Keywords needs-patch added; has-patch removed

Added another test test_wp_add_inline_script_before_concat_with_core_dependency() which is also failing with my patch.

#9 @kirasong
9 years ago

  • Owner set to ocean90
  • Status changed from new to assigned

#10 @ocean90
9 years ago

  • Owner changed from ocean90 to swissspidy
  • Priority changed from normal to high
  • Severity changed from normal to major

Assigning to @swissspidy since he had a patch too. I'm out of ideas at the moment.

We should set a deadline (today's dev chat?) for this and maybe think about a revert for 4.5 too.

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


9 years ago

#12 @gitlost
9 years ago

The test test_wp_add_inline_script_before_concat_with_core_dependency() can be made to work if it's acceptable to not concatenate when there's before inline script, the same as when there's after inline script, and losing the $print_html_before stuff and just using $print_html.

@gitlost
9 years ago

Patch to make before_handle behave similar to after_handle.

#13 @gitlost
9 years ago

However!.... there is a pre-4.5 issue here with the use of $print_html in that dependencies will not be honoured for handles which use it (ie those using conditionals or inline scripts), for instance this test (using pre-4.5 wp_script_add_data()):

<?php
        /**
         * @ticket 36392
         */
        public function test_wp_script_conditional_with_concat_dependency() {
                global $wp_scripts;

                $wp_scripts->do_concat = true;
                $wp_scripts->default_dirs = array( '/directory/' );

                wp_enqueue_script( 'one', '/directory/one.js' );
                wp_enqueue_script( 'two', '/directory/two.js' );
                wp_enqueue_script( 'three', '/directory/three.js', array( 'one' ) );

                wp_script_add_data( 'one', 'conditional', 'blah' );

                wp_print_scripts();
                $print_scripts = get_echo( '_print_scripts' );

                $ver = get_bloginfo( 'version' );

                $expected  = "<!--[if blah]>\n";
                $expected .= "<script type='text/javascript' src='/directory/one.js?ver={$ver}'></script>\n";
                $expected .= "<![endif]-->\n";
                $expected .= "<script type='text/javascript' src='/wp-admin/load-scripts.php?c=0&amp;load%5B%5D=two,three&amp;ver={$ver}'></script>\n";

                $this->assertEquals( $expected, $print_scripts );
        }

will fail, as the conditional stuff will always be outputted afterwards.

I'll upload some demo code which puts the data into batches that seems to fix it.

Should I just open a new ticket for this?

@gitlost
9 years ago

Demo code to "batch" stuff.

@gitlost
9 years ago

A more convincing version of the "batch" demo, plus extra tests.

#14 @gitlost
9 years ago

Looking through the diffs in #14853, it seems that not concatenating when have inline code (before and/or after) was an artifact of the out-of-bandness of $print_html, rather than wanted behaviour, so I've uploaded a version of the "batch" demo which only bumps the batch when there's conditional data or it's not in the default directories (and thus can't be concatenated).

#15 @kirasong
9 years ago

@swissspidy @ocean90 @westonruter: Any thoughts on 36392.4.patch, which looks to be a first run at solving the issue?

#16 @ocean90
9 years ago

  • Keywords has-patch commit added; needs-patch removed

36392.4.patch seems the way to go. Enqueuing the script after the concatenated scripts is the current behavior anyway.

Replying to gitlost:

However!.... there is a pre-4.5 issue here with the use of $print_html in that dependencies will not be honoured for handles which use it (ie those using conditionals or inline scripts), for instance this test (using pre-4.5 wp_script_add_data()):
Should I just open a new ticket for this?

Yes please.

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


9 years ago

#18 @gitlost
9 years ago

@ocean90 wilco...

@jeremyfelt
9 years ago

#19 @jeremyfelt
9 years ago

  • Keywords commit removed

With 36392.4.patch applied and the below snippet added—load-scripts is printed first, a jQuery is not defined error appears, the inline script prints, and then jQuery is included.

add_action( 'admin_init', 'example_enqueue' );
function example_enqueue() {
	wp_add_inline_script( 'jquery-core', 'console.log( "howdy" );', 'before' );
}

This does not appear to be a problem in current trunk.

36392.5.patch adds 2 more (currently failing) tests for attaching inline scripts directly to core enqueued scripts rather than when core scripts are dependencies. I believe this is the expected output. See @ocean90's comment on #14853 for background.

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


9 years ago

#21 follow-up: @azaozz
9 years ago

As far as I understand this, we cannot guarantee outputting a script right before or right after an enqueued script (that has dependencies) if we concatenate scripts. Example:

  • Add "before" and "after" scripts on jquery-ui-mouse.
  • Enqueue jquery-ui-dialog which depends on jquery-ui-draggable which depends on jquery-ui-mouse.

All of these scripts are concatenated by default. How is concatenating supposed to work in this case? The only way I see is to "break" the concatenation and take them and all their dependencies out, which seems to be what we are trying to do now.

If the "before" and "after" scripts do not have to be right before and right after, this can be achieved in couple of different ways. $wp_scripts->localize() works that way, or scripts can be outputted directly on the same hook with lower or higher priority. Then they will be before or after all other scripts.

If we *must* have "right before" and right after" scripts, thinking the only "sane" way would be to disable concatenating when they are used.

Also, looking forward to the time we stop concatenating scripts and stylesheets because of HTTP/2 :)

#22 in reply to: ↑ 21 @jeremyfelt
9 years ago

Replying to azaozz:

If we *must* have "right before" and right after" scripts, thinking the only "sane" way would be to disable concatenating when they are used.

I think this make sense as a sane expectation.

Also, looking forward to the time we stop concatenating scripts and stylesheets because of HTTP/2 :)

Big +1 :)

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


9 years ago

@gitlost
9 years ago

Attempt to document the limitation with the 36392.4.patch as simply as possible.

#24 follow-up: @gitlost
9 years ago

The above is an attempt to document the limitation with the 36392.4.patch:

One can't add inline script to scripts in the $default_dirs (which in standard use are '/wp-admin/js/' and '/wp-includes/js/') if those scripts have dependencies on each other.

#25 in reply to: ↑ 24 @azaozz
9 years ago

Replying to gitlost:

One can't add inline script to scripts in the $default_dirs (which in standard use are '/wp-admin/js/' and '/wp-includes/js/') if those scripts have dependencies on each other.

Pretty much all default scripts are dependent on each other, so the limitation should probably be "The before and after inline scripts cannot be added to default scripts".

However I'm pretty sure someone will "not see" the inline docs or try it anyways. Perhaps we will also need a "Doing it wrong" when that is attempted.

#26 follow-up: @swissspidy
9 years ago

All of these scripts are concatenated by default. How is concatenating supposed to work in this case? The only way I see is to "break" the concatenation and take them and all their dependencies out, which seems to be what we are trying to do now.

That's correct. However, _print_scripts() is currently hardcoded to print all concatenated scripts first and the rest afterwards. One way to implement this is to add load-scripts.php?… via $wp_scripts->add(…) multiple times. That way you'd get as much concatenation as possible, but with dependencies in the right order.

If the "before" and "after" scripts do not have to be right before and right after, this can be achieved in couple of different ways. $wp_scripts->localize() works that way, or scripts can be outputted directly on the same hook with lower or higher priority. Then they will be before or after all other scripts.

If we go that route it'd probably very difficult to change behaviour in the future if needed. As a developer, I'd expect the order to be more accurate since you use wp_add_inline_script() per script handle, not universally as wp_add_inline_script( 'do_stuff()', 'before' );

If we *must* have "right before" and right after" scripts, thinking the only "sane" way would be to disable concatenating when they are used.
Also, looking forward to the time we stop concatenating scripts and stylesheets because of HTTP/2 :)

I explored many options today and everything eventually comes back to this. HTTP/2 is an argument as well.

However, are we confident in disabling concatenating in this case? Will this confuse developers expecting concatenated scripts all the time?

#27 in reply to: ↑ 26 ; follow-up: @jeremyfelt
9 years ago

Replying to swissspidy:

If we *must* have "right before" and right after" scripts, thinking the only "sane" way would be to disable concatenating when they are used.
Also, looking forward to the time we stop concatenating scripts and stylesheets because of HTTP/2 :)

I explored many options today and everything eventually comes back to this. HTTP/2 is an argument as well.

I think we need a patch that does this.

However, are we confident in disabling concatenating in this case? Will this confuse developers expecting concatenated scripts all the time?

If a developer uses wp_add_inline_script() on a core script that is normally concatenated, then they are asking for more granular control of those scripts.

Printing them individually is a sane way to maintain that control and is what we should be thinking about for HTTP/2. We don't have to worry about front-end scripts here and this is a new feature, so I think we can be confident with that route.

#28 in reply to: ↑ 27 @kirasong
9 years ago

Replying to jeremyfelt:

I think we need a patch that does this.

Agreed. Can someone please put it together?

#29 @kirasong
9 years ago

  • Keywords needs-patch added; has-patch removed

#30 @azaozz
9 years ago

One way to implement this is to add load-scripts.php?… via $wp_scripts->add(…) multiple times.

Right. However that will make calculating the dependencies pretty complex. We will probably need to be able to go back and re-calculate things when a previously enqueued dependency has to be moved to another "chain" of concatenated scripts.

I explored many options today and everything eventually comes back to this. HTTP/2 is an argument as well.

However, are we confident in disabling concatenating in this case? Will this confuse developers expecting concatenated scripts all the time?

On a "script heavy" wp-admin screens this will probably be noticeable. However don't think we have much choice. If we want granular control, concatenating will be in the way. Even if we implement multiple calls to load-scripts.php, chances are several scripts will have "before" or "after" and we will end up with 3, 4, 5... different concatenated "pieces", making it quite less effective. (Generally the concatenated scripts are different for each wp-admin screen, so they aren't cached in the browser very often. Single scripts are the same on every screen so caching happens more frequently.)

Printing them individually is a sane way to maintain that control and is what we should be thinking about for HTTP/2.

Agreed.

@azaozz
9 years ago

#31 @azaozz
9 years ago

Think I figured it out. To be able to control when scripts tags are printed we would need to append all tags (strings) to another var inside $wp_scripts including the tag(s) with the concatenated scripts.

Currently we are echoing these tags directly from $wp_scripts::do_item(), but "holding" the scripts that can be concatenated to the very end. That's why any script that has "before" or "after" is always printed above the concatenated scripts.

Making new var to hold all script tags until the end (instead of echoing) seems straightforward, but thinking even a small risk of introducing regressions it too much at this point.

In 36392.7.patch: when concatenating scripts if "before" or "after" is set, stop concatenating and output all already processed scripts right away. This basically turns off concatenating when a plugin sets "before" or "after" on a core script, however keeps the already concatenated scripts.

The tests need fixing since now we echo from do_item() (getting too late here, will look tomorrow). Of course, more eyes welcome :)

Last edited 9 years ago by azaozz (previous) (diff)

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


9 years ago

@swissspidy
9 years ago

#33 @swissspidy
9 years ago

  • Keywords has-patch added; needs-patch removed

In 36392.diff I iterated on the latest patch to fix the unit tests. Afterwards, I got rid of the extra disable_concat property and the tests still ran properly.

Feedback appreciated.

@ocean90
9 years ago

#34 @ocean90
9 years ago

@jeremyfelt Nice catch, thanks. Thought we had a test for this so I haven't checked that explicit.

Since nobody seems to be in favor of reverting the feature (What a surprise. ;)) 36392.diff looks like the only way to keep wp_add_inline_script() without a major refactoring to support adding scripts to core dependencies.

36392.2.diff is the same as 36392.diff but with more descriptive method names and two more scripts for the last test.

#35 @azaozz
9 years ago

36392.2.diff looks good here.

I got rid of the extra disable_concat property

Yep, do_concat can be reused for this. I mostly added another "toggle" as if it is local (at the top of WP_Scripts::do_item()) it will act as a "break" in concatenating. That would make it work as described above: concat scripts as usual, when there is a script with "before" or "after", output all already concatenated scripts, then output the current script with the extras, and after that continue concatenating. That was actually the initial patch, perhaps something to try next release :)

Since nobody seems to be in favor of reverting the feature...

I'm actually 51/49 for 36392.2.diff vs. reverting at the moment. That change may still bring regression(s) somewhere. Hoping today more people will look, test, push it, etc.

Last edited 9 years ago by azaozz (previous) (diff)

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


9 years ago

@jeremyfelt
9 years ago

#37 @jeremyfelt
9 years ago

  • Keywords commit added

36392.2.diff looks good to me as well. 36392.3.diff is the exact same except for an additional test that accounts for the case of mixed concat/non-concat for core scripts when scripts one and two are standard and the third script has an inline script before. This works as expected.

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


9 years ago

#39 @jeremyfelt
9 years ago

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

In 37171:

Ensure consistent dependency order when using wp_add_inline_script()

This disables the concatenation of remaining enqueued scripts once wp_add_inline_script() is invoked, which allows us to reliably print these scripts and their before/after inline scripts in the desired order.

Props gitlost, azaozz, swisspidy, ocean90.
Fixes #36392.

#40 @johnbillion
4 years ago

In 49601:

Build/Test Tools: Improve the reliability of the test that ensures correct dependency order when inline scripts are used.

This test already ensures WP_Scripts->do_concat is true, therefore it has no dependency on SCRIPT_DEBUG being false. This means the test can run in an environment where the .min suffix is not used.

This change allows for the test to pass in this situation.

See #36392, #51734, #51344

Note: See TracTickets for help on using tickets.