Reporting unhandled exceptions breaks error page handling

[thedotedge]

Expected behavior

When uncaught exception is thrown, bugsnag intercepts it without possibility to capture it and show custom error page to the user.

Steps to reproduce

<?php
// some code that emits E_NOTICE
// ...

register_shutdown_function(function () use ($bugsnag) {
        if ($error = error_get_last()) {
            if (in_array($error['type'], [E_ERROR, E_PARSE, E_COMPILE_ERROR, E_CORE_ERROR, E_USER_ERROR])) {
                http_response_code(500);
                exit();
            }
        }
    });

$bugsnag = Bugsnag\Client::make($GLOBALS['config']['bugsnag']['api_key']);
Bugsnag\Handler::register($bugsnag);

throw new \RuntimeException('Triggered error 500');

1. When bugsnag handler is not registered, response has status code 500 error_get_last() correctly returns E_ERROR (intended behaviour)
2. When bugsnag handler is registered, error_get_last() does not return E_ERROR from RuntimeException anymore, rather some E_NOTICE from prior code.

Version

bugsnag-php v3.12.1
php-fpm 7.2.2

[snmaynard]

I believe you can call registerWithPrevious as in https://github.com/bugsnag/bugsnag-php/blob/master/src/Handler.php#L51

We should improve the documentation here at a minimum. But it feels like that should be the default behaviour. cc @kattrali @Cawllec

[Cawllec]

Yes, you can call registerWithPrevious in order to ensure the previous handlers aren't overwritten. I'll update the docs as I can't see where we mention this.

[thedotedge]

I have tried that, however, https://github.com/bugsnag/bugsnag-php/blob/master/src/Handler.php#L71 overrides shutdown handler anyway. While error and exception handlers preserve original handler, shutdown is overridden.

[thedotedge]

Furthermore, bugsnag somehow messes up response code in case of fatal error:

This returns status code 500:

<?php
ini_set('display_errors', 0);
throw new \RuntimeException('Triggered error 500'); 

This returns status code 200:

<?php
ini_set('display_errors', 0);
$bugsnag = Bugsnag\Client::make($GLOBALS['config']['bugsnag']['api_key']);
Bugsnag\Handler::register($bugsnag);
throw new \RuntimeException('Triggered error 500'); 

[Cawllec]

I'm not in the office the rest of this week, but will get to this as soon as possible once I'm back.

[thedotedge]

Any updates @Cawllec?

[Cawllec]

Hey @thedotedge, I'm back. So, according to the docs, register_shutdown_function doesn't return a callable as it will execute all of the registered shutdown functions unless exit is called in one of them, something we don't do.

I'll investigate the incorrect exit code, and see if I can work out what's going on there.

[Cawllec]

I've checked out the exit status issue, and it's actually a PHP quirk - when an exception handler is registered PHP will exit with 0 as its exit status. You can override this behaviour by adding your own exception handler that sets the exit code with exit($code), but it's not behaviour specific to Bugsnag, rather to any script that utilises an exception handler.

[thedotedge]

Thanks @Cawllec. Did you have a look at simplified steps to reproduce above? Registering bugsnag handler breaks http return response code for me in case of uncaught exception.

[thedotedge]

Ping @Cawllec

[Cawllec]

Hi, sorry @thedotedge, I meant that you get a 200 status code with an exception handler when running in the browser, and a 0 when running in the console. You should be able to change the response code using the http_response_code method.

[thedotedge]

Thanks @Cawllec, given that this script is run by php-fpm in the browser, where does one set http_response_code? And should not that be done by bugsnag exception handler?

<?php
ini_set('display_errors', 0);
$bugsnag = Bugsnag\Client::make($GLOBALS['config']['bugsnag']['api_key']);
Bugsnag\Handler::register($bugsnag);
throw new \RuntimeException('Triggered error 500'); 

[Cawllec]

I'm unsure of how the behaviour would change between vanilla PHP and PHP-FPM, but you if you want to have a callback run after the Bugsnag exception (or error) handler you need to register it before you register the Bugsnag handlers, and you'd need to register them using the registerWithPrevious method.
For example, your original code snippet would look like:

<?php
// some code that emits E_NOTICE
// ...

register_shutdown_function(function () use ($bugsnag) {
        if ($error = error_get_last()) {
            if (in_array($error['type'], [E_ERROR, E_PARSE, E_COMPILE_ERROR, E_CORE_ERROR, E_USER_ERROR])) {
                http_response_code(500);
                exit();
            }
        }
    });

# When the bugsnag exception handler is added with `registerWithPrevious` it will keep this callable and execute it after notifying.
set_exception_handler(function($exception) {
    # Set http_response_code to 500, or any other response tasks post exception-handling
    http_response_code(500);
});

$bugsnag = Bugsnag\Client::make($GLOBALS['config']['bugsnag']['api_key']);

# Note - This was Bugsnag\Handler::register, now it's Bugsnag\Handler::registerWithPrevious
Bugsnag\Handler::registerWithPrevious($bugsnag);

throw new \RuntimeException('Triggered error 500');

Try this out and let me know if the behaviour is as expected.

[thedotedge]

Thanks @Cawllec, the code above does not seem to work, since
if (in_array($error['type'], [E_ERROR, E_PARSE, E_COMPILE_ERROR, E_CORE_ERROR, E_USER_ERROR])) is never entered ($error['type'] would be one of the notices from the code before exception is thrown, not the error). Either way, why does Bugsnag\Handler::register($bugsnag); change default behaviour?

[Cawllec]

Sorry it's taken me a while to reply, I've been on leave.

Bugsnag\Handler::register($bugsnag) will alter default behaviour, but only in the way that registering any callbacks using register_shutdown_function, set_exception_handler, and set_error_handler will. This includes exceptions being caught by the handler registered in set_exception_handler not being retrievable via error_get_last.

By registering your own exception handler using set_exception_handler before calling Bugnsag\Handler::registerWithPrevious($bugsnag), you can register a handler that will run after the Bugsnag handler, that has still has the original exception. You should be able to do error-redirection and processing in this function, rather than a callback registered using register_shutdown_function.

[TheMY3]

@Cawllec has same error, tryied to use set_exception_handler and registerWithPrevious method, but it steel not working.
set_exception_handler can handle original exception, but stacktrace is empty.
I always try to do something with register_shutdown_function, I see original exception in breadcrumbs inside Client object but can't get it, function error_get_last return previous error, not my exception.

[thedotedge]

Hi! Have you been able to use the steps to reproduce to confirm the issue?

[Cawllec]

Hi, like I say, Bugsnag is not changing default PHP behaviour. This is an effect of using the PHP set_exception_handler and set_error_handler methods. Any PHP error handling library would act in the same way. I'll see about producing an example that may help with your issue, but there isn't anything we can change in our notifier.

[imjoehaines]

This should now be fixed in v3.23.1