Make WordPress Core

Opened 3 months ago

Last modified 2 months ago

#40685 new defect (bug)

/wp-includes/class-wp-hook.php function resort_active_iterations not working correct

Reported by: tim2017 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.7.4
Component: General Keywords:
Focuses: Cc:


if you have current priority of 5, whose only hook is cancelling itself (remove_filter) and the new priority list is for example (2,7,12), then this part

while ( current( $iteration ) < $current ) {
           if ( false === next( $iteration ) ) {

sets the current pointer to 7, so the next priority worked on is the 12 in apply_filters function, while it should be 7.

Attachments (1)

40685.patch (12.3 KB) - added by gitlost 3 months ago.
Demo patch, with unit tests.

Download all attachments as: .zip

Change History (4)

#1 @gitlost
3 months ago

Nice bug!

At first I thought it might be possible to fix this by manipulating the internal array pointer of iterations to point to the previous element on remove_filter() so that the next() in apply_filters() would work correctly - but a major flaw with this is that if the filter being removed is the first in iterations then there's no previous element to go back to! So the next() in apply_filters() will always skip an element.

So just not calling resort_active_iterations() on remove_filter() seems like a simple way to fix the issue. But this leaves the iterations array invalid, so its return needs to be checked before using it in apply_filters(), which affects the performance of the loop. (It might still be a good, more BC solution though.)

The approach taken in the attached demo patch is to not call resort_active_iterations() either, but also to not unset the callback priority entries at all but leave them empty, so iterations is left pointing at a "zombie" entry, avoiding the need for a check.

This has some behavioural issues - for instance to get the full unit tests working required changing some of them to not test for isset( $hook->callbacks[ $priority ] ) but ! empty() instead, and also not do assertArrayNotHasKey( 'blah', $wp_filter ) but has_filter( 'blah' ) - so it's very much a demo.

The patch includes unit tests that fail appropriately.

3 months ago

Demo patch, with unit tests.

#2 @wvega
2 months ago

#40906 was marked as a duplicate.

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

2 months ago

Note: See TracTickets for help on using tickets.