Amp-wp: register_shutdown_function not being triggered

[maciejmackowiak]

Bug Description

When there is a PHP error during AMP sanitization register_shutdown_function is not being triggered.

Expected Behaviour

register_shutdown_function should be triggered.

Steps to reproduce

1. Copy this code to functions.php it will cause Fatal error: Uncaught Error: Call to undefined function not_a_function() on AMP pages.

register_shutdown_function(function(){
    global $wp;  
    $current_url = home_url(add_query_arg(array($_GET), $wp->request));
    error_log('register_shutdown_function triggered ' . $current_url);
});

add_action('amp_init', function(){
    class Fatal_Error_Sanitizer extends \AMP_Base_Sanitizer
    {
        public function sanitize()
        {
           not_a_function();
        }
    }
    add_filter(
        'amp_content_sanitizers',
        function ($sanitizers) {
            $sanitizers[ '\Fatal_Error_Sanitizer' ] = [];
            return $sanitizers;
        }
    );

});

1. Go to any non-AMP page.
2. View error log and you will see register_shutdown_function triggered {current non-AMP url}.
3. Go to any AMP page.
4. View error log register_shutdown_function triggered {current AMP url} will not be there.

Additional context

• WordPress version: 5.3
• Plugin version: 1.4.1
• Gutenberg plugin version (if applicable):
• AMP plugin template mode: Transitional
• PHP version: 7.3
• OS: any
• Browser: any
• Device: any

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

[archon810]

cc @dcramer @Jean85 @ste93cry because it affects Sentry when used with AMP.

[Jean85]

Sentry (and probably any tool that reports errors) relies on register_shutdown_function, so that's a normal consequence of this bug. Maybe there's a die() somewhere?

[archon810]

@westonruter Any ideas?

[westonruter]

I don't think there's anything specific to the AMP plugin going on here. I think it's just how PHP works. The sanitizers are running during an output buffer callback which runs before the shutdown functions run, and apparently if a fatal error occurs during either an output buffer callback or a shutdown handler, then anything after will not run. Consider this code running on a non-AMP page:

register_shutdown_function(function(){
    error_log('1️⃣ First shutdown function: ' . $_SERVER['REQUEST_URI']);
    if ( isset( $_GET['1'] ) ) {
        not_a_function_1();
    }
});

register_shutdown_function(function(){
    error_log('2️⃣ Second shutdown function: ' . $_SERVER['REQUEST_URI']);
    if ( isset( $_GET['2'] ) ) {
        not_a_function_2();
    }
});

ob_start( function( $content ) {
    error_log( '3️⃣ Output buffer running callback: ' . $_SERVER['REQUEST_URI'] );
    if ( isset( $_GET['3'] ) ) {
        not_a_function_3();
    }
    return $content;
} );

If you navigate to /?3 the only error_log() call executed is:

3️⃣ Output buffer running callback: /?3

So none of the shutdown handlers are run. If you ask for the error to happen in the first shutdown handler, then the error_log() in the second shutdown handler is not run. For example, if you navigate to /?1 the only error_log() calls executed are:

3️⃣ Output buffer running callback: /?1
1️⃣ First shutdown function: /?1

The 2️⃣ is not executed.

[archon810]

Do you think it's a bug in PHP then, or expected behavior?

[westonruter]

Yeah, I'm not sure. If a fatal error ocurrs in one shutdown handler then I suppose it makes sense it would prevent other shutdown handlers from running.

[schlessera]

I think this is due to the following change with PHP 7.0+:

As the E_ERROR is turned into an E_RECOVERABLE_ERROR, it doesn't trigger the shutdown handler. However, at the end of the execution, any E_RECOVEREABLE_ERROR that was not caught will be turned into a fatal error.

To catch the E_RECOVERABLE_ERROR, you would need a regular error handler, instead of a shutdown function.

@westonruter Is this something we want to look into to match the same behavior inside as outside of the output buffer callback?

[schlessera]

Oh, nevermind, I think I misunderstand the above text. I'll run some tests to verify the actual behavior.

[archon810]

Should we involve PHP devs in this, maybe file a ticket with them? This seems like a pretty major issue if it can't be done.

[Jean85]

At the moment, those are handled at https://bugs.php.net/

[archon810]

Found this open bug that is exactly what we need, so I'll add some more info there and we'll see if PHP devs address it: https://bugs.php.net/bug.php?id=78565.

Also this one seems related: https://bugs.php.net/bug.php?id=78495.

[westonruter]

Closing since there doesn't seem to be anything we can do without a change in PHP.