WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#35956 closed defect (bug) (fixed)

Script's dependencies are always moved to header

Reported by: stephenharris Owned by: ocean90
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.6
Component: Script Loader Keywords: has-patch
Focuses: Cc:
PR Number:

Description (last modified by ocean90)

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

Attachments (8)

35956_sort.diff (1.4 KB) - added by gitlost 4 years ago.
Demo sort of dependencies by group when recursing in all_deps().
35956_tests.diff (2.7 KB) - added by gitlost 4 years ago.
Some unit tests.
35956_sort2.diff (2.3 KB) - added by gitlost 4 years ago.
Update to save/restore $this->group.
35956_alldeps.patch (726 bytes) - added by pcfreak30 4 years ago.
35956_tests2.diff (2.9 KB) - added by gitlost 4 years ago.
Updated unit tests to check siblings.
35956_alldeps2.diff (2.6 KB) - added by stephenharris 4 years ago.
Ensure script's group is passed onto its dependencies. Reverts 36604
35956_tests3.diff (2.9 KB) - added by gitlost 4 years ago.
Version of unittests compatible with 35956_alldeps2.diff
35956.diff (5.1 KB) - added by stephenharris 4 years ago.
Improvements to earlier patch: Removed changes for styles as these were not relevant. Included tests and used $_group to $new_group for clarity.

Download all attachments as: .zip

Change History (19)

#1 @ocean90
4 years ago

#35957 was marked as a duplicate.

#2 @ocean90
4 years ago

  • Description modified (diff)
  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.5
  • Owner set to ocean90
  • Status changed from new to reviewing

#3 follow-up: @gitlost
4 years ago

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
4 years ago

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

@gitlost
4 years ago

Some unit tests.

#4 @gitlost
4 years ago

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
4 years ago

Update to save/restore $this->group.

@gitlost
4 years ago

Updated unit tests to check siblings.

#5 @pcfreak30
4 years ago

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.

#6 in reply to: ↑ 3 @stephenharris
4 years ago

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

Last edited 4 years ago by ocean90 (previous) (diff)

@stephenharris
4 years ago

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

#7 @stephenharris
4 years ago

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

#8 @gitlost
4 years ago

That looks the bee's knees.

@gitlost
4 years ago

Version of unittests compatible with 35956_alldeps2.diff

@stephenharris
4 years ago

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

#9 @stephenharris
4 years ago

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

#10 @ocean90
4 years ago

  • Status changed from reviewing to accepted

#11 @ocean90
4 years ago

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

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.

Note: See TracTickets for help on using tickets.