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

Unable to disable plugin from theme #27

Closed
fiskhandlarn opened this issue Apr 3, 2018 · 23 comments
Closed

Unable to disable plugin from theme #27

fiskhandlarn opened this issue Apr 3, 2018 · 23 comments
Labels
feature request Request for a new feature

Comments

@fiskhandlarn
Copy link
Contributor

When developing locally I would like to use PHP's native error handler instead of Bugsnag's, but still have the same code base/database for my local and live server (and not being forced to manually deactivate the plugin for different servers).

ATM it's impossible to override with the bugsnag_set_error_and_exception_handlers filter from within functions.php (since it's applied directly when the plugin loads (which is before the theme)).

Expected behavior

add_filter('bugsnag_set_error_and_exception_handlers', function() {
    return false;
});

should disable this plugin's custom error handlers.

Observed behavior

Nothing is happening with the above filter present in my theme's functions.php.

Steps to reproduce

See above.

Version

1.3.0

Additional information

If I rewrite the Bugsnag_Wordpress constructor so that activateBugsnag() is called on init:

        add_action('init', array($this, 'activateBugsnag'));

and change activateBugsnag() to public, the error handlers are indeed disabled, but I get this warning:

Warning: strpos(): Empty needle in plugins\bugsnag\bugsnag-php\Error.php on line 386

The same warning appears without the modified class if I run restore_error_handler(); in my functions.php.

@Cawllec
Copy link
Contributor

Cawllec commented Apr 5, 2018

Hi @fiskhandlarn, from some preliminary tests it seems that after adding your filter the Bugsnag exception and error handlers are no-longer being set. Are you saying this is not what you are seeing?

@fiskhandlarn
Copy link
Contributor Author

fiskhandlarn commented Apr 6, 2018

@Cawllec They are not disabled for me in version 1.3.0. If I rewrite the plugin so that the filter is being run when set in functions.php I still get the warnings in Error.php. What version are you using?

@Cawllec
Copy link
Contributor

Cawllec commented Apr 6, 2018

This is with 1.3.0 as well. I'll have a further look into what you're reporting and get back to you.

@fiskhandlarn
Copy link
Contributor Author

@Cawllec Strange. The only occurrence of bugsnag_set_error_and_exception_handlers I can find is this line in bugsnag.php:

            $set_error_and_exception_handlers = apply_filters('bugsnag_set_error_and_exception_handlers', true);

Which is called directly when the plugin is loaded, which happens before functions.php is even loaded.

Did you put your add_filter() call in your theme's functions.php?

@Cawllec
Copy link
Contributor

Cawllec commented Apr 6, 2018

I did.

@fiskhandlarn
Copy link
Contributor Author

@Cawllec When/where is the bugsnag_set_error_and_exception_handlers filter applied in your bugsnag.php?

For me it looks like this:

Line 314 (run when the plugin is included):
$bugsnagWordpress = new Bugsnag_Wordpress();

Line 33 (inside __construct()):
$this->activateBugsnag();

Line 70 (inside activateBugsnag()):
$this->constructBugsnag();

Line 88 (inside constructBugsnag()):
$set_error_and_exception_handlers = apply_filters('bugsnag_set_error_and_exception_handlers', true);

@Cawllec
Copy link
Contributor

Cawllec commented Apr 11, 2018

So my path is going:
314: $bugsnagWordpress = new Bugsnag_Wordpress();
33: $this->activateBugsnag();
70: $this->constructBugsnag();
88: $set_error_and_exception_handlers = apply_filters('bugsnag_set_error_and_exception_handlers', true);
Similar to yours.

As for the filter, at the bottom of functions.php I've added:

add_filter('bugsnag_set_error_and_exception_handlers', function() {
    return true;
});

@fiskhandlarn
Copy link
Contributor Author

@Cawllec Well yes, bugsnag's error handling is enabled with your code. But not because you're adding the filter in your functions.php, but because this is the default behaviour; as seen in bugsnag.php here:

$set_error_and_exception_handlers = apply_filters('bugsnag_set_error_and_exception_handlers', true);

As you can see, $set_error_and_exception_handlers is set to true by default.

What I'm asking for is the abilitiy to disable bugsnag's error handlers (as specified in the subject of this issue). What happens when you instead use this code in your functions.php?

add_filter('bugsnag_set_error_and_exception_handlers', function() {
    return false;
});

(Please notice the return false above instead of your return true.)

@Cawllec
Copy link
Contributor

Cawllec commented Apr 12, 2018

Sorry, I meant return false. They are being disabled with the return false in the filter.

@fiskhandlarn
Copy link
Contributor Author

@Cawllec Ah, alright :)

But I cannot for the life of me understand why your filter works and mine don't. The only way I can think of is if your functions.php is being loaded before bugsnag.php. Can you please confirm whether this is the case? (By echoing something at the top of both files for example.)

