Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error handler improvements #600

Merged

Conversation

imjoehaines
Copy link
Contributor

@imjoehaines imjoehaines commented Sep 14, 2020

Goal

This PR adds a number of improvements to the error handler:

  1. Report new exceptions raised by the previous error handler
  2. Allow the default PHP exception handler to run if the previous handler wants it to (by re-raising the exception that's being handled)
  3. Avoid double reporting exceptions if the previous handler re-raises them (currently this will report once in the exception handler and once in the shutdown handler)

Because of these improvements we've also decided to always call the previous handlers, regardless of if register or registerWithPrevious is used

It's still possible to prevent Bugsnag from calling the previous handlers like this:

// Assuming $client is an instance of \Bugsnag\Client
$handler = new \Bugsnag\Handler($client);

$handler->registerErrorHandler(false);
$handler->registerExceptionHandler(false);
$handler->registerShutdownHandler();

Or the "chain" of error/exception handlers can be broken by creating handlers that do nothing:

set_error_handler(function () {});
set_exception_handler(function () {});

\Bugsnag\Handler::register();

Design

Some of the changes are a bit unintuitive, but the PHPT tests should show why they are necessary. The technique of re-throwing the exception inside the exception handler especially, but it's important to solve issues like #523 — this PR doesn't entirely fix that issue as we also need to re-raise exceptions when there is no previous handler, which will be done separately

Testing

Mostly tested through the PHPT tests and manually

The unit test has been re-written to use much less global mocking by setting/restoring error/exception handlers in the tests (one test still has a mocked global function). Unfortunately git tried too hard to diff the old & new file, so the diff is useless for reviewing ☹️

@imjoehaines imjoehaines changed the title Always call previous handlers Error handler improvements Sep 14, 2020
@imjoehaines imjoehaines changed the base branch from test-improvements to error-handler-improvements-base September 14, 2020 15:26
These tests prove it's still possible to avoid calling the previous
handlers, if that's desired
@@ -1,17 +1,14 @@
--TEST--
Bugsnag\Handler should call the previous exception handler

TODO this is currently reported twice! Once in the exceptionhandler and once on shutdown
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new enableShutdownHandler flag fixes this bug ✨

@imjoehaines imjoehaines marked this pull request as ready for review September 15, 2020 08:45
@imjoehaines imjoehaines merged commit 367376d into error-handler-improvements-base Sep 15, 2020
@imjoehaines imjoehaines deleted the always-call-previous-handlers branch September 15, 2020 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants