WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 18 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 3 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 3 years ago.
Updated diff with clearer array typecasting as suggested by @scribu
20683.3.diff (698 bytes) - added by ryan 3 years ago.
Revert

Download all attachments as: .zip

Change History (19)

@vhauri3 years ago

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

comment:1 @scribu3 years ago

  • Keywords has-patch added

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

@vhauri3 years ago

Updated diff with clearer array typecasting as suggested by @scribu

comment:3 @markjaquith3 years 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 @markjaquith3 years 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 @dd323 years ago

In [21421]:

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

comment:7 @azaozz3 years ago

In [21481]:

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

comment:8 @nacin3 years 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:9 @scribu3 years ago

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

comment:10 @DrewAPicture3 years ago

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

@ryan3 years ago

Revert

comment:12 @ryan3 years 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:13 follow-up: @Otto422 years 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:14 @SergeyBiryukov2 years ago

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

comment:15 in reply to: ↑ 13 @rmccue2 years 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:16 @nacin18 months ago

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