WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 3 months ago

#20683 reopened enhancement

WP_Dependencies' constructor should accept a string/array value for $deps rather than converting a string to an empty array

Reported by: vhauri Owned by: markjaquith
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: has-patch
Focuses: Cc:

Description

Currently, WP_Dependencies' constructor takes any $deps value that's not an array and converts it to an empty array. I propose bringing the constructor's behavior more in line with standard WP function paramater behavior by accepting a string as a single dependency. Essentially this involves adding an is_string() check on $deps before checking if it's not an array, and creating an array with a single element in the case that it is a string.

if ( !is_array($this->deps) )
  $this->deps = array();

becomes

if ( is_string( $this->deps ) )
  $this->deps = (array) $this->deps;
elseif ( !is_array( $this->deps ) )
  $this->deps = array();

See the attached diff for a proposed patch.

Attachments (3)

20683.diff (514 bytes) - added by vhauri 2 years ago.
Patch to allow a single $deps in WP_Dependencies to be passed as a string
20683.2.diff (515 bytes) - added by vhauri 2 years ago.
Updated diff with clearer array typecasting as suggested by @scribu
20683.3.diff (698 bytes) - added by ryan 18 months ago.
Revert

Download all attachments as: .zip

Change History (19)

vhauri2 years ago

Patch to allow a single $deps in WP_Dependencies to be passed as a string

comment:1 scribu2 years ago

  • Keywords has-patch added

+1, but I think array( $this->deps ) is clearer than (array) $this->deps.

vhauri2 years ago

Updated diff with clearer array typecasting as suggested by @scribu

comment:3 markjaquith21 months ago

  • Milestone changed from Awaiting Review to 3.5
  • Owner set to markjaquith
  • Status changed from new to accepted

Looks good. Nice common sense API improvement.

comment:4 markjaquith21 months ago

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

In [21420]:

Let WP_Dependencies accept a string for a single dependency instead of requiring an array wrapper. props vhauri. fixes #20683

comment:6 dd3221 months ago

In [21421]:

Fix the plupload script enqueue, props SergeyBiryukov, Fixes #21467. See #20683.

comment:5 azaozz21 months ago

In [21481]:

When WP_Dependencies accept a string for a single dependency, make sure the string is not empty, see #20683

comment:6 nacin19 months ago

  • Keywords revert added
  • Resolution fixed deleted
  • Status changed from closed to reopened

This seemed like an easy win, but will likely be this release's hey-look-we-broke-your-scripts bug if it gets into 3.5 final. See #22111. Re-opening.

comment:7 scribu19 months ago

Ugh... yes, let's revert it.

comment:8 DrewAPicture18 months ago

From @nacin's comment on #22111, seems like it's not currently worth rocking the boat. Is it coming out or staying in?

ryan18 months ago

Revert

comment:10 ryan18 months ago

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

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:11 follow-up: Otto4212 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I don't accept that this is unsolvable.

The problem here seems to be that by allowing $deps to be a string, then somebody who had accidentally put a version there (like in #22111) would get a non-existent dependency. The code in WP_Dependency would then simply not include the code because of a missing dependency.

This problem can be solved by simply ignoring non-existent dependencies and letting the script/stylesheet load anyway. Yes, it may be broken, but it will definitely be broken if it doesn't load at all.

Dependencies define the order of loading, but we don't necessarily have to state that they will also define absolute dependence upon the defined enqueued items. If somebody has a non-existent dependency, then that's clearly a coding error, and throwing a _doing_it_wrong but loading the scripts anyway would make more sense.

Looks to me like you can just remove those checks in function all_deps, or change them to keep going anyway but throw a debugging error.

comment:12 SergeyBiryukov12 months ago

  • Keywords revert removed
  • Milestone changed from 3.5 to Awaiting Review

comment:13 in reply to: ↑ 11 rmccue12 months ago

Replying to Otto42:

I don't accept that this is unsolvable.

The problem here seems to be that by allowing $deps to be a string, then somebody who had accidentally put a version there (like in #22111) would get a non-existent dependency. The code in WP_Dependency would then simply not include the code because of a missing dependency.

This problem can be solved by simply ignoring non-existent dependencies and letting the script/stylesheet load anyway. Yes, it may be broken, but it will definitely be broken if it doesn't load at all.

Dependencies define the order of loading, but we don't necessarily have to state that they will also define absolute dependence upon the defined enqueued items. If somebody has a non-existent dependency, then that's clearly a coding error, and throwing a _doing_it_wrong but loading the scripts anyway would make more sense.

Surprisingly, I agree with you. Dependencies do mean that the code relies on it, but getting a Javascript error with jQuery is undefined (e.g.) seems a little easier to debug than the script not loading at all.

comment:14 nacin3 months ago

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