WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#9870 closed defect (bug) (fixed)

Fix for WP_Dependencies::dequeue

Reported by: chrisjean Owned by: azaozz
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)

wp-dependencies-patch.txt (701 bytes) - added by chrisbliss18 5 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Denis-de-Bernardy5 years ago

  • Component changed from General to JavaScript
  • Milestone changed from Unassigned to 2.8

comment:2 Denis-de-Bernardy5 years ago

Can you highlight an actual use-case for the behavior? I can't make sense of using a handle with a param in it.

comment:3 chrisbliss185 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.

comment:4 Denis-de-Bernardy5 years ago

Re the component, it doesn't matter much -- it's just that Andrews usually looks into tickets related to WP_Dependencies.

comment:5 Denis-de-Bernardy5 years ago

  • Keywords dev-feedback added
  • Owner changed from chrisbliss18 to Denis-de-Bernardy
  • Status changed from new to assigned

andrew?

comment:6 westi5 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.

comment:7 westi5 years ago

  • Owner changed from azzozz to azaozz

Fix typo :-(

comment:8 azaozz5 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.

comment:9 Denis-de-Bernardy5 years ago

  • Milestone changed from 2.8 to 2.9

punting per IRC discussion. this can wait until 2.9 unless andrew has objections.

comment:10 Denis-de-Bernardy5 years ago

  • Keywords dev-feedback removed

comment:11 azaozz5 years ago

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

(In [12049]) Bring WP_Dependencies::dequeue() in line with WP_Dependencies::enqueue(), props chrisbliss18, fixes #9870

Note: See TracTickets for help on using tickets.