Script's dependencies are always moved to header

[stephenharris]

I found this after the patch ([36604]) for #35873 was applied. I was going to report it there, but digging a little deeper I think this is a pre-existing bug which has only been brought to light.

I'm seeing a that all dependencies of registered script are being moved to the header, even when the script that depends on them is loaded in the footer. It's late here, so I'm going to post this example and follow with some unit tests tomorrow.

Suppose we have:

wp_register_script('parent','parent.js', array('child'), '1', true);
wp_register_script('child', 'child.js', array(), '1', true);

Then we have the trace:

1. WP_Scripts->do_head_items()
2. WP_Dependencies->do_items(false,0)
3. WP_Scripts->all_deps( $handles ) (we loose all reference to the group here, defaults to false)

At this point we loop through the handles and call WP_Scripts->set_group( $handle, $recursion, $group ).

$group here is always false, at least when printing header scripts. Now lets assume we are at handle 'parent' in our loop. We then call WP_Scripts->all_deps (and so WP_Dependencies->all_deps), passing the dependencies as the first argument (e.g. array( 'child' )), the second argument ($recursion) as true (naturally), and the final argument false still.

That loops over the dependencies and we call

WP_Scripts->set_group( 'child', true, $group )`

As noted $group is always false (and cast to 0), so with the following, the 'child' script's group is always set to 0

if ( $recursion ) {
	$group = min( $this->group, $group );
}

My first thoughts are that at some point $group should really be set to the parent's group (e.g. 1 in this case).

[ocean90]

#35957 was marked as a duplicate.

[gitlost]

It's very confusing but the $group isn't always false for WP_Dependencies::set_group(), as it's set in WP_Scripts::set_group(). I think the problem is that the class variable $this->group in the WP_Dependencies::set_group() will be set to the first group in the dependencies, and if this happens to be zero then that's it.

Note for instance that if you change the ordering of the dependences in @stephenharris 's example so that child-footer is listed before child-head then it works as expected:

<?php
function my_enqueue_scripts() {
        wp_register_script( 'child-head', '/child-head.js', array(), null, false );
        wp_register_script( 'child-footer', '/child-footer.js', array(), null, true );
        wp_register_script( 'parent', '/parent.js', array( 'child-footer', 'child-head' ), null, true );
        wp_enqueue_script( 'parent' );
}

add_action( 'wp_enqueue_scripts', 'my_enqueue_scripts' );

Thus a way around this (though I'm sure there's better ways) would be to sort the dependencies (on recursion) from highest to lowest group when processing them in all_deps(). so that set_group() gets called in the appropriate order.

I'll upload a demo diff.

[gitlost]

Demo sort of dependencies by group when recursing in all_deps().

[gitlost]

Some unit tests.

[gitlost]

Looking at this again it's still going to fail if a dependency sets the $this->group to zero ahead of its (non-zero) sibling so I think saving and restoring the $this->group at each level is also needed. Perhaps. Maybe. I'll upload second versions...

[gitlost]

Update to save/restore $this->group.

[gitlost]

Updated unit tests to check siblings.

[pcfreak30]

I have noticed as well that despite all_deps taking a group argument, it is never used. Attached is a 1 line change to pass the group which normally would be 0 or 1.

[stephenharris]

Replying to gitlost:

It's very confusing but the $group isn't always false for WP_Dependencies::set_group(), as it's set in WP_Scripts::set_group().

Ah yes, of course. So I think the issue really was with [36604].

Your sorting patch does appear to be work, but I think I've got to the root of the problem which addresses this and the issue reported in #35873.

When processing dependencies $this->group will be the minimum of the script's registered group and all preceding siblings. This is wrong because only a scripts 'ancestors' in the dependency chain should affect where it is loaded. Effectively $this->group introduced a form of global state which potentially corrupted the group of dependencies. Sorting covers up this problem.

The real issue in #35873 was that script were not moving their dependencies to a lower group when necessary. E.g. if you reversed patch [36604], I found the following failed to load the child script:

wp_register_script( 'child-footer', '/child-footer.js', array(), null, true );
wp_register_script( 'parent-header', '/parent-header.js', array( 'child-footer' ), null, false );
wp_enqueue_script( 'parent-header' );

The 'grandfather' part of that ticket was irrelevant.

There are two reasons why dependencies aren't being appropriately moved. First of all in this line: ​https://github.com/WordPress/WordPress/blob/484d9ec6a6763690b4060a90af05cc107c941b64/wp-includes/class.wp-dependencies.php#L166 we pass $group: the purpose here is to ensure that the dependencies are loaded in that group or lower, but we are passing the wrong value. Our parent script could have moved with WP_Scripts::set_group.

Secondly WP_Dependencies::all_deps() always has $group evaluated to false because it is never given a third parameter by WP_Scripts::all_deps() (see ​https://github.com/WordPress/WordPress/blob/484d9ec6a6763690b4060a90af05cc107c941b64/wp-includes/class.wp-scripts.php#L359).

Patch to follow...

[stephenharris]

Ensure script's group is passed onto its dependencies. Reverts 36604

[stephenharris]

@gitlost That latest patch is failing Tests_Dependencies_Scripts::test_wp_register_script_with_dependencies_in_footer2 in your test patch 35956_tests2.diff, it switches the order between grandchild-head and child-head but there is no dependency between them, so that's OK.

[gitlost]

That looks the bee's knees.

[gitlost]

Version of unittests compatible with 35956_alldeps2.diff

[stephenharris]

Improvements to earlier patch: Removed changes for styles as these were not relevant. Included tests and used $_group to $new_group for clarity.

[ocean90]

In 36871:

Dependencies: Improve group processing of script dependencies.

This is a follow-up to [36604].

When processing dependencies $this->group will be the minimum of the script's registered group and all preceding siblings. This is wrong because only a scripts ancestors in the dependency chain should affect where it is loaded. Effectively $this->group introduced a form of global state which potentially corrupted the group of dependencies. Sorting covers up this problem.
The issue in #35873 was that script were not moving their dependencies to a lower group when necessary.

The fix:

• In WP_Dependencies::all_deps() pass the new $group value to WP_Dependencies::all_deps(). Previously the wrong value was passed because the parent script could have moved with WP_Scripts::set_group().
• In WP_Scripts::all_deps() pass the $group parameter to WP_Dependencies::all_deps() so it doesn't always use false for $group. Same for WP_Styles::all_deps().

Props stephenharris, gitlost.
Fixes #35956.