Make WordPress Core

Opened 2 months ago

Last modified 2 months ago

#62490 new defect (bug)

doing_action wrong in shutdown

Reported by: kkmuffme's profile kkmuffme Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version:
Component: General Keywords: has-patch has-unit-tests reporter-feedback
Focuses: Cc:

Description

<?php
add_action( 'init', 'wp_die', 10, 0 );

function last_this() {
        if ( doing_action( 'init' ) ) {
                trigger_error( 'yes', E_USER_WARNING );
        }
}
add_action( 'shutdown', 'last_this', 10, 0 );

You'll get "yes" in error log.

Change History (7)

This ticket was mentioned in PR #7852 on WordPress/wordpress-develop by @kkmuffme.


2 months ago
#1

  • Keywords has-patch has-unit-tests added

#2 @johnbillion
2 months ago

  • Keywords reporter-feedback added

If the init action calls wp_die() which calls exit which calls the shutdown handler, then this seems like correct behaviour to me. What am I missing?

#3 follow-up: @kkmuffme
2 months ago

"doing_action" implies that the hook is currently being executed, which means you could add an action on it.
But with shutdown this will never happen.

e.g.

<?php
add_action( 'init', 'wp_die', 10, 0 );

function last_this() {
        if ( doing_action( 'init' ) ) {
                add_action( 'init', 'foo', 20 );
        }
}
add_action( 'shutdown', 'last_this', 10, 0 );

function foo() {
        echo "foo";
}

It will never echo "foo" in that case

Additionally, the current behavior will keep the hooks after shutdown too.
e.g.

<?php
function first_this() {
        register_shutdown_function( 'my_shutdown' );
        wp_die();
}
add_action( 'init', 'first_this', 10, 0 );

function my_shutdown() {
    // this will be "init" - but no "init" processing will take place at all at that point, since we are AFTER "shutdown"
    trigger_error( current_action(), E_USER_WARNING );
}

function last_this() {
        // this will be "shutdown"
        trigger_error( current_action(), E_USER_WARNING );
}
add_action( 'shutdown', 'last_this', 10, 0 );

#4 @kkmuffme
2 months ago

Btw I'd even go a step further (as a separate ticket/PR) and check that if $wp_current_filter isn't empty on shutdown, we should prevent the last hooks callback from running again in shutdown (in case that hook would be executed on shutdown), since calling exit (or throwing an error) in a shutdown function, will also exit any/all shutdown functions that would run after it - and since we already know that at this point in time, it's easy enough to prevent.

The question is purely whether only this filter callback should be removed or if that filter is called again, we just return and stop WP execution (but not exit; to allow any other shutdown to run)

#5 in reply to: ↑ 3 @johnbillion
2 months ago

Replying to kkmuffme:

"doing_action" implies that the hook is currently being executed, which means you could add an action on it.
But with shutdown this will never happen.

This does work but the reason "foo" doesn't get output in your example is because wp_die() has called exit on an earlier priority, therefore preventing further callbacks from firing. I don't think that has anything to do with the shutdown hook.

These examples are a bit contrived though - what was it you were trying to do?

#6 @kkmuffme
2 months ago

wp_die() has called exit on an earlier priority, therefore preventing further callbacks from firing.

Exactly, but there is no way of me knowing that something has called wp_die()/exit in real-world code - I would have to always check "did_action( 'shutdown' )" to check that - the example is simplified, since I directly hooked it on "shutdown".
Also it might be a PHP error that caused the shutdown.

These examples are a bit contrived though - what was it you were trying to do?

They're the minimal reproducable.

The essence is: if doing_action returns true, we can assume that we can hook onto it and our code will get executed. However this is not the case.
Technically (as seen in the example) this can mean that some code is never executed on a hook, if another hook callback always exits before it (e.g. serve a file)

#7 @johnbillion
2 months ago

Right, but that's nothing to do with WordPress. If you have an extensible system and an extension can terminate the processing at any point, nothing is guaranteed to execute.

I guess I'm struggling to see what the actual issue is here. It still feels contrived.

Note: See TracTickets for help on using tickets.