Make WordPress Core

Opened 7 years ago

Closed 14 months ago

Last modified 6 months ago

#41185 closed defect (bug) (wontfix)

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

Reported by: kraftner's profile kraftner Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.8
Component: Plugins Keywords: has-unit-tests needs-patch
Focuses: Cc:


If you add a callback for a hook inside a callback on the very same hook current_priority() (introduced in returns false. Simple example:

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 ":(";

}, 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.

Attachments (3)

41185.patch (1005 bytes) - added by kraftner 7 years ago.
Test reproducing the bug
41185.2.patch (1001 bytes) - added by kraftner 7 years ago.
41185.3.patch (1.4 KB) - added by kraftner 7 years ago.

Download all attachments as: .zip

Change History (11)

This ticket was mentioned in Slack in #core by kraftner. View the logs.

7 years ago

#2 @johnjamesjacoby
7 years ago

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.


  • 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
Last edited 7 years ago by johnjamesjacoby (previous) (diff)

7 years ago

Test reproducing the bug

#3 @kraftner
7 years ago

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?

#4 @DrewAPicture
7 years ago

  • Keywords has-unit-tests needs-patch added

7 years ago

#5 @kraftner
7 years ago

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

#6 @kraftner
7 years ago

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 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...

7 years ago

#7 @kraftner
14 months ago

  • Resolution set to wontfix
  • Status changed from new to closed

Since this only applies to PHP < 7 I am closing this since says this isn't supported anymore anyway.

#8 @swissspidy
6 months ago

  • Milestone Awaiting Review deleted
Note: See TracTickets for help on using tickets.