WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 5 months ago

#21520 new defect (bug)

Prevent recursive script dependencies in wp_enqueue_script

Reported by: batmoo Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: needs-patch
Focuses: Cc:
PR Number:

Description

If a script sets itself as a dependency, we should catch that, strip out the dependency, and throw a _doing_it_wrong:

wp_enqueue_script( 'my-script', '/path/to/file.js', array( 'my-script' ) );

This may need to be done at the lowest level possible, i.e. _WP_Dependency

Attachments (2)

21520.patch (636 bytes) - added by azaozz 7 years ago.
recursion.patch (639 bytes) - added by pross 3 years ago.

Download all attachments as: .zip

Change History (16)

@azaozz
7 years ago

#1 @ryan
7 years ago

Related: #20683

#2 @azaozz
7 years ago

Wouldn't that break WP_Dependencies and throw PHP fatal error the first time it's attempted (so the dev that tried it would know he did something wrong)?

If we go there we may need to handle circular dependencies too. Example:

add_action( 'init', 'deps_test' );
function deps_test() {
	wp_register_script( 'my-1', '/one.js', array('my-2'), '123', true );
	wp_register_script( 'my-2', '/two.js', array('my-1'), '123', true );
	
	wp_enqueue_script('my-1');
}

#3 @toscho
7 years ago

  • Cc info@… added

I would rather see such code break with a yelling error to force authors to catch it before they release it into the wild.

#4 @ryan
7 years ago

  • Milestone changed from Awaiting Review to 3.5

Since #20683 these circular dependencies run to memory exhaustion. Even with [21481] I'm seeing exhaustion.

#5 follow-up: @ryan
7 years ago

So these themes I'm seeing break were passing a string instead of an array for the dependencies. Thus the dependency was ignored. With [21420] these dependencies suddenly started working. Alas, some of them were circular dependencies that no one noticed before since the string dependency was thrown out.

This is very edge, but it is fatal and rather hard to track down since the exhaustion usually happens somewhere in the depths of all_deps().

#6 in reply to: ↑ 5 @azaozz
7 years ago

Replying to ryan:

...
This is very edge, but it is fatal and rather hard to track down since the exhaustion usually happens somewhere in the depths of all_deps().

To stay back-compat seems we would need to revert [21420] until there is a check to prevent circular dependencies in WP_Dependencies.

#7 @jondavidjohn
7 years ago

I'm with toscho on this...

Seems like a user error that should be front and center. Glazing over it might cause more confusion than benefit.

#8 @ryan
7 years ago

In [22286]:

Revert [21420] and [21481]. Accepting a string caused back compat problems including the possibility of revealing previously hidden circular dependencies resulting in infinite loops.

fixes #20683 #22111
see #21520

#9 @ryan
7 years ago

  • Milestone changed from 3.5 to Future Release

#10 @nacin
6 years ago

  • Component changed from General to Script Loader

#11 @johnbillion
5 years ago

#30518 was marked as a duplicate.

#12 @marsjaninzmarsa
4 years ago

I've just spent ~1 hour worrying about why I'm getting segfault on every pageload, and with this simple path I have no idea why it isn't resolved yet...

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


4 years ago

#14 @boonebgorges
4 years ago

  • Keywords needs-patch added

@pross
3 years ago

Note: See TracTickets for help on using tickets.