WordPress.org

Make WordPress Core

Opened 5 months ago

Last modified 5 months ago

#41185 new defect (bug)

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

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

Description

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.

Attachments (3)

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

Download all attachments as: .zip

Change History (9)

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


5 months ago

#2 @johnjamesjacoby
5 months 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.

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

@kraftner
5 months ago

Test reproducing the bug

#3 @kraftner
5 months 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
5 months ago

  • Keywords has-unit-tests needs-patch added

@kraftner
5 months ago

#5 @kraftner
5 months ago

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

#6 @kraftner
5 months 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 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
5 months ago

Note: See TracTickets for help on using tickets.