[HttpKernel] Add a better error messages when passing a private or non-tagged controller #25201

Simperfit · 2017-11-29T07:29:52Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#25192
License	MIT
Doc PR	none

Add more tests

sroze · 2017-11-29T07:31:12Z

src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php

+            return parent::instantiateController($class);
+        } catch (FatalThrowableError $e) {
+            if ($this->container instanceof ContainerBuilder) {
+                var_dump($this->container->getRemovedIds());


Simperfit · 2017-11-29T07:45:23Z

oops i pushed it to fast since I was in the train will remove it Le mer. 29 nov. 2017 à 08:31, Samuel ROZE <notifications@github.com> a écrit :

…

***@***.**** commented on this pull request. ------------------------------ In src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php <#25201 (comment)>: > @@ -82,10 +85,19 @@ protected function createController($controller) */ protected function instantiateController($class) { - if ($this->container->has($class)) { - return $this->container->get($class); + try { + if ($this->container->has($class)) { + return $this->container->get($class); + } + + return parent::instantiateController($class); + } catch (FatalThrowableError $e) { + if ($this->container instanceof ContainerBuilder) { + var_dump($this->container->getRemovedIds()); Meh — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#25201 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADSq8r-Ka3NRfSQN47vNDH_zF_4JpHFrks5s7QhtgaJpZM4Quh-N> .

sroze · 2017-11-29T08:13:18Z

src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php

+            }
+
+            return parent::instantiateController($class);
+        } catch (FatalThrowableError $e) {


You need to be more specific than that. First of all: isn't it on the $this->container->get(...) that you want to catch the exception? Also, can you use the exception specific to this error, not a Debug exception?

Actually, I'm not sure: this is Debug turning an E_RECOVERABLE_ERROR into an exception.
There is no way to catch this error without the debug component.
And any other exception might be unrelated in fact.
So this looks specific to me: improves DX by throwing to the user when applicable.

The problem I encountered into issue #25192 is that the Controller is not registered into the container, and the parent::instantiateController() method is called, which instantiate the controller without any argument.

So the try catch should only be for the return parent::instantiateController($class);.

Can we not use introspection in this method (instantiateController) to check that the constructor does not require arguments, and throw an exception here if it does?

So it is maybe something to change on the ControllerResolver directly?

I think I would go for that and throw an exception telling something similar to

"Your controller constructor requires argument but is neither declared as a public service nor tagged with controller.service_arguments"

But having instrospection is heavy work so I don't know if it worth it...

the parent doens't know anything about the container, so here is the correct place, isn't it?
catching only the parent call looks sensible to me also

@nicolas-grekas I think the point here is that ControllerResolver::instantiateController() cannot instantiate controllers requiring parameters in the constructor. So this method should check that the controller's constructor does not require any arguments and throw an exception if it does. This is not related to container.

If a controller is correctly configured as a public service or tagged with controller.service_arguments, then the controller will not be instantiate, and pulled from the container so the ControllerResolver::instantiateController() method will never be called, so no problem here...

So this method should check that the controller's constructor does not require any arguments and throw an exception if it does.

This supposes that the check is free, which is not the case. Here, we're in a performance critical code path. There is no better way to do this check than the current one: try, and catch any error.

sroze · 2017-11-29T08:13:40Z

Status: Needs Work

nicolas-grekas · 2017-11-29T08:24:15Z

src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php

@@ -13,6 +13,9 @@

 use Psr\Container\ContainerInterface;
 use Psr\Log\LoggerInterface;
+use Symfony\Component\Debug\Exception\FatalThrowableError;
+use Symfony\Component\DependencyInjection\ContainerBuilder;


Why ContainerBuilder, and not Container?

ill use Container

nicolas-grekas · 2017-11-29T08:38:22Z

src/Symfony/Component/HttpKernel/Tests/Controller/ContainerControllerResolverTest.php

@@ -175,7 +196,7 @@ protected function createControllerResolver(LoggerInterface $logger = null, Cont

    protected function createMockContainer()
    {
-        return $this->getMockBuilder(ContainerInterface::class)->getMock();
+        return $this->getMockBuilder(ContainerBuilder::class)->getMock();


Ill revert that.

nicolas-grekas · 2017-11-29T09:30:05Z

to be rebased on 3.4 btw

nicolas-grekas · 2017-11-29T19:12:33Z

src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php

@@ -82,10 +84,25 @@ protected function createController($controller)
     */
    protected function instantiateController($class)
    {
-        if ($this->container->has($class)) {
-            return $this->container->get($class);
+        try {


Should be moved below, see comments

nicolas-grekas · 2017-11-29T22:14:57Z

src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php

+        } catch (\ArgumentCountError $e) {
+        }
+
+        if (null !== $e) {


$e should be initialized, is it? This "if" should be merged with the next one also.

nicolas-grekas · 2017-11-29T22:17:26Z

src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php

+
+        if (null !== $e) {
+            if ($this->container instanceof Container) {
+                if (in_array($class, $this->container->getRemovedIds())) {


A bit unrelated but maybe an opportunity to catch character case mismatches also here? Supposes that we can prevent the parent from failing hard in that case.

ogizanagi · 2017-11-30T07:12:12Z

src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php

@@ -86,6 +94,18 @@ protected function instantiateController($class)
            return $this->container->get($class);
        }

-        return parent::instantiateController($class);
+        $e = null;


Could be removed and use isset($e) in if below ?

isn't better for performance to do this instead of isset ?

Hmm, no idea. Actually I thought we already used isset($e) for such cases in many places. But it seems I was wrong. So fine 👍

Setting to null is the CS we use in the code base.

oh, in fact, this can be removed, the variable is not needed: if we reach L99, $e will always be non null
which means the null !== $e check can be removed from the below if also

ogizanagi · 2017-11-30T07:15:50Z

src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php

+        } catch (\ArgumentCountError $e) {
+        }
+
+       if (null !== $e && $this->container instanceof Container && in_array($class, $this->container->getRemovedIds())) {


Shouldn't this be in the Fwb ControllerResolver instead?

@nicolas-grekas WDYT?

this would limit the behavior to ppl using the fwb, I don't see any reason to do so

nicolas-grekas · 2017-11-30T07:25:20Z

src/Symfony/Component/HttpKernel/Tests/Controller/ContainerControllerResolverTest.php

+            ->will($this->returnValue(array(ImpossibleConstructController::class)))
+        ;
+
+        if (method_exists($this, 'expectException')) {


Could we use an annotation instead?

Ill try but in other test we use this instead of annot, it's maybe because of lower version of php using lower version of phpunit.

nicolas-grekas · 2017-11-30T07:26:55Z

src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php

+        }
+
+        if (null !== $e && $this->container instanceof Container && in_array($class, $this->container->getRemovedIds())) {
+            throw new \InvalidArgumentException(sprintf('The controller %s seems to be removed from the container, which means that he maybe private, add the tag \'controller.service_arguments\' or made it public.', $class));


RuntimeException? The argument is valid, the config is not.
Also pass $e as previous exception to the constructor?

nicolas-grekas · 2017-11-30T08:24:54Z

src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php

+        }
+
+        if (null !== $e && $this->container instanceof Container && in_array($class, $this->container->getRemovedIds())) {
+            throw new \RuntimeException(sprintf('The controller %s seems to be removed from the container, which means that he maybe private, add the tag \'controller.service_arguments\' or made it public.', $class), 0, $e);


I propose this:
new \LogicException(sprintf('Controller "%s" cannot be fetched from the container because it is private. Did you forget to tag the service with "controller.service_arguments"?', $class), 0, $e);

nicolas-grekas · 2017-11-30T08:28:46Z

src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php

+        } catch (\ArgumentCountError $e) {
+        }
+
+        if (null !== $e && $this->container instanceof Container && in_array($class, $this->container->getRemovedIds())) {


null !== $e && to be removed, see comment above

nicolas-grekas · 2017-11-30T08:29:10Z

src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php

+
+        if (null !== $e && $this->container instanceof Container && in_array($class, $this->container->getRemovedIds())) {
+            throw new \RuntimeException(sprintf('The controller %s seems to be removed from the container, which means that he maybe private, add the tag \'controller.service_arguments\' or made it public.', $class), 0, $e);
+        }


else throw $e

dunglas · 2017-11-30T09:34:48Z

src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php

+        }
+
+        if ($this->container instanceof Container && in_array($class, $this->container->getRemovedIds())) {
+            throw new \LogicException(sprintf('Controller "%s" cannot be fetched from the container because it is private. Did you forget to tag the service with "controller.service_arguments"?', $class), 0, $e);


It looks to weird from a DX POV to recommend to add this tag instead of just recommending to make the service public.

I crafted the message so that, from DX pov, the best experience is suggested by default (allowing services as arguments, ie the tag)
and from a more exprienced DX pov, the word "private" hints the advanced dev about "public" being just missing.
If one doesn't know the difference, then we should definitely suggest the tag. Hence the current message.

dunglas

👍

…n-tagged controller

nicolas-grekas · 2017-11-30T14:36:38Z

Thank you @Simperfit.

…ivate or non-tagged controller (Simperfit) This PR was merged into the 3.4 branch. Discussion ---------- [HttpKernel] Add a better error messages when passing a private or non-tagged controller | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | #25192  | License | MIT | Doc PR | none - [x] Add more tests Commits ------- b1173f3 [HttpKernel] Add a better error messages when passing a private or non-tagged controller

…ler has been removed (sroze) This PR was merged into the 3.4 branch. Discussion ---------- [DX][HttpKernel] Throw a sensible exception when controller has been removed | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25335 | License | MIT | Doc PR | ø Following on #25201, we need to throw the same kind of sensible exception when the controller service is not found. Commits ------- 458d63f Throw a sensible exception when controller has been removed

carsonbot added Status: Needs Review HttpKernel labels Nov 29, 2017

sroze reviewed Nov 29, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Nov 29, 2017

nicolas-grekas reviewed Nov 29, 2017

View reviewed changes

Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 2 times, most recently from a0c7d55 to 193a461 Compare November 29, 2017 08:46

nicolas-grekas added this to the 3.4 milestone Nov 29, 2017

Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch from 193a461 to 1f14f4d Compare November 29, 2017 09:38

Simperfit changed the base branch from 4.0 to 3.4 November 29, 2017 09:39

Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 2 times, most recently from 7ce94f3 to 35ab6dd Compare November 29, 2017 19:10

nicolas-grekas reviewed Nov 29, 2017

View reviewed changes

Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 3 times, most recently from 9ddc5c9 to 828d8c4 Compare November 29, 2017 19:28

nicolas-grekas reviewed Nov 29, 2017

View reviewed changes

Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 3 times, most recently from 7cb768b to 6345f3b Compare November 30, 2017 07:15

ogizanagi reviewed Nov 30, 2017

View reviewed changes

Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch from 6345f3b to 210016f Compare November 30, 2017 07:22

nicolas-grekas reviewed Nov 30, 2017

View reviewed changes

Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 2 times, most recently from b0d2aba to 1a872d1 Compare November 30, 2017 07:37

nicolas-grekas reviewed Nov 30, 2017

View reviewed changes

Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 2 times, most recently from 2d29a78 to 83c0784 Compare November 30, 2017 09:17

dunglas reviewed Nov 30, 2017

View reviewed changes

Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 2 times, most recently from a784e86 to 92e67e6 Compare November 30, 2017 09:47

carsonbot added Status: Reviewed and removed Status: Needs Work labels Nov 30, 2017

dunglas approved these changes Nov 30, 2017

View reviewed changes

Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 4 times, most recently from f5709c5 to 5824d18 Compare November 30, 2017 14:20

[HttpKernel] Add a better error messages when passing a private or no…

b1173f3

…n-tagged controller

Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch from 5824d18 to b1173f3 Compare November 30, 2017 14:25

nicolas-grekas approved these changes Nov 30, 2017

View reviewed changes

nicolas-grekas merged commit b1173f3 into symfony:3.4 Nov 30, 2017

This was referenced Nov 30, 2017

Release v3.4.0 #25223

Merged

Release v4.0.0 #25224

Merged

nicolas-grekas mentioned this pull request Dec 5, 2017

[DependencyInjection] legit definition seems removed #25335

Closed

sroze deleted the feature/add-dx-when-passing-a-private-controller branch December 5, 2017 16:27

sroze mentioned this pull request Dec 5, 2017

[DX][HttpKernel] Throw a sensible exception when controller has been removed #25339

Merged

Uh oh!

[HttpKernel] Add a better error messages when passing a private or non-tagged controller #25201

[HttpKernel] Add a better error messages when passing a private or non-tagged controller #25201

Uh oh!

Conversation

Simperfit commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit commented Nov 29, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sroze commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit commented Nov 29, 2017 •

edited

Loading

nicolas-grekas Nov 29, 2017 •

edited

Loading

nicolas-grekas Nov 29, 2017 •

edited

Loading

ogizanagi Nov 30, 2017 •

edited

Loading

nicolas-grekas Nov 30, 2017 •

edited

Loading

dunglas Nov 30, 2017 •

edited

Loading

nicolas-grekas Nov 30, 2017 •

edited

Loading

dunglas left a comment •

edited

Loading