apply_filters skip all handlers in the next priority, when remove_filter is called

[wvega]

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

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?

[wvega]

Keep $this->callbacks[ $priority ] even if empty.

[wvega]

Also, if keeping $this->callbacks[ $priority ] around, even when empty, is possible, could $this->resort_active_iterations() be avoided inside WP_Hook::remove_filter?

[wvega]

My patch is causing other problems. If you expect isset( $wp_filter[ 'some_tag' ] ) to be false after all filters/actions have been removed, then the patch breaks that feature.

I'll try to come up with a better approach.

[gitlost]

Duplicate of #40685 I think.

[wvega]

Replying to gitlost:

Duplicate of #40685 I think.

Yes, that's the same problem I encountered and was trying to fix.