Make WordPress Core

Opened 7 years ago

Last modified 4 years ago

#40393 new defect (bug)

Using remove_action within an action callback skips over execution of other callbacks with lower priority

Reported by: weeblrpress's profile weeblrpress Owned by:
Milestone: Awaiting Review Priority: normal
Severity: major Version:
Component: Plugins Keywords: dev-feedback
Focuses: Cc:

Description

Description:

When remove_action is used by an action callback to remove itself from the list of callbacks, this results in all callbacks hooked with the immediately lower priority to be skipped by apply_filters.

Here is simple code to demonstrate:

<?php

class Sample {
    
    public function test()
        {
                add_action('custom_action', array($this, 'callback_1'), 1);
                add_action('custom_action', array($this, 'callback_2'));
                add_action('custom_action', array($this, 'callback_3'), 20);

                echo '<h1>First Run</h1>';
                echo '<pre>';
                do_action('custom_action');
                echo '</pre>';
                echo '<h1>Second Run</h1>';
                echo '<pre>';
                do_action('custom_action');
                echo '</pre>';
        }

        public function callback_1()
        {
                echo "Callback 1\n";
        }

        public function callback_2()
        {
                echo "Callback 2\n";
                remove_action('custom_action', array($this, 'callback_2'));
        }

        public function callback_3()
        {
                echo "Callback 3 - Priority 20\n";
        }
}

$runner = new Sample;
$runner->test();

The output is:

First Run
Callback 1
Callback 2

Second Run
Callback 1
Callback 3 - Priority 20

The expected output should be:

First Run
Callback 1
Callback 2
Callback 3 - Priority 20

Second Run
Callback 1
Callback 3 - Priority 20

The net effect of this issue is that a plugin using remove_action in that way will break another, totally unrelated plugin, if they both add actions on same tag, with different prorities.

WooCommerce is using this method, it uses remove_action inside a pre_get_posts action callback. This broke our Accelerated Mobile Pages plugin when doing AMP pages for WooCommerce.
This was reported to WooCommerce here, but it was then suggested it was a core issue.

Additional notes:

  • all callbacks must object methods. The problem does not occur if callbacks are functions.
  • the callback using remove_action must be the only registered callback for that priority level
<?php
add_action('custom_action', array($this, 'callback_1'));
add_action('custom_action', array($this, 'callback_2'));
add_action('custom_action', array($this, 'callback_3'), 20);
add_action('custom_action', array($this, 'callback_4'), 20);

will not cause the issue, all callbacks are executed.

<?php
add_action('custom_action', array($this, 'callback_1'), 5);
add_action('custom_action', array($this, 'callback_2'));
add_action('custom_action', array($this, 'callback_3'), 20);

will cause the issue, callback_3 is not executed.

  • only the next priority level is removed (but all callbacks for that level are removed):
<?php
add_action('custom_action', array($this, 'callback_1'), 1);
add_action('custom_action', array($this, 'callback_2'));
add_action('custom_action', array($this, 'callback_3'), 15);
add_action('custom_action', array($this, 'callback_4'), 20);

will cause callback_3 to be skipped and callback_4 to be executed normally

<?php
add_action('custom_action', array($this, 'callback_1'), 1);
add_action('custom_action', array($this, 'callback_2'));
add_action('custom_action', array($this, 'callback_3'), 20);
add_action('custom_action', array($this, 'callback_4'), 20);

will cause both callback_3 and callback_4 to be skipped

Related: the following tickets are related, even duplicates, but are marked as fixed, which is why I am opening this ticket: #33144, 21169, 37679

Change History (11)

#1 @mgibbs189
7 years ago

  • Keywords dev-feedback added
  • Severity changed from normal to major
  • Version 4.7.3 deleted

@boonebgorges Any chance you could pass this along to the appropriate WP core dev? This ticket is 4 months old.

This issue seems to be affecting numerous WooCommerce-related plugins, including FacetWP.

@weeblrpress's bug report seems very thorough and I'm able to reproduce as well (thanks by the way).

#2 @boonebgorges
7 years ago

@mgibbs189 Maybe @pento or @jbrinley could have a look?

If someone were able to give a potential diagnosis or gesture toward a fix, it'd be more likely to move more quickly. Sadly, bugs like this rarely fix themselves :-(

#3 @weeblrpress
7 years ago

@mgibbs189 @boonebgorges

From recollection, it was coming from the main loop in WP_Hook::apply_filters():

<?php

this->iterations[ $nesting_level ] = array_keys( $this->callbacks );

do {
  /* iterate over current priority level */
  foreach ( $this->callbacks[ $priority ] as $the_ ) {

  $value = call_user_func_array( $the_['function'], $args );

  /* end iterate over current priority level */
  }

} while ( false !== next( $this->iterations[ $nesting_level ] ) );

The while statement execute a next on the array to get callbacks for next priority level. But if remove_action is called upon the currently being executed action during all_user_func_array(), and current action is the only one in this prio level, then the callback set is removed:

from WP_Hook::remove_filter():

<?php

if ( $exists ) {
        unset( $this->callbacks[ $priority ][ $function_key ] );
        if ( ! $this->callbacks[ $priority ] ) {
                unset( $this->callbacks[ $priority ] );
                if ( $this->nesting_level > 0 ) {
                        $this->resort_active_iterations();
                }
        }
}

So when next() is executed, one priority level is skipped.

Hope this helps, maybe the fix is that remove_filter should not remove the callback priority level?

Rgds

#4 @jbrinley
7 years ago

WP_Hook::resort_active_iterations() appropriately handles inserting a filter into the array, but it looks like a bug slipped through for removing one.

In the while loop, if it changes from

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

to

                        while ( current( $iteration ) < $current ) {
                                $next = next( $iteration );
                                if ( false === $next ) {
                                        break;
                                } elseif ( $next > $current ) {
                                        // The current key has gone away. Point
                                        // to the previous key so it's grabbed
                                        // on the next iteration
                                        prev( $iteration );
                                        break;
                                }
                        }

it seems to resolve the issue. I've haven't run the full test suite against that yet to test for regressions.

Last edited 7 years ago by jbrinley (previous) (diff)

#5 @mgibbs189
7 years ago

@jbrinley Makes sense, thank you. That explains why, in WP_Hook::remove_filter(), moving the unset() to after resort_active_iterations fixed the issue (sort of).

Last edited 7 years ago by mgibbs189 (previous) (diff)

#6 @mgibbs189
7 years ago

@weeblrpress Any chance you could confirm that @jbrinley's fix resolves the issue in your scenarios?

Thanks for submitting the ticket, BTW :)

#7 @weeblrpress
7 years ago

@mgibbs189 @jbrinley

You're welcome! Yes, that change in WP_Hook::resort_active_iterations() makes all the registered action be executed.

Not withstanding running the full test suite, I didn't see any adverse effects either.

#8 @mgibbs189
6 years ago

@jbrinley have you been able to regression test yet?

#10 @weeblrpress
6 years ago

@nprasath002 9 years old :)

This sure does seem related although in my testing I did not have the problem with functions as handlers, only with object methods.

#11 @jchristopher
4 years ago

Just ran in to this issue as well, can confirm @jbrinley's fix resolves the issue in my use case as well.

Note: See TracTickets for help on using tickets.