bugsnag / bugsnag-wordpress

BugSnag error monitoring for WordPress sites
http://wordpress.org/plugins/bugsnag/
GNU General Public License v2.0
20 stars 15 forks source link

Move exception handler to init action #44

Closed jonasemde closed 4 years ago

jonasemde commented 4 years ago

Goal

As mention here https://github.com/bugsnag/bugsnag-wordpress/issues/41 this plugin prevents all error output. It removes PHP errors from being displayed on screen, even with WP_DEBUG turned on.

The current WordPress filter "bugsnag_set_error_and_exception_handlers" is useless. The function is called on construct and could not be overwritten as planned.

Design

Now the exception handler is set on WordPress init.

Tests

manual

Linked issues

Fixes #41

imjoehaines commented 4 years ago

Hey @jonasemde, thanks for the PR!

We're concerned about delaying the error and exception handlers here as it will have an impact on the reporting of any errors that might occur between initial construction and when the initActions method is called by WordPress

I think there's an alternative option here — you can add a filter for the bugsnag_set_error_and_exception_handlers option in the main functions.php file (i.e. wp-includes/functions.php) rather than in the theme specific functions.php file. Given this is an option primarily for development, this seems like a fine solution

I've tested this by adding the following code to wp-includes/functions.php and it correctly disables the Bugsnag error and exception handlers, allowing the default PHP behaviour to show the error in the browser:

add_filter('bugsnag_set_error_and_exception_handlers', function () {
    return false;
});
jonasemde commented 4 years ago

I'am not fine with editing WordPress core files. This is a bad practice and would removed with every update or deploy.

You mentioned the loss of error reporting until the init function runs.

As mention here: https://github.com/bgsnag/bugsnag-wordpress/blob/master/bugsnag.php#L86 the errors are still going to be reported to Bugsnag!?! Or did I miss something?

The WordPress configuration works mainly via PHP constants. Alternatively, it would be possible to use a constant instead of the filter.

TIA

imjoehaines commented 4 years ago

You mentioned the loss of error reporting until the init function runs.

As mention here: https://github.com/bgsnag/bugsnag-wordpress/blob/master/bugsnag.php#L86 the errors are still going to be reported to Bugsnag!?! Or did I miss something?

The errors are still reported due to the shutdown function we register, but we lose some context about the error because we can't tell at that point if we're dealing specifically with an exception

So the errors are still visible in Bugsnag, but the user experience isn't as good because we can't extract as much information as we would with the exception handler

The WordPress configuration works mainly via PHP constants. Alternatively, it would be possible to use a constant instead of the filter.

I think a constant makes a lot of sense for this use case, thanks for the suggestion!

imjoehaines commented 4 years ago

@jonasemde I've opened https://github.com/bugsnag/bugsnag-wordpress/pull/45 which adds a constant to allow the exception handlers to be disabled