[wp-trac] [WordPress Trac] #40906: apply_filters skip all handlers in the next priority, when remove_filter is called

[WordPress Trac]

#40906: apply_filters skip all handlers in the next priority, when remove_filter is
called
--------------------------+-----------------------------
 Reporter:  wvega         |      Owner:
     Type:  defect (bug)  |     Status:  new
 Priority:  normal        |  Milestone:  Awaiting Review
Component:  Plugins       |    Version:  trunk
 Severity:  normal        |   Keywords:
  Focuses:                |
--------------------------+-----------------------------
 If a hook handler removes itself and it is the last handler for that hook
 at that priority, `apply_filters` will skip all handlers in the next
 priority.

 The conditions I identified as necessary to reproduce this issue are:

 1. The priority of the handler that removes itself must not be the first
 priority for that hook.
 2. The handler that removes itself must be the only handler registered for
 that priority at the time `remove_filter` is called.
 3. There must but handlers registered for the same hook using a priority
 greater than the priority of the handler that removes itself.

 To illustrate the problem better, please consider the following code:

 {{{#!php
 <?php

 public function test_do_action_with_handler_that_removes_itself() {
     $a = new MockAction();
     $b = new MockAction();

     $this->hook = new WP_Hook();

     $this->hook->add_filter( 'handler_that_removes_itself',
 '__return_null', 10, 1 );
     $this->hook->add_filter( 'handler_that_removes_itself', array( $this,
 '_handler_that_removes_itself' ), 20, 1 );
     $this->hook->add_filter( 'handler_that_removes_itself', array( $a,
 'action' ), 20 + 1, 1 );
     $this->hook->add_filter( 'handler_that_removes_itself', array( $b,
 'action' ), 20 + 2, 1 );

     $this->hook->do_action( array( null ) );

     $this->assertEquals( 1, $a->get_call_count(), 'One of the callbacks
 was never executed.' );
     $this->assertEquals( 1, $b->get_call_count() );
 }

 public function _handler_that_removes_itself() {
     $success = $this->hook->remove_filter( 'handler_that_removes_itself',
 array( $this, '_handler_that_removes_itself' ), 20 );
     $this->assertTrue( $success );
 }
 }}}

 The test above fails against [40871], because `$a->action` is never
 called.

 I took a look at #17817, but I don't think is a duplicate. I think it is
 an edge case for the modifications from [38571].

 The problem is that when a handler is removed, and there are no more
 handlers for the priority it was assigned to, `WP_Hook` calls
 `resort_active_iterations` leaving the array pointer on the position of
 the next priority. WordPress should continue calling the handlers in that
 position, but that's not what happens in this scenario. When
 `WP_Hook::apply_filters` executes `false !== next( $this->iterations[
 $nesting_level ] )` to figure out if there are more handlers to call, the
 pointer moves forward another position, completely skipping all handlers
 in the now current priority.

 I tried to solve this problem by setting the pointer of
 `$this->iterations[ $nesting_level ]` to the priority that was right
 before the priority used for `_handler_that_removes_itself`.

 That worked, but it also caused
 `Tests_WP_Hook_Add_Filter::test_remove_and_add_last_filter` to go into an
 infinite loop.

 Then, I found out that keeping the `$this->callbacks[ $priority ]` array,
 even after all handlers for that priority are removed, fixes the issue and
 causes no tests in the **hooks** group to break.

 I'm including a patch with the modifications that worked for me and the
 test from the beginning.

 Do you think that's a suitable solution?

--
Ticket URL: <https://core.trac.wordpress.org/ticket/40906>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform