current_priority() in WP_Hook broken after adding callback inside same hook

[kraftner]

If you add a callback for a hook inside a callback on the very same hook current_priority() (introduced in https://core.trac.wordpress.org/changeset/39430) returns false. Simple example:

<?php
add_action( 'init', function(){

        add_action( 'init', function(){} );

        global $wp_filter;

        $filter_name     = current_filter();
        $filter_instance = $wp_filter[ $filter_name ];
        $priority        = $filter_instance->current_priority();

        if( false === $priority ){
                echo ":(";
                exit();
        }

}, 0);

The problem seems to be that when you add another callback add_filter() in WP_Hook triggers resort_active_iterations() which runs a foreach over $this->iterations.
In PHP<=5.6 (not in PHP7+) foreach moves the internal array pointer and leaves it at the end of the array. This in turn then makes the false === current( $this->iterations ) check at the beginning of current_priority() return false.

So what I think we need to do is ensure that at the the end of resort_active_iterations() the array pointer of $this->iterations is back to where it was before.

As this touches the very very heart of WP I'm looking for feedback and confirmation before even trying to come up with a patch.

[johnjamesjacoby]

Good catch. If true, it is unintentional, and goes against efforts in the past 2 years to allow for and support same-action additions. I am unsure if same-priority was ever addressed, and without looking, doubt that there are unit-tests for this case.

Todos:

• Confirm the bug
• Find the related resolved issue from recent years
• Confirm missing test coverage
• Write tests to reproduce the bug
• Patch WP_Hook (likely as suggested above) to fix the comparison logic

[kraftner]

Test reproducing the bug

[kraftner]

Thanks for your fast feedback - I have attached a patch that adds a test reproducing this issue.

Concerning "Find the related resolved issue from recent years" - did you mean #17817 or something else?

[kraftner]

The earlier patch didn't properly clean up after itself.

[kraftner]

The more I look into it the more I come to the conclusion that the implementation of current_priority() as relying on the array pointer of $this->iterations as well as relying on $wp_filter[ current_filter() ] to get to that method at all is flawed. Let me explain:

First problem is that relying on $wp_filter[ current_filter() ] as proposed in https://core.trac.wordpress.org/ticket/39007#comment:4 is fragile, e.g. when the underlying filter has had all hooks including the one currently running removed in the meantime which leads to the removal of the corresponding WP_Hook instance in $wp_filter. (See the newly added test. Also not sure if that might needs to be filed as a separate issue.)

So even if you'd fix the array pointer (and work around the above mentioned issue with $wp_filter[ current_filter() ]) you still have the problem that at the point that you call current_priority() $this->iterations might not be available any more, e.g. when a hook removes itself as the last hook and is therefore cleared out at the beginning of resort_active_iterations(). This is because current( $this->iterations ) === [] after the removal but we're still running a hook under a priority that we can't determine any more with the current_priority() method.

I hope this is understandable as my head is already steaming right now...

[kraftner]

Since this only applies to PHP < 7 I am closing this since ​https://wordpress.org/about/requirements/ says this isn't supported anymore anyway.