[HttpKernel] Create responses for unhandled HttpExceptionInterface exceptions #26928
Conversation
| if ($event->hasResponse()) { | ||
| $response = $event->getResponse(); | ||
| } elseif ($e instanceof HttpExceptionInterface) { | ||
| $response = new Response($e->getMessage()); |
There was a problem hiding this comment.
Wondering... couldn't $e->getMessage() potentially expose something that is not meant to be displayed to a user/client ever?
Maybe better use Response::$statusTexts[$code]?
There was a problem hiding this comment.
When twig is installed, this is the message that is displayed (isn't it?), so I don't think there is anything harmful here.
There was a problem hiding this comment.
Hmm really? I thought with twig we only show the status_text?
|
@nicolas-grekas This is more elegant than #26138. |
| $response = $kernel->handle(new Request()); | ||
|
|
||
| $this->assertSame(404, $response->getStatusCode()); | ||
| $this->assertSame('404 Not Found', $response->getContent()); |
There was a problem hiding this comment.
As demonstrated by this test change, this changes the behavior.
There was a problem hiding this comment.
So, I would not do the change in 3.4.
|
Rebased on master, ready for 4.1 |
|
Can you add a note in the CHANGELOG? |
|
CHANGELOG updated |
| $response = $event->getResponse(); | ||
| } elseif ($e instanceof HttpExceptionInterface) { | ||
| $code = $e->getStatusCode(); | ||
| $response = new Response(isset(Response::$statusTexts[$code]) ? $code.' '.Response::$statusTexts[$code] : $code); |
There was a problem hiding this comment.
Would it make sense to do this only in non-debug mode? And keep the exception in debug mode?
There was a problem hiding this comment.
👍 for keeping them in debug
There was a problem hiding this comment.
Nevermind my prior comment. You already answered it in #26928 (comment). Please ignore. :-)
HttpKernel doesn't know about debug/non-debug. |
|
@nicolas-grekas wondering if the front controller really is the best place for this? Shouldn't it have as little logic as possible? In functional tests the front controller is not used for example 😕 |
|
Well, adding a listener is possible, nothing prevents it. |
I agree that this "fallback listener" should only set a response if there is none set yet (by any other listener with higher priority). But if TwigBundle is registered then it makes sense to remove this fallback listener? or not? Otherwise its just a no-op listener that is instantiated and called for no reason? |
|
We could remove it, but it should not be a requirement. And in fact, it would be a micro-optimization, since listeners are lazy and exceptions are not on the typical hot path. |
|
Let's continue on #26138 |
Alternative to #26138