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

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

[xyulex]

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

[ocean90]

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

[ocean90]

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

[westonruter]

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

test_wp_add_inline_script_before_concat_with_core_dependency() is failing

[ocean90]

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

[ocean90]

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.

[gitlost]

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]

Patch to make before_handle behave similar to after_handle.

[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()):

<?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]

Demo code to "batch" stuff.

[gitlost]

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

[gitlost]

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

[kirasong]

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

[ocean90]

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.

[gitlost]

@ocean90 wilco...

[jeremyfelt]

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.

[azaozz]

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

[jeremyfelt]

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

[gitlost]

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

[gitlost]

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.

[azaozz]

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.

[swissspidy]

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?

[jeremyfelt]

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.

[kirasong]

Replying to jeremyfelt:

I think we need a patch that does this.

Agreed. Can someone please put it together?

[azaozz]

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]

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

[swissspidy]

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]

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

[azaozz]

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. 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 more people will look, test, push it today.

[jeremyfelt]

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.

[jeremyfelt]

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.

[johnbillion]

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