Opened 12 years ago
Closed 11 years ago
#9870 closed defect (bug) (fixed)
Fix for WP_Dependencies::dequeue
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 2.9 | Priority: | normal |
Severity: | normal | Version: | 2.8 |
Component: | JavaScript | Keywords: | has-patch tested needs-unit-tests |
Focuses: | Cc: |
Description
The WP_Dependencies::dequeue method has three major flaws that prevent it from working: does not split handle around '?' to divide handle from arg (see the WP_Dependencies::enqueue method), tries to remove the handle from the queue array by using the handle as an index of the array when the handle is a value of the array, and does not remove a relevant entry from the args array if the handle has an arg.
The provided patch (wp-dependencies-patch.txt) was tested with 2.8-beta1-11389 and corrected all three issues.
Attachments (1)
Change History (12)
#1
@
12 years ago
- Component changed from General to JavaScript
- Milestone changed from Unassigned to 2.8
#3
@
12 years ago
Frankly, I don't understand that either; however, I'm matching the functionality found in the WP_Dependencies::enqueue method. The existing code for the two methods is as follows:
function enqueue( $handles ) { foreach ( (array) $handles as $handle ) { $handle = explode('?', $handle); if ( !in_array($handle[0], $this->queue) && isset($this->registered[$handle[0]]) ) { $this->queue[] = $handle[0]; if ( isset($handle[1]) ) $this->args[$handle[0]] = $handle[1]; } } } function dequeue( $handles ) { foreach ( (array) $handles as $handle ) unset( $this->queue[$handle] ); }
As you can see, the enqueue method does a number of things that the dequeue method does not attempt to duplicate. In addition, the fact that it attempts to improperly remove an item from the array using a value as an index causes a complete failure of the method. Thus, it is extremely easy to create unexpected results. The code I used to test the functionality of the class is as follows:
$dep = new WP_Dependencies; $dep->add( 'test', '' ); $dep->add( 'another', '' ); $dep->add( 'this', '' ); $dep->add( 'that', '' ); $dep->add( 'the_other', '' ); $dep->add( 'handle', '' ); $dep->add( 'not_used', '' ); $dep->add( 'data_handle', '' ); $dep->enqueue( array( 'test', 'another' ) ); $dep->enqueue( array( 'this', 'that' ) ); $dep->enqueue( 'the_other' ); $dep->enqueue( 'handle?data' ); $dep->enqueue( 'data_handle?dh_data' ); $dep->dequeue( array( 'test', 'another', 'handle?data' ) ); print_r( $dep->queue ); print_r( $dep->args );
When run, you can see that the dequeue call fails to remove anything from the queue and args.
While I may not understand all the use cases, it does not change the fact that the dequeue method as it stands now fails to remove what is added by the queue method. My patch resolves the issues and allows the method to properly remove handles and args added by the queue method.
FYI: Not that it matters to me, but it may somewhere internally. This isn't a JavaScript component. The class is used as a parent class for both WP_Styles and WP_Scripts.
#4
@
12 years ago
Re the component, it doesn't matter much -- it's just that Andrews usually looks into tickets related to WP_Dependencies.
#5
@
12 years ago
- Keywords dev-feedback added
- Owner changed from chrisbliss18 to Denis-de-Bernardy
- Status changed from new to assigned
andrew?
#6
@
12 years ago
- Cc westi added
- Keywords needs-unit-tests added
- Owner changed from Denis-de-Bernardy to azzozz
- Status changed from assigned to reviewing
Assiging to Andrew for review.
Will need merging with BackPress when changed.
Would be good to write some tests for these classes at some point.
#8
@
12 years ago
WP_Dependencies::dequeue hasn't been changed since the class was added over a year ago and doesn't seem used anywhere in WordPress, may possibly be used in BackPress.
At the same time it doesn't really dequeue a script as all dependencies are left in the queue. Despite that the patch seems like a good idea, will at least bring it in line with WP_Dependencies::enqueue.
Support for query strings in the handle seems to have been added for back-compat, haven't seen that used anywhere. Will have to coordinate with other apps using BackPress before adding it.
Can you highlight an actual use-case for the behavior? I can't make sense of using a handle with a param in it.