[HttpKernel] Throw double-bounce exceptions #13767
Conversation
nicolas-grekas
commented
Feb 23, 2015
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | - |
| License | MIT |
| Doc PR | - |
| @@ -67,13 +68,13 @@ public function onKernelException(GetResponseForExceptionEvent $event) | |||
| try { | |||
| $response = $event->getKernel()->handle($request, HttpKernelInterface::SUB_REQUEST, true); | |||
| } catch (\Exception $e) { | |||
| $this->logException($exception, sprintf('Exception thrown when handling an exception (%s: %s)', get_class($e), $e->getMessage()), false); | |||
| $this->logException($e, sprintf('Exception thrown when handling an exception (%s: %s at %s line %s)', get_class($e), $e->getMessage(), $e->getFile(), $e->getLine()), false); | |||
There was a problem hiding this comment.
Would it be possible to add an optional $context argument to logException? That makes it possible to properly log in the 'exception' context. Besides of having a cleaner messages and cleaner code, it will be handled properly by monolog handlers.
There was a problem hiding this comment.
The context is already logged, see https://github.com/symfony/symfony/pull/13767/files#diff-3a012244a1c6d5c901c4f26d6b2ca299R102
There was a problem hiding this comment.
Is the parent exception logged properly? In that is not the case, I prefer to see a smaller message or log both exceptions.
There was a problem hiding this comment.
You should read the code... https://github.com/symfony/symfony/pull/13767/files#diff-3a012244a1c6d5c901c4f26d6b2ca299R53
There was a problem hiding this comment.
My bad, I missed the first one. I still stand for "I prefer to see a smaller message" though. All the information passed to the message is located in the context as well.
There was a problem hiding this comment.
What about something as simple as this? The context should log the Exception and the formatters will properly format the logs for you. In case of the HTML formatter, this will be the basic exception information; Exception Class, Message and Trace.
// message is up for improvement
$this->logException($e, 'An Exception has been thrown when handling another Exception.', false);There was a problem hiding this comment.
IMO, this would wrongly aggregate all same log lines, where they should not. An uncaught double exception is important to caught, and rare. I'd keep the msg asis.
There was a problem hiding this comment.
I'm not too familiar with logging, but isn't that what the context is for? You could always put the actual exception class in the message, more than that would make it unreadable clutter.
There was a problem hiding this comment.
I propose you do an other PR if you want, as that was not the prupose of this one (I did not change the message but for adding line+file info) :)
There was a problem hiding this comment.
Yes, I think this is starting to fall out of the scope of this PR.
|
Please note that this PR is not meant to be merged asis. The current code in 2.3 eats double-bounce exceptions, and this can be fixed by the added |
b892c6a to
bdf6dde
Compare
89ce962 to
c45ece9
Compare
c45ece9 to
0ebcf63
Compare
|
PR is now ready |
|
Thank you @nicolas-grekas. |
This PR was merged into the 2.3 branch. Discussion ---------- [HttpKernel] Throw double-bounce exceptions | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- 0ebcf63 [HttpKernel] Throw double-bounce exceptions