Skip to content
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

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

Conversation

@Simperfit
Copy link
Contributor

@Simperfit Simperfit commented Nov 29, 2017

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
return parent::instantiateController($class);
} catch (FatalThrowableError $e) {
if ($this->container instanceof ContainerBuilder) {
var_dump($this->container->getRemovedIds());
Copy link
Member

@sroze sroze Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh

Loading

@Simperfit
Copy link
Contributor Author

@Simperfit Simperfit commented Nov 29, 2017

Loading

}

return parent::instantiateController($class);
} catch (FatalThrowableError $e) {
Copy link
Member

@sroze sroze Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Loading

Copy link
Member

@sroze sroze Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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...

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

@sroze
Copy link
Member

@sroze sroze commented Nov 29, 2017

Status: Needs Work

Loading

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

use Psr\Container\ContainerInterface;
use Psr\Log\LoggerInterface;
use Symfony\Component\Debug\Exception\FatalThrowableError;
use Symfony\Component\DependencyInjection\ContainerBuilder;
Copy link
Member

@nicolas-grekas nicolas-grekas Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ContainerBuilder, and not Container?

Loading

Copy link
Contributor Author

@Simperfit Simperfit Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ill use Container

Loading

@@ -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();
Copy link
Member

@nicolas-grekas nicolas-grekas Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Loading

Copy link
Contributor Author

@Simperfit Simperfit Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ill revert that.

Loading

@Simperfit Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 2 times, most recently from a0c7d55 to 193a461 Nov 29, 2017
@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 29, 2017

to be rebased on 3.4 btw

Loading

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Nov 29, 2017
@Simperfit Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch from 193a461 to 1f14f4d Nov 29, 2017
@Simperfit Simperfit changed the base branch from 4.0 to 3.4 Nov 29, 2017
@Simperfit Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 2 times, most recently from 7ce94f3 to 35ab6dd Nov 29, 2017
@@ -82,10 +84,25 @@ protected function createController($controller)
*/
protected function instantiateController($class)
{
if ($this->container->has($class)) {
return $this->container->get($class);
try {
Copy link
Member

@nicolas-grekas nicolas-grekas Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be moved below, see comments

Loading

@Simperfit Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 3 times, most recently from 9ddc5c9 to 828d8c4 Nov 29, 2017
} catch (\ArgumentCountError $e) {
}

if (null !== $e) {
Copy link
Member

@nicolas-grekas nicolas-grekas Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading


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

@nicolas-grekas nicolas-grekas Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

@Simperfit Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 3 times, most recently from 7cb768b to 6345f3b Nov 30, 2017
@@ -86,6 +94,18 @@ protected function instantiateController($class)
return $this->container->get($class);
}

return parent::instantiateController($class);
$e = null;
Copy link
Member

@ogizanagi ogizanagi Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Contributor Author

@Simperfit Simperfit Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member

@ogizanagi ogizanagi Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

} catch (\ArgumentCountError $e) {
}

if (null !== $e && $this->container instanceof Container && in_array($class, $this->container->getRemovedIds())) {
Copy link
Member

@ogizanagi ogizanagi Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be in the Fwb ControllerResolver instead?

Loading

Copy link
Member

@ogizanagi ogizanagi Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

@Simperfit Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch from 6345f3b to 210016f Nov 30, 2017
->will($this->returnValue(array(ImpossibleConstructController::class)))
;

if (method_exists($this, 'expectException')) {
Copy link
Member

@nicolas-grekas nicolas-grekas Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use an annotation instead?

Loading

Copy link
Contributor Author

@Simperfit Simperfit Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

}

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));
Copy link
Member

@nicolas-grekas nicolas-grekas Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

@Simperfit Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 2 times, most recently from b0d2aba to 1a872d1 Nov 30, 2017
}

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);
Copy link
Member

@nicolas-grekas nicolas-grekas Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Loading

} catch (\ArgumentCountError $e) {
}

if (null !== $e && $this->container instanceof Container && in_array($class, $this->container->getRemovedIds())) {
Copy link
Member

@nicolas-grekas nicolas-grekas Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading


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);
}
Copy link
Member

@nicolas-grekas nicolas-grekas Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else throw $e

Loading

@Simperfit Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 2 times, most recently from 2d29a78 to 83c0784 Nov 30, 2017
}

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);
Copy link
Member

@dunglas dunglas Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

@Simperfit Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 2 times, most recently from a784e86 to 92e67e6 Nov 30, 2017
Copy link
Member

@dunglas dunglas left a comment

👍

Loading

@Simperfit Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch 4 times, most recently from f5709c5 to 5824d18 Nov 30, 2017
@Simperfit Simperfit force-pushed the feature/add-dx-when-passing-a-private-controller branch from 5824d18 to b1173f3 Nov 30, 2017
@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 30, 2017

Thank you @Simperfit.

Loading

@nicolas-grekas nicolas-grekas merged commit b1173f3 into symfony:3.4 Nov 30, 2017
1 of 3 checks passed
Loading
nicolas-grekas added a commit that referenced this issue Nov 30, 2017
…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 <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | #25192 <!-- #-prefixed issue number(s), if any -->
| 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
@fabpot fabpot mentioned this pull request Nov 30, 2017
@fabpot fabpot mentioned this pull request Nov 30, 2017
@sroze sroze deleted the feature/add-dx-when-passing-a-private-controller branch Dec 5, 2017
fabpot added a commit that referenced this issue Dec 7, 2017
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants