Make WordPress Core

Opened 21 months ago

Last modified 3 months ago

#21520 new defect (bug)

Prevent recursive script dependencies in wp_enqueue_script

Reported by: batmoo Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Script Loader Keywords:
Focuses: Cc:


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 (1)

21520.patch (636 bytes) - added by azaozz 21 months ago.

Download all attachments as: .zip

Change History (11)

azaozz21 months ago

comment:1 ryan21 months ago

Related: #20683

comment:2 azaozz21 months 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 );

comment:3 toscho21 months 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.

comment:4 ryan20 months 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.

comment:5 follow-up: ryan20 months 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().

comment:6 in reply to: ↑ 5 azaozz20 months 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.

comment:7 jondavidjohn20 months 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.

comment:8 ryan18 months 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

comment:9 ryan18 months ago

  • Milestone changed from 3.5 to Future Release

comment:10 nacin3 months ago

  • Component changed from General to Script Loader
Note: See TracTickets for help on using tickets.