Call previously defined exception and error handlers #395

mterwill · 2017-08-02T21:29:34Z

This library overwrites previously set user exception and error handlers. Both set_error_handler and set_exception_handler will return callables for the previously defined handlers if they were set. Switch the library's behavior to respect this.

Closes #392

PHPUnit's default behavior is to translate PHP errors into exceptions and raise those. Rely on this assumption in testErrorHandler. Ref: https://phpunit.de/manual/current/en/writing-tests-for-phpunit.html

Both set_exception_handler and set_error_handler return the values to which they were previously set. Be respectful and call these handlers as it may introduce unexpected behavior in the user's code and also provides no way to attach an error handler besides manually wrapping the bugsnag library. PHPUnit's default behavior is to translate PHP errors into exceptions and raise those. Rely on this assumption in our tests of the error handler. Ref: https://phpunit.de/manual/current/en/writing-tests-for-phpunit.html

mterwill · 2017-08-02T21:41:46Z

src/Handler.php

@@ -69,15 +137,23 @@ public function exceptionHandler($throwable)
     */
    public function errorHandler($errno, $errstr, $errfile = '', $errline = 0)
    {
-        if ($this->client->shouldIgnoreErrorCode($errno)) {
-            return;
-        }


FWIW, the interpreter interprets this null return value differently than the false you were returning below.

It is important to remember that the standard PHP error handler is completely bypassed for the error types specified by error_types unless the callback function returns FALSE.

http://php.net/manual/en/function.set-error-handler.php

We can verify this:

$ cat test-exits-null.php <?php set_error_handler( function ($errno, $errstr) { return; } ); trigger_error("Uh oh!", E_USER_WARNING); $ php test-exits-null.php $ # warning not outputted

$ cat test-exits-false.php <?php set_error_handler( function ($errno, $errstr) { return false; } ); trigger_error("Uh oh!", E_USER_WARNING); $ php test-exits-false.php PHP Warning: Uh oh! in /private/tmp/test-exits-false.php on line 9 Warning: Uh oh! in /private/tmp/test-exits-false.php on line 9

FWIW, the interpreter interprets this null return value differently than the false you were returning below.

Looks like a bug. Either our doc was wrong, and we meant bool|null, or we meant to return false.

@ExpectedException

Since this library needs to be compatible with older versions of PHP, CI tests run on two versions of PHPUnit. 4.8 doesn't have PHPUnit_Framework_TestCase::expectException, however both newer and older versions of PHPUnit provide support for the annotation @ExpectedException, so use that instead.

kattrali · 2017-08-08T19:08:54Z

src/Handler.php


-        return false;
+        if ($this->previousErrorHandler) {
+            call_user_func(


Should this call be returned?

kattrali · 2017-08-08T19:09:53Z

This works well. I left a comment for the one question I had, around returning the value of the previous handler, in the case it will return false.

GrahamCampbell · 2017-08-08T19:27:31Z

src/Handler.php

+     *
+     * @return void
+     */
+    public function registerErrorHandler($callPrevious = true)


Setting this default to true is a backwards compatibility breaker. It should be set to false by default I think?

Up to y'all. Setting the default to false will preserve the current broken behavior for new installations and create a bug that can lurk for a while.

This change will only break backwards compatibility people who

load their custom handler

call Bugsnag::register

re-register their custom handler on top of that, saving the values returned by set_x_handler and re-calling that

I'm guessing those who have already worked around this will have just called Bugsnag\Handler::xHandler directly, in which case previousXHandler will be null since register is never called.

I'd think calling the user's function twice is better than never. 🤷‍♂️

Setting the default to false will preserve the current broken behavior for new installations and create a bug that can lurk for a while.

I don't think the current behaviour is broken actually. The handler is intentionally replacing existing ones. It's a feature if we want to have one that doesn't behave like this.

GrahamCampbell · 2017-08-08T19:28:38Z

src/Handler.php

     *
     * @return static
     */
-    public static function register($client = null)
+    public static function register($client = null, $callPrevious = true)


We should be very careful adding new parameters to public or protected methods in PHP since it's technically a BC break. A way around this would be to define a new method called registerWithPrevious.

Not only that, but it's more readable. Boolean parameters can be confusing.

Why would optional parameters that preserve the existing behavior break backwards compatibility? [obviously this does not preserve existing behavior, but we can continue that discussion above]

I'm generally not the biggest fan of boolean flags either, but in this case I think removing it would either (a) add a lot of duplicated code or (b) add unnecessary complexity to the existing code.

Why would optional parameters that preserve the existing behavior break backwards compatibility? [obviously this does not preserve existing behavior, but we can continue that discussion above]

Because PHP allows you to call methods with too many parameters without errors, and this class is non-final so could have been extended with a different signature for the 2nd parameter. Given the current handler didn't support preserving the previous handler, I'm sure other people will have extended it in their projects to add this.

I'm generally not the biggest fan of boolean flags either, but in this case I think removing it would either (a) add a lot of duplicated code or (b) add unnecessary complexity to the existing code.

I'm only suggesting we don't have it in the register function. The others are fine. :)

GrahamCampbell · 2017-08-08T19:30:18Z

src/Handler.php

+
+    /**
+     * Register the bugsnag exception handler and save the previous one
+     * (if it exists) to call later.


Please ensure that "short descriptions" are less than 8 chars and don't overflow past one line.

GrahamCampbell · 2017-08-08T20:38:57Z

src/Handler.php

+    protected $previousErrorHandler;
+
+    /**
+     * The previously registered exception handler returned by the interpreter.


I think The previously registered exception handler. is probably fine, and similar for above. While the main implementation of the PHP language is an interpreter (after a compilation phase to an intermediate language), there exist other implementations of the language, so no reason to assume the runtime is actually an interpreter. Just something very minor. :P

This test should have failed when changing the default behavior not to call the previous error handler. Instead of returning false, which is the default from the Bugsnag handler, and thus proves nothing, return a sentinel value and assert based on that.

Since these are new methods expected to be invoked primarily by register or registerWithPrevious, make the boolean flag a required parameter. A user might wish to only register certain handlers themselves, so it makes sense to leave these public, but the behavior is different depending on the flag. The caller should be forced to make an explicit decision.

mterwill · 2017-08-08T21:27:18Z

This should be fully backwards compatible now, with a new Handler::registerWithPrevious method added to register bugsnag handlers and preserve previously registered exception handlers.

Your documentation page should probably be updated to include a note about this once it's released.

kattrali · 2017-08-08T21:40:30Z

Thanks @mterwill 🎉

GrahamCampbell

Looks pretty solid. 👍

Matthew Terwilliger added 2 commits August 2, 2017 17:25

Add test cases to show errant behavior

8f00f8b

PHPUnit's default behavior is to translate PHP errors into exceptions and raise those. Rely on this assumption in testErrorHandler. Ref: https://phpunit.de/manual/current/en/writing-tests-for-phpunit.html

mterwill force-pushed the call-previous-handlers branch from 566202f to 92d05e3 Compare August 2, 2017 21:30

mterwill commented Aug 2, 2017

View reviewed changes

kattrali reviewed Aug 8, 2017

View reviewed changes

Matthew Terwilliger added 2 commits August 8, 2017 15:25

Add test to check custom error handler value is returned

10f219e

Return value from previousErrorHandler

3174cb6

GrahamCampbell suggested changes Aug 8, 2017

View reviewed changes

Matthew Terwilliger added 2 commits August 8, 2017 15:52

Update phpdoc

6d2e9f7

Preserve register behavior; add registerWithPrevious

0b980f9

GrahamCampbell suggested changes Aug 8, 2017

View reviewed changes

Matthew Terwilliger added 2 commits August 8, 2017 16:42

mterwill force-pushed the call-previous-handlers branch from 2dacf25 to 8e0407a Compare August 8, 2017 20:42

Update docblocks

b09d2e8

GrahamCampbell approved these changes Aug 8, 2017

View reviewed changes

kattrali merged commit d277503 into bugsnag:master Aug 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call previously defined exception and error handlers #395

Call previously defined exception and error handlers #395

mterwill commented Aug 2, 2017

mterwill Aug 2, 2017

GrahamCampbell Aug 8, 2017

kattrali Aug 8, 2017

kattrali commented Aug 8, 2017

GrahamCampbell Aug 8, 2017

mterwill Aug 8, 2017

GrahamCampbell Aug 8, 2017

GrahamCampbell Aug 8, 2017

GrahamCampbell Aug 8, 2017

mterwill Aug 8, 2017

GrahamCampbell Aug 8, 2017

GrahamCampbell Aug 8, 2017

GrahamCampbell Aug 8, 2017

GrahamCampbell Aug 8, 2017

mterwill commented Aug 8, 2017

kattrali commented Aug 8, 2017

GrahamCampbell left a comment

Call previously defined exception and error handlers #395

Call previously defined exception and error handlers #395

Conversation

mterwill commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kattrali commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mterwill commented Aug 8, 2017

kattrali commented Aug 8, 2017

GrahamCampbell left a comment

Choose a reason for hiding this comment