[2.0] Refactor the error and exception handlers #584

ste93cry · 2018-04-24T10:34:47Z

Q	A
Branch?	2.0
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
License	MIT

This PR is a huge refactoring of the error and exception handlers. I tried to make them as transparent as possible in regards to other handlers, so the error handler converts the errors to exceptions to be able to log them but never throws them unless necessary. The aim of my work is to fix the many bugs that we're experiencing in 1.x, in particular #579, #552, #391, #343. I'm currently tracking an issue with Zend Diactoros (zendframework/zend-diactoros#298) that prevents any error past the first one from being logged through custom error handlers after usage of the Stream class.

Jean85

It seems a good work for now! 👍

For test, mind that a try...catch would stop even failed assertions.

Jean85 · 2018-04-24T10:58:25Z

lib/Raven/Client.php

+    /**
+     * @var ErrorHandler The instance of the error handler
+     */
+    private $errorHandler;


Can you move this back up to reduce the diff?

I've decided to remove the property as the client does not necessarily need to know the instance of the error handler and so that there is no circular reference between them

Jean85 · 2018-04-24T11:02:30Z

lib/Raven/ErrorHandler.php

-        // This prevents duplicated exceptions in PHP 7.0+
-        if (PHP_VERSION_ID >= 70000 && E_ERROR === $type) {
-            return false;
+        if (!empty($error) && $error['type'] & (E_ERROR | E_PARSE | E_CORE_ERROR | E_CORE_WARNING | E_COMPILE_ERROR | E_COMPILE_WARNING)) {


Maybe you should wrap those two lines with a private method to give it a name?

Extracting the code that just checks whether the error is a fatal error does not makes much sense imho, and there would also be the cost of calling one more function

Jean85 · 2018-04-24T11:07:22Z

tests/ErrorHandlerTest.php

+     * @expectedException \UnexpectedValueException
+     * @expectedExceptionMessage The value of the $reservedMemorySize argument must be an integer greater than 0.
+     */
+    public function testConstructorThrowsWhenReservedMemorySizeIsWrong()


You're not using the data from the provider

Jean85 · 2018-05-03T13:36:12Z

lib/Raven/ErrorHandler.php

+        // If there is no exception handler that can run after the current one
+        // give back the exception to the native PHP handler
+        if (null === $this->previousExceptionHandler) {
+            throw $exception;


Doesn't re-throwing change the stack trace of the exception?

After a few tests it seems it doesn't

Jean85 · 2018-05-03T13:37:21Z

lib/Raven/ErrorHandler.php

+        $this->previousExceptionHandler = null;
+
+        try {
+            call_user_func($previousExceptionHandler, $exception);


$previousExceptionHandler($exception) is maybe slightly faster?

Jean85 · 2018-05-03T13:38:13Z

lib/Raven/ErrorHandler.php


-        return $this;
+        $this->handleException($previousExceptionHandlerException);


Is this loop-prone?

Yes it should be as the next time the function will be called $this->previousExceptionHandler will be null and after sending the exception to Sentry the exception itself will be rethrown

Jean85 · 2018-05-03T13:42:43Z

tests/phpt/fatal_error.phpt

+    $pendingRequests = Assert::getObjectAttribute($transport, 'pendingRequests');
+
+    Assert::assertEmpty($pendingRequests);
+});


Those asserts can trigger the autoloading. If this is done under PHP 5 inside an error handling context, it fails silently. You should add an echo at the end and expect it to be sure that all those assertions are executed correctly.

Jean85 · 2018-05-03T13:43:50Z

tests/phpt/out_of_memory.phpt

+$foo = str_repeat('x', 1024 * 1024 * 30);
+?>
+--EXPECTF--
+Fatal error: Allowed memory size of %d bytes exhausted (tried to allocate %d bytes) in %s on line %d


Shouldn't you add something to be sure that you really handled the error?

Done, I've added an echo statement after all the assertions whose existence is checked in the --EXPECTF-- section of the PHPT test

Jean85

LGTM now! 👍 Great work!

ste93cry · 2018-05-04T20:18:32Z

I still have to complete the refactor of the ErrorHandler for the breadcrumbs. Please don't merge this PR yet until I remove the WIP prefix

Jean85 · 2018-05-04T21:16:14Z

Sorry, I thought it was complete!

ste93cry · 2018-05-06T19:23:59Z

@dcramer @stayallive @Jean85 In 1.x there is an additional error handler that only registers a breadcrumb. Does it makes sense to leave that class which should duplicate all the logic of the new ErrorHandler (with all the code to make it working even with PHP bug 63206) or can we just drop the install_default_breadcrumb_handlers option and just always log the breadcrumb in case of error (which imho it makes sense)?

Jean85 · 2018-05-06T20:24:03Z

IMHO the option is indipentent from the practical implementation of the handler. I agree that we can reuse the same code to do that stuff, but I would leave them separate, so an end user would still be able to customize the breadcrumbs.

Maybe we can create two instances? Or two different child classes?

ste93cry · 2018-05-06T20:30:34Z

so an end user would still be able to customize the breadcrumbs.

Would it makes sense for an user to use only the error handler that registers the breadcrumbs and not the handler that captures the error itself?

Jean85 · 2018-05-06T20:32:43Z

No, obviously not, why?

ste93cry · 2018-05-06T20:36:12Z

Because then it would not make sense to have two error handlers, one for capturing the errors and one for registering the breadcrumb. Or at least, I don't see the sense behind this. Having two handlers means reserving memory for both of them (to handle fatal errors) and duplicating the logic for each of them. Also, I don't see why an user would want to not register the breadcrumbs but instead capture the errors

Jean85 · 2018-05-06T20:55:24Z

The scenario that I want to preserve is the possibility of replacing (or enhancing) the breadcrumb handler. If you collapse both handler this would not be possible anymore.

Drop the install_default_breadcrumb_handlers configuration option

… thrown

ste93cry · 2018-05-09T21:57:59Z

I updated the PR by refactoring the breadcrumb handler and it's ready for another round of code reviews. The zendframework/zend-diactoros#298 issue persists and if nothing changes before releasing 2.0 we will have to switch PSR-7 library or no errors after the first one will be logged and sent to the Sentry server

Jean85

Fantastic work!! 🎉

Jean85 · 2018-05-10T08:51:37Z

lib/Raven/AbstractErrorHandler.php

+
+        $errorAsException = new \ErrorException(self::ERROR_LEVELS_DESCRIPTION[$level] . ': ' . $message, 0, $level, $file, $line);
+
+        $backtrace = $errorAsException->getTrace();


This could be inlined

ste93cry · 2018-05-12T19:09:56Z

I fixed the fact that fatal errors were reported twice in PHP 7+ (yes, again...) and this should be the good time. Before merging, should I switch PSR-7 library in this PR or do you prefer to postpone it later?

Jean85 · 2018-05-13T15:20:29Z

I think that switching PSR-7 implementor should be done in a separate PSR, let's keep it simple!

ste93cry added this to the Release 2.0 milestone Apr 24, 2018

ste93cry requested review from stayallive, Jean85 and nokitakaze April 24, 2018 10:34

Jean85 reviewed Apr 24, 2018

View reviewed changes

Jean85 reviewed May 3, 2018

View reviewed changes

Jean85 mentioned this pull request May 4, 2018

[2.0] Road to the 2.0 release #596

Closed

15 tasks

Jean85 changed the title ~~[WIP] [2.0] Refactor the error and exception handlers~~ [2.0] Refactor the error and exception handlers May 4, 2018

Jean85 approved these changes May 4, 2018

View reviewed changes

Jean85 changed the title ~~[2.0] Refactor the error and exception handlers~~ [WIP] [2.0] Refactor the error and exception handlers May 4, 2018

ste93cry added 4 commits May 9, 2018 21:35

Refactor the error and exception handler and improve its unit tests

808c85d

Apply code review changes as requested

6865d1d

Drop the install_default_breadcrumb_handlers configuration option

90c12cf

Drop the install_default_breadcrumb_handlers configuration option

Refactor the error handler that records a breadcrumb when an error is…

70b42f9

… thrown

ste93cry requested a review from Jean85 May 9, 2018 20:21

Update the UPGRADE file

a6b1824

Jean85 approved these changes May 10, 2018

View reviewed changes

Prevent fatal errors from being reported twice in PHP 7+

a4a151b

ste93cry changed the title ~~[WIP] [2.0] Refactor the error and exception handlers~~ [2.0] Refactor the error and exception handlers May 13, 2018

ste93cry merged commit ec2ec1e into getsentry:2.0 May 13, 2018

ste93cry deleted the error-handler-refactoring branch May 13, 2018 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2.0] Refactor the error and exception handlers #584

[2.0] Refactor the error and exception handlers #584

ste93cry commented Apr 24, 2018 •

edited

Loading

Jean85 left a comment

Jean85 Apr 24, 2018

ste93cry Apr 24, 2018

Jean85 Apr 24, 2018

ste93cry Apr 25, 2018

Jean85 Apr 24, 2018

Jean85 May 3, 2018

ste93cry May 4, 2018

Jean85 May 3, 2018

Jean85 May 3, 2018

ste93cry May 3, 2018

Jean85 May 3, 2018

Jean85 May 3, 2018

ste93cry May 4, 2018

Jean85 left a comment

ste93cry commented May 4, 2018

Jean85 commented May 4, 2018

ste93cry commented May 6, 2018

Jean85 commented May 6, 2018

ste93cry commented May 6, 2018

Jean85 commented May 6, 2018

ste93cry commented May 6, 2018

Jean85 commented May 6, 2018

ste93cry commented May 9, 2018

Jean85 left a comment

Jean85 May 10, 2018

ste93cry commented May 12, 2018

Jean85 commented May 13, 2018


		return $this;
		$this->handleException($previousExceptionHandlerException);


		$errorAsException = new \ErrorException(self::ERROR_LEVELS_DESCRIPTION[$level] . ': ' . $message, 0, $level, $file, $line);

		$backtrace = $errorAsException->getTrace();

[2.0] Refactor the error and exception handlers #584

[2.0] Refactor the error and exception handlers #584

Conversation

ste93cry commented Apr 24, 2018 • edited Loading

Jean85 left a 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

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

Jean85 left a comment

Choose a reason for hiding this comment

ste93cry commented May 4, 2018

Jean85 commented May 4, 2018

ste93cry commented May 6, 2018

Jean85 commented May 6, 2018

ste93cry commented May 6, 2018

Jean85 commented May 6, 2018

ste93cry commented May 6, 2018

Jean85 commented May 6, 2018

ste93cry commented May 9, 2018

Jean85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ste93cry commented May 12, 2018

Jean85 commented May 13, 2018

ste93cry commented Apr 24, 2018 •

edited

Loading