[HttpKernel] Catch HttpExceptions when templating is not installed #26138

junowilderness · 2018-02-11T01:01:29Z

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	?
Deprecations?	no
Tests pass?	?
Fixed tickets	#25844
License	MIT
Doc PR	symfony/symfony-docs#...

Test manually
Check for BC breaks
Needs tests

dmaicher · 2018-02-11T12:54:04Z

src/Symfony/Component/HttpKernel/EventListener/HttpExceptionListener.php

+
+    public function onKernelException(GetResponseForExceptionEvent $event)
+    {
+        if ($this->container->has('twig')) {


instead we could remove the service definition within a compiler pass? So it would have zero runtime overhead if twig is available

@dmaicher Thank you for reviewing. Yes, that's a good idea. Do you feel TwigBundle or HttpKernel should have that compiler pass logic? I think TwigBundle but I'd like to hear your opinion.

I would create the compiler pass inside HttpKernel and use it accordingly in the FrameworkBundle like done for example here: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php#L31

junowilderness · 2018-02-26T13:54:45Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/ExceptionListenerPass.php

@@ -36,5 +36,9 @@ public function process(ContainerBuilder $container)
                $container->removeDefinition('twig.exception_listener');
            }
        }
+
+        if ($container->hasDefinition('twig.exception_listener')) {


@dmaicher What do you think of putting the responsibility in the existing TwigBundle compiler pass?

junowilderness · 2018-02-26T19:30:15Z

I don't understand enough about SF tests to know why tests are failing.

nicolas-grekas · 2018-04-14T16:05:05Z

I'd like to propose an alternative, see #26928.

junowilderness · 2018-04-16T15:50:59Z

#26928 is simpler.

nicolas-grekas · 2018-04-23T08:35:51Z

Reopening as discussed in #26928

nicolas-grekas · 2018-04-23T08:37:07Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml

@@ -67,6 +67,12 @@
            <argument type="service" id="router" on-invalid="ignore" />
        </service>

+        <service id="http_exception_listener" class="Symfony\Component\HttpKernel\EventListener\HttpExceptionListener">
+            <tag name="kernel.event_subscriber" />
+            <argument type="service" id="service_container" />


the argument is never used, it should be removed

nicolas-grekas · 2018-04-23T08:37:33Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml

+        <service id="http_exception_listener" class="Symfony\Component\HttpKernel\EventListener\HttpExceptionListener">
+            <tag name="kernel.event_subscriber" />
+            <argument type="service" id="service_container" />
+            <argument>%kernel.debug%</argument>


I would not make this depend on the debug parameter, let's remove it.

nicolas-grekas · 2018-04-23T08:38:19Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/ExceptionListenerPass.php

@@ -36,5 +36,9 @@ public function process(ContainerBuilder $container)
                $container->removeDefinition('twig.exception_listener');
            }
        }
