Make WordPress Core

Opened 12 years ago

Closed 8 months ago

#20683 closed enhancement (wontfix)

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

Reported by: vhauri's profile vhauri Owned by: markjaquith's profile markjaquith
Milestone: Priority: normal
Severity: normal Version: 2.6
Component: Script Loader Keywords: has-patch close 2nd-opinion needs-refresh
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 (4)

20683.diff (514 bytes) - added by vhauri 12 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 12 years ago.
Updated diff with clearer array typecasting as suggested by @scribu
20683.3.diff (698 bytes) - added by ryan 12 years ago.
Revert
35841.patch (3.5 KB) - added by sebastian.pisula 8 years ago.

Download all attachments as: .zip

Change History (24)

@vhauri
12 years ago

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

#1 @scribu
12 years ago

  • Keywords has-patch added

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

@vhauri
12 years ago

Updated diff with clearer array typecasting as suggested by @scribu

#3 @markjaquith
12 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
12 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
12 years ago

In [21421]:

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

#7 @azaozz
12 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
12 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
12 years ago

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

#10 @DrewAPicture
12 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
12 years ago

Revert

#12 @ryan
12 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
11 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
11 years ago

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

#15 in reply to: ↑ 13 @rmccue
11 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
10 years ago

  • Component changed from General to Script Loader

#17 @chriscct7
9 years ago

  • Keywords dev-feedback added

#18 @ocean90
8 years ago

#35841 was marked as a duplicate.

#19 @desrosj
4 years ago

  • Keywords close 2nd-opinion needs-refresh added; dev-feedback removed
  • Milestone set to Awaiting Review
  • Version set to 2.6

The latest patch needs a refresh against current trunk.

I'm also considering just closing this out as wontfix. WP_Depenencies was introduced in 2.6, and specifying dependencies in array format when using wp_enqueue_script()/wp_enqueue_style() is now such a common practice that I don't know that the benefit of this change still exists.

Marking close/2nd-opinion for others to weigh in.

#20 @JeffPaul
8 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

Slow concur here with @desrosj and accepting the close on this as the value to change is outweighed by further effort here to do so.

Note: See TracTickets for help on using tickets.