And maybe you can dump $set_error_and_exception_handlers just after line 88 with your false-filter active?

@Cawllec
Copy link
Contributor

Cawllec commented Apr 12, 2018

So with a print_r at the top of functions.php and bugsnag.php, and with the

add_filter('bugsnag_set_error_and_exception_handlers', function() {
    return false;
});

and a var_dump on $set_error_and_exception_handlers the output I get is:

Loaded functions.php
Loaded bugsnag.php
bool(false)

(with newlines added by me).

So it seems that your Bugsnag.php may be being loaded prematurely? What wordpress version are you running?

@fiskhandlarn
Copy link
Contributor Author

@Cawllec It was 4.9.4, but I've updated to 4.9.5 now (with the same behaviour).

I haven't seen that loading order before. https://wordpress.stackexchange.com/questions/26537/between-functions-php-widgets-and-plugins-which-is-loaded-first/26622#26622 also states that plugins are loaded before the theme.

Are you running "vanilla" WordPress?

@Cawllec
Copy link
Contributor

Cawllec commented Apr 12, 2018

I'm using the Wordpress 4.9.4 docker image , running on PHP 7.2. It should be as unmodified as possible, I'll check and see if the image has any differences.

@fiskhandlarn
Copy link
Contributor Author

@Cawllec Please do :)

If your WordPress files look like this:
https://github.com/WordPress/WordPress/blob/master/wp-settings.php#L304
https://github.com/WordPress/WordPress/blob/master/wp-settings.php#L433

... the theme should be loaded after all plugins.

Just to rule out any confusion, you are using the filter in the functions.php in a theme, right?

@Cawllec
Copy link
Contributor

Cawllec commented Apr 12, 2018

I absolutely was not. You're quite right about loading order - bugsnag.php loads before theme/functions.php meaning the add_filter function has no effect there.

I misread your point very early on, sorry this has been a bit more roundabout than it should have been.
I understand your issue now, I'll create a ticket to have a look into the best way for us to solve this issue.

  • @GrahamCampbell, do you have any input on potential fixes/solutions here?

@fiskhandlarn
Copy link
Contributor Author

@Cawllec No worries, I'm only happy we understand each other now. :)

A solution for actually make the filter usable is to call activateBugsnag in a later stage ('init' for example). But then bugsnag wouldn't report any bugs happening before then. The best solution would therefore probably be to turn on the error handling as soon as the plugin loads and then remove it if requested later.

But, I have no idea how to fix the plugin from causing these warnings when the error handlers once are disabled:

Warning: strpos(): Empty needle in plugins\bugsnag\bugsnag-php\Error.php on line 386

@Cawllec Cawllec added bug Confirmed bug feature request Request for a new feature and removed bug Confirmed bug labels Apr 16, 2018
@fiskhandlarn
Copy link
Contributor Author

fiskhandlarn commented May 31, 2018

@Cawllec @GrahamCampbell Any news on this? :)

@Cawllec
Copy link
Contributor

Cawllec commented Jun 11, 2018

Hi @fiskhandlarn, there's some higher priority work going on at the moment, but this, and more of a general update to this notifier, are in the pipeline, and I will update you when I know more.

@Cawllec
Copy link
Contributor

Cawllec commented Jan 29, 2019

Hi @fiskhandlarn, it's been a while but I've had a chance to look into this issue again. The strpos issue you are seeing seems to be related to the filters that've been setup. Are you setting any filters in your configuration?

@Cawllec
Copy link
Contributor

Cawllec commented Jan 29, 2019

I'm going to close this issue for now, but if the issue persists please get in touch.

@Cawllec Cawllec closed this as completed Jan 29, 2019
@fiskhandlarn
Copy link
Contributor Author

@Cawllec The strpos issue isn't really the real issue IMO. The original issue (that the bugsnag_set_error_and_exception_handlers filter is unusable) still persists (as confirmed by you in #27 (comment)). Please reopen.

@Cawllec
Copy link
Contributor

Cawllec commented Jan 30, 2019

The ticket to fix this is being prioritised, but will likely be included with other notifier improvements. The solution as it stands is to use restore_error_handler and restore_exception_handler conditionally and avoid using blank string filters '' so the strpos warning isn't flagged up.

As work on the notifier improvements are implemented this thread will be brought back in.

@fiskhandlarn
Copy link
Contributor Author

@Cawllec Alright, I will use restore_error_handler and restore_exception_handler.

FYI: if the "bugsnag_filterfields" option isn't set in the database $this->filterFields (in Bugsnag_Wordpress class) will be set to false which leads to the strpos warning. Bugsnag_Wordpress::filterFields() doesn't account for this. Although, I can now see that this happens when our theme sets "bugsnag_api_key" and "bugsnag_notify_severities" options in our custom install.php file without setting the "bugsnag_filterfields" option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for a new feature
Projects
None yet
Development

No branches or pull requests

2 participants