-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[ErrorCatcher][ExceptionHandler] Handle \Throwable #31694
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
[ErrorCatcher][ExceptionHandler] Handle \Throwable #31694
Conversation
@@ -85,7 +85,7 @@ public function testHeaders() | |||
|
|||
ob_start(); | |||
$handler->sendPhpResponse(new MethodNotAllowedHttpException(['POST'])); | |||
$response = ob_get_clean(); | |||
ob_get_clean(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused var
}); | ||
|
||
$handler->handle($exception); | ||
$handler->handle(new \Exception()); | ||
ob_end_flush(); // Necessary because of this PHP bug : https://bugs.php.net/bug.php?id=76563 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lost more than 4 hours on this. If someone wants to look at it, just put this in a controller :
$handler = ExceptionHandler::register();
$handler->setHandler(function ($e) {
echo 'foo';
});
$handler->handle(new \RuntimeException('bar'));
dump(ob_get_clean()); die();
It will dump the full content (default error handler content + "foo"). If you comment the dump, only "foo" is actually rendered.
bf577d0
to
2653752
Compare
@nicolas-grekas @yceruto Just rebased this on master to apply the changes on the new Since it touches 2 components, should I split this ? |
2653752
to
64d02cb
Compare
… the ErrorCatcher component (fancyweb) This PR was merged into the 5.0-dev branch. Discussion ---------- [Debug] Remove all deprecated classes that were moved to the ErrorCatcher component | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | - #31694 (comment) Commits ------- 8ce3a89 [Debug] Remove all deprecated classes that were moved to the ErrorCatcher component
The plan is to remove ExceptionHandler from ErrorHandler if you want to have a look. |
…(fancyweb) This PR was merged into the 3.4 branch. Discussion ---------- [Debug][ExceptionHandler] Add tests for custom handlers | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - In #31694 I mixed many things but the whole PR was closed. I wrote some tests for custom handlers + the handle tests don't use mock anymore I think they are useful even if the `ExceptionHandler` will disappear in the new component because it will still exists in 4.4 for the next 3 years. Commits ------- c53e253 [Debug][ExceptionHandler] Add tests for custom handlers
…(fancyweb) This PR was merged into the 3.4 branch. Discussion ---------- [Debug][ExceptionHandler] Add tests for custom handlers | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - In symfony/symfony#31694 I mixed many things but the whole PR was closed. I wrote some tests for custom handlers + the handle tests don't use mock anymore I think they are useful even if the `ExceptionHandler` will disappear in the new component because it will still exists in 4.4 for the next 3 years. Commits ------- c53e25332a [Debug][ExceptionHandler] Add tests for custom handlers
Follow up of #28954
I also removed an unused test file + added a test for the new behavior + added 3 tests when you use custom handlers.