WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 4 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 dev-feedback
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 4 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 4 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 (20)

@vhauri
4 years ago

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

#1 @scribu
4 years ago

  • Keywords has-patch added

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

@vhauri
4 years ago

Updated diff with clearer array typecasting as suggested by @scribu

#3 @markjaquith
4 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.

#4 @markjaquith
4 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

#6 @dd32
4 years ago

In [21421]:

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

#7 @azaozz
4 years ago

In [21481]:

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

#8 @nacin
3 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.

#9 @scribu
3 years ago

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

#10 @DrewAPicture
3 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?

@ryan
3 years ago

Revert

#12 @ryan
3 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

#13 follow-up: @Otto42
3 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.

#14 @SergeyBiryukov
3 years ago

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

#15 in reply to: ↑ 13 @rmccue
3 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.

#16 @nacin
2 years ago

  • Component changed from General to Script Loader

#17 @chriscct7
4 months ago

  • Keywords dev-feedback added
Note: See TracTickets for help on using tickets.