+
+        if ($container->hasDefinition('twig.exception_listener')) {
+            $container->removeDefinition('http_exception_listener');


I'd suggest to not conditionally unregister the listener, let it be, that's not a measurable overhead anyway

nicolas-grekas · 2018-04-23T08:38:46Z

src/Symfony/Component/HttpKernel/EventListener/HttpExceptionListener.php

+    protected $container;
+    protected $debug;
+
+    public function __construct(ContainerInterface $container, $debug = false)


can be removed (alongside with properties) since we're not gonna use it

nicolas-grekas · 2018-04-23T08:40:29Z

src/Symfony/Component/HttpKernel/EventListener/HttpExceptionListener.php

+
+    public function onKernelException(GetResponseForExceptionEvent $event)
+    {
+        if (!$this->debug) {


I'd suggest an alternative logic in the method:
when the event already contains a response, this listener should just do nothing.
otherwise and if possible, it would be great if creating the response could be delegated to then ExceptionHandler (the Debug component is already a required dependency)

nicolas-grekas

(see previous review)

nicolas-grekas · 2018-04-23T08:51:10Z

@cilefen up to finish this one? Otherwise no pb, please advise and we'll take over.

junowilderness · 2018-04-23T10:49:05Z

I will look look at it today.

junowilderness · 2018-04-23T14:11:06Z

src/Symfony/Component/HttpKernel/EventListener/HttpExceptionListener.php

+        $flattenException = FlattenException::create($event->getException());
+        $handler = new ExceptionHandler(false);
+        $response = new Response($handler->getContent($flattenException));
+        $event->setResponse($response);


@nicolas-grekas This does not actually work as expected in the dev environment. In fact I do no understand how to detect the environment without injecting it.

@nicolas-grekas ... and indeed I may be way "off the mark" in terms of what you wrote about delegating to the ExceptionHandler.

Actually, I was wrong, kernel.debug may be required to populate the first argument of ExceptionHandler's constructor.
About the implementation, I suggest to borrow from ExceptionHandler::sendPhpResponse()

@nicolas-grekas hmm but should this new listener really stream/echo the response to the client directly? I think it should just set the response on the event and nothing else?

Edit: I mean its just this line? https://github.com/symfony/symfony/pull/26928/files#diff-3d3b8968ae2d94a9aab7078dec41e607R228

We should not call sendPhpResponse but take inspiration from it. Eg header management is missing right now .

Ah ok got it 👍 yes makes sense to use HttpExceptionInterface::getHeaders() also

junowilderness · 2018-04-23T18:05:54Z

src/Symfony/Component/HttpKernel/EventListener/HttpExceptionListener.php

+
+    public function onKernelException(GetResponseForExceptionEvent $event)
+    {
+        if ($event->hasResponse()) {


Also I am not sure if this evaluates to true often in the production environment without twig installed.

It should evaluate to false unless one handled the exception, either via twigbundle or any other mean, isn't it?

@nicolas-grekas The issue at hand is that (I think) there is never a response on $event when twig is not installed. But as I think you mentioned above we need to also check if the environment is debug (in which case the twig debug handler should activate). Does that make sense?

We should forget about Twig here, it's totally independent code. What matters is if there is a listener that already set a response, and if not, then generate one. This question doesn't need to wonder about debug or not.
There are ppl that do set responses here, while not using twig (eg for APIs.)

nicolas-grekas · 2018-04-23T18:46:51Z

@dmaicher I'm wondering: the approach with the global exception handler I proposed in the front controller is also able to deal with fatal errors.
What does the approach in this PR has that's making it better?
That's an honest question :)

dmaicher · 2018-04-23T19:02:20Z

@nicolas-grekas sure let's discuss it 😊

What does the approach in this PR has that's making it better?

In my opinion the front controller should be a really lightweight "wrapper" around the kernel and just send the response to the client that has been returned by the kernel
if using WebTestCase on some API project (no custom exception listeners, no TwigBundle) you want to assert 404 responses for example: How to do it when that logic is in the front-controller?
people have to update their front controllers to benefit from this feature? 😃

is also able to deal with fatal errors.

Currently I'm using Symfony\Component\Debug\ErrorHandler::register(); for that in my front controllers so that they are turned into exceptions 😋

nicolas-grekas · 2018-04-23T19:25:35Z

Currently I'm using ErrorHandler::register(); for that in my front controllers so that they are turned into exceptions

but FrameworkBundle::boot() already does the same, isn't it? So fatals are already turned into exceptions on prod also and you could remove the line you added in your front controller, isn't it?

dmaicher · 2018-04-24T08:21:36Z

but FrameworkBundle::boot() already does the same, isn't it? So fatals are already turned into exceptions on prod also and you could remove the line you added in your front controller, isn't it?

true! I just double checked it and you are right 😉 This particular project was started with Symfony 2.3 ages ago and I remember back then there was some reason for this. But now it works fine 👍 😃

nicolas-grekas · 2018-04-25T14:17:21Z

I don't have the double display on my side. I'm not testing with Apache but with php -S, but it shouldn't make a difference. Can you try again from a fresh install? If my linked patch above is OK to you, would you borrow it and update the PR accordingly?

junowilderness · 2018-04-25T14:36:26Z

@nicolas-grekas Your diff works for me, and seems simpler. The only question I have in fact is whether it intended that a production environment 404 should look like this:

nicolas-grekas · 2018-04-25T14:42:54Z

whether it intended that a production environment 404 should look like this

yes it is IMHO: we need to provide some default page, we have one in Debug. If this doesn't fit, then we should update Debug (ie fix #22964)
And ppl are still free to register their own listener if the default doesn't fit.
But this PR is not the place to work on the display.

junowilderness · 2018-04-25T14:57:03Z

@nicolas-grekas I see. I've just now switched to your diff. I haven't rebased or squashed yet.

nicolas-grekas · 2018-04-25T15:28:09Z

Thanks. It'd be awesome if you could squash and add a test case if possible... :)

junowilderness · 2018-04-25T15:54:48Z

@nicolas-grekas It is rebased and squashed. I stole the test addition you made in #26928.

…nstalled

nicolas-grekas

I stole the test addition you made in #26928.

that didn't work actually as an exception is thrown there.
I pushed a test on your fork instead :)

All good on my side, thank you!

nicolas-grekas · 2018-04-29T07:28:55Z

Thank you @cilefen.

…nstalled (cilefen) This PR was merged into the 3.4 branch. Discussion ---------- [HttpKernel] Catch HttpExceptions when templating is not installed | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | ? | Deprecations? | no | Tests pass? | ? | Fixed tickets | #25844 | License | MIT | Doc PR | symfony/symfony-docs#...  - [x] Test manually - [x] Check for BC breaks - [x] Needs tests  Commits ------- 4e527aa bug #25844 [HttpKernel] Catch HttpExceptions when templating is not installed

This PR was merged into the 3.4 branch. Discussion ---------- [HttpKernel] fix registering IDE links | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27194 | License | MIT | Doc PR | - Reverts some changes made in #27074 in `FrameworkBundle::boot()` that create the linked issue. Leverages changes done in #26138 instead. Commits ------- 92e3023 [HttpKernel] fix registering IDE links

…lating is not installed (cilefen)" This reverts commit b213c5a, reversing changes made to 61af0e3.

…templating is not installed (cilefen)" (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- Revert "bug #26138 [HttpKernel] Catch HttpExceptions when templating is not installed (cilefen)" | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27212 | License | MIT | Doc PR | - This reverts commit b213c5a, reversing changes made to 61af0e3. This breaks BC and is more like a new feature, let's move this on master. Commits ------- c6acad7 Revert "bug #26138 [HttpKernel] Catch HttpExceptions when templating is not installed (cilefen)"

* 3.4: Revert "bug #26138 [HttpKernel] Catch HttpExceptions when templating is not installed (cilefen)"

* 4.0: Revert "bug #26138 [HttpKernel] Catch HttpExceptions when templating is not installed (cilefen)"

* 4.1: Revert "bug #26138 [HttpKernel] Catch HttpExceptions when templating is not installed (cilefen)"

carsonbot added Status: Needs Review HttpKernel Bug BC Break labels Feb 11, 2018

nicolas-grekas removed the BC Break label Feb 11, 2018

nicolas-grekas added this to the 3.4 milestone Feb 11, 2018

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Feb 11, 2018

dmaicher reviewed Feb 11, 2018

View reviewed changes

junowilderness commented Feb 26, 2018

View reviewed changes

junowilderness mentioned this pull request Feb 26, 2018

Functional tests fail when NotFoundHttpException is thrown #25311

Closed

nicolas-grekas mentioned this pull request Apr 14, 2018

[HttpKernel] Create responses for unhandled HttpExceptionInterface exceptions #26928

Closed

junowilderness closed this Apr 16, 2018

nicolas-grekas reopened this Apr 23, 2018

nicolas-grekas reviewed Apr 23, 2018

View reviewed changes

nicolas-grekas requested changes Apr 23, 2018

View reviewed changes

nicolas-grekas changed the title ~~Fix bug #25844 [HttpKernel] Catch HttpExceptions without templating installed~~ [HttpKernel] Catch HttpExceptions when templating is not installed Apr 23, 2018

junowilderness commented Apr 23, 2018

View reviewed changes

nicolas-grekas mentioned this pull request Apr 24, 2018

Register ExceptionHandler in prod symfony/recipes#401

Closed

nicolas-grekas mentioned this pull request Apr 25, 2018

[Debug] is the new design desired when the Debug component is used in non-debug mode, and standalone? #22964

Closed

bug #25844 [HttpKernel] Catch HttpExceptions when templating is not i…

4e527aa

…nstalled

nicolas-grekas approved these changes Apr 25, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Apr 25, 2018

nicolas-grekas added the Ready label Apr 25, 2018

nicolas-grekas merged commit 4e527aa into symfony:3.4 Apr 29, 2018

This was referenced Apr 30, 2018

Release v3.4.9 #27102

Merged

Release v4.0.9 #27103

Merged

junowilderness deleted the 25844 branch May 1, 2018 15:58

nicolas-grekas mentioned this pull request May 9, 2018

[FrameworkBundle][BC Break] Adding http_exception_listener introduces BC break to exception handling #27212

Closed

nicolas-grekas mentioned this pull request May 28, 2018

[HttpKernel] fix registering IDE links #27396

Merged

nicolas-grekas mentioned this pull request Jun 5, 2018

[HttpKernel] Turn HTTP exceptions to responses on terminateWithException() #27505

Closed

nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request Jun 6, 2018

Revert "bug symfony#26138 [HttpKernel] Catch HttpExceptions when temp…

c6acad7

…lating is not installed (cilefen)" This reverts commit b213c5a, reversing changes made to 61af0e3.

nicolas-grekas added a commit that referenced this pull request Jun 6, 2018

Merge branch '3.4' into 4.0

59cd277

* 3.4: Revert "bug #26138 [HttpKernel] Catch HttpExceptions when templating is not installed (cilefen)"

nicolas-grekas added a commit that referenced this pull request Jun 6, 2018

Merge branch '4.0' into 4.1

72f7ac0

* 4.0: Revert "bug #26138 [HttpKernel] Catch HttpExceptions when templating is not installed (cilefen)"

nicolas-grekas added a commit that referenced this pull request Jun 6, 2018

Merge branch '4.1'

50c4384

* 4.1: Revert "bug #26138 [HttpKernel] Catch HttpExceptions when templating is not installed (cilefen)"

This was referenced Jun 25, 2018

Release v3.4.12 #27704

Merged

Release v4.0.12 #27705

Merged

Release v4.1.1 #27706

Merged

[HttpKernel] Catch HttpExceptions when templating is not installed #26138

[HttpKernel] Catch HttpExceptions when templating is not installed #26138

Conversation

junowilderness commented Feb 11, 2018 • edited by nicolas-grekas Loading

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

junowilderness commented Feb 26, 2018

nicolas-grekas commented Apr 14, 2018

junowilderness commented Apr 16, 2018

nicolas-grekas commented Apr 23, 2018

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

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 23, 2018

junowilderness commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmaicher Apr 23, 2018 • edited Loading

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

nicolas-grekas commented Apr 23, 2018

dmaicher commented Apr 23, 2018

nicolas-grekas commented Apr 23, 2018 • edited Loading

dmaicher commented Apr 24, 2018

nicolas-grekas commented Apr 25, 2018 • edited Loading

junowilderness commented Apr 25, 2018

nicolas-grekas commented Apr 25, 2018

junowilderness commented Apr 25, 2018

nicolas-grekas commented Apr 25, 2018 • edited Loading

junowilderness commented Apr 25, 2018

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 29, 2018

junowilderness commented Feb 11, 2018 •

edited by nicolas-grekas

Loading

dmaicher Apr 23, 2018 •

edited

Loading

nicolas-grekas commented Apr 23, 2018 •

edited

Loading

nicolas-grekas commented Apr 25, 2018 •

edited

Loading

nicolas-grekas commented Apr 25, 2018 •

edited

Loading