Make WordPress Core

Opened 11 years ago

Closed 8 years ago

Last modified 8 years ago

#25247 closed defect (bug) (fixed)

WP_Dependencies unsets a to do item when it isn't loaded

Reported by: markoheijnen's profile markoheijnen Owned by: chriscct7's profile chriscct7
Milestone: 4.5 Priority: normal
Severity: normal Version: 3.6
Component: Script Loader Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Currently it unsets $this->to_do no matter if do_item() return true or false. For example If you move jquery to the footer it will not load jquery at all. since it thinks jquery-core is loaded.

add_action( 'wp_default_scripts', 'move_jquery_to_footer_and_not_load_migrate' );
public function move_jquery_to_footer_and_not_load_migrate( &$scripts ) {
	if( ! is_admin() ) {
		$scripts->registered['jquery']->deps  = array( 'jquery-core' );
		$scripts->add_data( 'jquery', 'group', 1 );
		$scripts->add_data( 'jquery-core', 'group', 1 );
	}
}

Attachments (17)

#25247_bugfix.diff (984 bytes) - added by nmarks 10 years ago.
Patch for #25247
#25247_bugfix_v2.diff (1.1 KB) - added by nmarks 9 years ago.
Revised patch for #25247
25247-unit-test.diff (1.1 KB) - added by nmarks 9 years ago.
Unit test for ticket #25247
25247_dependencies.diff (1.4 KB) - added by gitlost 9 years ago.
Proposed patch.
25247_tests.diff (3.4 KB) - added by gitlost 9 years ago.
Unit tests for patch.
25247_scripts.diff (2.2 KB) - added by gitlost 9 years ago.
Optional cleanup patch to WP_Scripts.
25247_tests2.diff (3.4 KB) - added by gitlost 9 years ago.
Refresh on unit tests to patch to latest version.
25247_scripts2.diff (2.2 KB) - added by gitlost 9 years ago.
Refresh for 4.4.
25247_tests3.diff (3.5 KB) - added by gitlost 9 years ago.
Refresh for 4.4.
25247_scripts3.diff (2.3 KB) - added by gitlost 8 years ago.
Refresh for 4.5.
25247_scripts4.diff (2.3 KB) - added by gitlost 8 years ago.
Updated @since to 4.5 on overridden add().
25247_dependencies2.diff (1014 bytes) - added by gitlost 8 years ago.
Updated in light of [36550] (#35643).
25247_tests5.diff (4.9 KB) - added by gitlost 8 years ago.
Updated in light of [36550] (#35643) and #35873.
25247_scripts5.diff (2.3 KB) - added by gitlost 8 years ago.
Refreshed to run against trunk with no "hunk succeeded" messages.
25247_tests6.diff (4.6 KB) - added by gitlost 8 years ago.
Fixed outputExpectedRegex stuff.
25247.diff (7.9 KB) - added by gitlost 8 years ago.
Combined all diffs per @ocean90 request.
25247_to_do_deps.patch (654 bytes) - added by pcfreak30 8 years ago.

Download all attachments as: .zip

Change History (51)

#1 follow-up: @nacin
11 years ago

  • Keywords needs-unit-tests added

Could you submit a unit test patch demonstrating the issue?

#2 @ocean90
10 years ago

  • Cc ocean90 added

#3 follow-up: @ocean90
10 years ago

  • Cc azaozz added
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Version set to 3.6

This script has worked until 3.6:

/**
 * Prints jQuery in footer on front-end.
 */
function ds_print_jquery_in_footer( &$scripts) {
	if ( ! is_admin() )
		$scripts->add_data( 'jquery', 'group', 1 );
}
add_action( 'wp_default_scripts', 'ds_print_jquery_in_footer' );

With 3.6 I would set the group arg for the dependencies to 1 too, but that doesn't work.

#4 @nacin
10 years ago

  • Component changed from General to Script Loader

@nmarks
10 years ago

Patch for #25247

#5 @nmarks
10 years ago

  • Keywords has-patch added; needs-patch removed

I added a patch for this. I found that the function was adding jquery to the 'done' array as it doesn't have an src set (src is set on jquery-core).

What's currently happening is when it is doing the footer items, jquery is already in the done array and it makes no attempt to load it.

I've added a patch that checks the current group and set group (if isset) on the item when adding to the done array in the !(has-src) conditional.

For this to work, just set the jQuery and its dependencies' group to 1 with the wp_default_scripts action.

Last edited 10 years ago by nmarks (previous) (diff)

#6 @nmarks
9 years ago

  • Keywords dev-feedback needs-testing added

#7 @pauldewouters
9 years ago

Tested the patch #25247_bugfix.diff on WordPress 4.0 on a site with a variety of javascript files with dependencies. jQuery is loaded in the footer as expected, in first place so all depending scripts are loaded after it, using Marko's function.
Would be great to get this merged in, as scripts should really load in the footer

@nmarks
9 years ago

Revised patch for #25247

#8 @nmarks
9 years ago

Hey fellas,

I submitted a revised patch that makes the conditional more readable by expressing some of the conditions as variables and ensured that spacing etc follows the WordPress PHP coding standards.

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

#9 in reply to: ↑ 1 @nmarks
9 years ago

@nacin would you like this to be a test for WP_Scripts or WP_Dependencies?

Edit: I need to use a method from WP_Scripts to test this properly, but all the code changes are in the parent class WP_Dependencies. Should I put the test in Tests_Dependencies_Scripts or Tests_Dependencies?

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

@nmarks
9 years ago

Unit test for ticket #25247

#10 @nmarks
9 years ago

  • Keywords needs-unit-tests removed

Ok.

Since it relies on the WP_Scripts version of the do_item method, I put the unit test in tests/dependencies/scripts.php.

I also verified that it fails without my patch and passes with my patch.

Please Note: If you are testing, I have put the unit test and the core fix in 2 separate patches so you'll need both:

[Core Patch](https://core.trac.wordpress.org/attachment/ticket/25247/%2325247_bugfix_v2.diff)
[Unit Test](https://core.trac.wordpress.org/attachment/ticket/25247/25247-unit-test.diff)

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

#11 @gitlost
9 years ago

I think the no src issue is a bit of a red herring. The real problem here - as stated by the OP markoheijnen - is that the do_items() loop is removing an item from the to_do list whether or not it succeeded. An issue reported by helgatheviking on WP stackexchange (Loading a script with a dependency, is unloading another script's dependency is another symptom of the problem, which can be seen simply with this setup:

		$scripts = new WP_Scripts;
		$scripts->add( 'jquery-ui-datepicker', 'one', array(), 'v1', 1 ); 
		$scripts->add( 'wc-admin-meta-boxes', 'two', array( 'jquery-ui-datepicker' ) ); 
		$scripts->add( 'My_Plugin_Metabox', 'three', array( 'wc-admin-meta-boxes' ), 'v1', 1 ); 
		$scripts->enqueue( array( 'My_Plugin_Metabox' ) );
		$scripts->do_items( false, 0 );
		$scripts->do_items( false, 1 );

Note the mismatch in groups between 'wc-admin-meta-boxes' and 'jquery-ui-datepicker', If you run this, 'jquery-ui-datepicker' won't be outputted, it's just appended to the $in_footer array in WP_Scripts and left there, and since the array is no longer used in do_footer_items(), it gets lost.

Basically it seems that when the code was changed to do two passes of do_items() instead of one, the loop wasn't updated. The fix is simple, only remove the item from the to_do list when it's successfully added (or when the src is null).

The helgatheviking scenario also shows up another issue, in that although with the loop fix 'jquery-ui-datepicker' will get outputted, it will output in the footer, after the dependent 'wc-admin-meta-boxes' which gets outputted in the head. To fix this it would seem best to force any dependencies to the lowest common group, which can I think be achieved by setting the class $this->group when recursing in set_group().

I'll attach a patch and unit tests with these changes.

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

@gitlost
9 years ago

Proposed patch.

@gitlost
9 years ago

Unit tests for patch.

#12 @gitlost
9 years ago

Also the following is an optional patch to WP_Scripts, removing references to the vestigal $in_footer array, and also replacing the use of item $args === 1 with add_data('group', 1) (by overriding the add() function) - having the group state in two separate variables is not the best idea and no doubt legacy again...

@gitlost
9 years ago

Optional cleanup patch to WP_Scripts.

#13 @nmarks
9 years ago

Tested gitlost's patch, looks good.

@gitlost
9 years ago

Refresh on unit tests to patch to latest version.

#14 @gitlost
9 years ago

Would like to see this bug get more attention, it's 18 months old and in a fundamental part of WP which shouldn't have bugs (and where some non-bug changes seem to be getting more consideration).

@gitlost
9 years ago

Refresh for 4.4.

@gitlost
9 years ago

Refresh for 4.4.

#15 @gitlost
9 years ago

Refreshes for gardening drive - 25247_dependencies.diff still good.

#16 @OddOneOut
8 years ago

Just got hit by this as well, using a clean installation of WP 4.3. Hopefully the above patches get merged soon.

@gitlost
8 years ago

Refresh for 4.5.

#17 @gitlost
8 years ago

Refresh for 4.5 Bug Scrub. 25247_dependencies.diff and 25247_tests3.diff still good.

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


8 years ago

#19 @chriscct7
8 years ago

  • Keywords has-unit-tests added

Unit tests are still good to go.

#20 @chriscct7
8 years ago

  • Owner set to chriscct7
  • Status changed from new to reviewing

@gitlost
8 years ago

Updated @since to 4.5 on overridden add().

#21 @gitlost
8 years ago

Per https://wordpress.slack.com/archives/core/p1454692115001728, set since to 4.5 on overridden add() in class.wp-scripts.php.

@gitlost
8 years ago

Updated in light of [36550] (#35643).

@gitlost
8 years ago

Updated in light of [36550] (#35643) and #35873.

@gitlost
8 years ago

Refreshed to run against trunk with no "hunk succeeded" messages.

#22 @gitlost
8 years ago

The above cater for the changes [36550] re #35643, and also relate to #35873.

@gitlost
8 years ago

Fixed outputExpectedRegex stuff.

#23 @ocean90
8 years ago

@gitlost Can you please combine the patches into one patch? It's currently unclear which patches need to be applied.

@gitlost
8 years ago

Combined all diffs per @ocean90 request.

#24 @gitlost
8 years ago

There you go! (Combined in the scripts stuff as well although it's optional.)

#25 in reply to: ↑ 3 @ocean90
8 years ago

Replying to ocean90:

This script has worked until 3.6:

/**
 * Prints jQuery in footer on front-end.
 */
function ds_print_jquery_in_footer( &$scripts) {
	if ( ! is_admin() )
		$scripts->add_data( 'jquery', 'group', 1 );
}
add_action( 'wp_default_scripts', 'ds_print_jquery_in_footer' );

With 3.6 I would set the group arg for the dependencies to 1 too, but that doesn't work.

After [36550] this is working now:

<?php
/**
 * Prints jQuery in footer on front-end.
 */
function ds_print_jquery_in_footer( &$scripts) {
        if ( ! is_admin() ) {
                $scripts->add_data( 'jquery-core', 'group', 1 );
                $scripts->add_data( 'jquery', 'group', 1 );
                $scripts->add_data( 'jquery-migrate', 'group', 1 );
        }
}
add_action( 'wp_default_scripts', 'ds_print_jquery_in_footer' );

#26 @ocean90
8 years ago

  • Keywords dev-feedback removed

@gitlost Thanks! So let's ignore the scripts part for now.
I've removed the change to unset( $this->to_do[$key] ); and the tests are still passing. Is this change still necessary? If yes, we need a test which fails before the change.

#27 follow-up: @gitlost
8 years ago

No, I don't think the change is necessary (although it seems more logical to me) and I don't have a failing test so I suppose it's turned into a bit of a hijack now given [36550] so this ticket should probably now be closed (and the proposed removal of the legacy scripts stuff put into a new ticket)?

#28 @ocean90
8 years ago

In 36596:

Tests: Test that jQuery can be moved into footer after [36550].

Props gitlost.
See #25247.

#29 in reply to: ↑ 27 @ocean90
8 years ago

  • Keywords needs-testing removed
  • Milestone changed from Future Release to 4.5

Replying to gitlost:

No, I don't think the change is necessary (although it seems more logical to me) and I don't have a failing test so I suppose it's turned into a bit of a hijack now given [36550] so this ticket should probably now be closed (and the proposed removal of the legacy scripts stuff put into a new ticket)?

Yes, please create a ticket for the scripts change and I'll add your test test_group_mismatch_in_deps() to #35873.


I'm going to close this ticket as fixed since the original issue is fixed, see [36596].

#30 @ocean90
8 years ago

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

#31 @pcfreak30
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I have recently encountered this myself. If you force a item to the footer, but it has dependencies, the dependencies may not go to the footer at all due to being removed from the to_do array. An example is forcing the "jquery" script alias to the footer.

<?php
add_action('wp_print_scripts', function () {
    if (!is_admin()) {
        $jquery = wp_scripts()->registered['jquery'];
        wp_dequeue_script('jquery');
        wp_deregister_script('jquery');
        wp_register_script('jquery', $jquery->src, $jquery->deps, $jquery->ver, true);
    }
}, 0);

This is relying on jquery to be a dependency to other items. So it is more of a 2nd level dependency issue.

A very simple 2-3 line patch is attached

Last edited 8 years ago by pcfreak30 (previous) (diff)

#32 @ocean90
8 years ago

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

@pcfreak30 Please create a new ticket for this.

#33 @pcfreak30
8 years ago

@ocean90 Isn't this the same issue as what I am pointing out?

#34 @pcfreak30
8 years ago

Ticket #35963 created

Note: See TracTickets for help on using tickets.