[DX] Improve exception message when AbstractController::getParameter fails #27443

curry684 · 2018-05-31T10:05:11Z

Q	A
Branch?	4.1
Bug fix?	no (DX)
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#27436
License	MIT
Doc PR	symfony/symfony-docs#...

Improve exception message for situations where the parameter_bag is not present in AbstractController. Also fixed the exception to the correct type.

Fixes symfony#27436

nicolas-grekas · 2018-05-31T10:21:40Z

src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php

@@ -64,7 +65,9 @@ public function setContainer(ContainerInterface $container)
    protected function getParameter(string $name)
    {
        if (!$this->container->has('parameter_bag')) {
-            throw new \LogicException('The "parameter_bag" service is not available. Try running "composer require dependency-injection:^4.1"');
+            throw new ServiceNotFoundException('parameter_bag', null, null, array(),
+                'The "parameter_bag" service could not be located. Ensure that symfony/dependency-injection 4.1.0 or higher '.


it would be great to decide ourselves instead of this "and" (line below) in the message (a class_exists check and we can know for sure which alternative is the correct one)

Any ideas on a class to reliably test that was introduced in 4.1 and will not disappear until "infinity", so not internal?

ContainerBagInterface :)

chalasr · 2018-05-31T14:45:18Z

src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php

-            throw new \LogicException('The "parameter_bag" service is not available. Try running "composer require dependency-injection:^4.1"');
+            $message = 'The "parameter_bag" service could not be located. ';
+            if (!interface_exists(ContainerBagInterface::class)) {
+                $message .= 'Upgrade symfony/dependency-injection to at least 4.1.0 to have it injected automatically.';


suggesting to install/upgrade symfony/dependency-injection does not seem relevant as framework-bundle requires it ^4.1

Lol, hadn't bothered to doublecheck that because it was the original message and I assumed it to have a reason.

…^4.1

nicolas-grekas · 2018-05-31T16:03:50Z

src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php

-            throw new \LogicException('The "parameter_bag" service is not available. Try running "composer require dependency-injection:^4.1"');
+            throw new ServiceNotFoundException('parameter_bag', null, null, array(),
+                'The "parameter_bag" service could not be located. Ensure that the controller is either set '.
+                'to be autoconfigured or wired manually to inject it.');


[...] to be autoconfigured or has the "container.service_subscriber" tag.?

should be on one single long line

sprintf('The "%s::getParameter()" method is missing a parameter bag to work properly. Did you forget to register your controller as a service subscriber? This can be fixed e.g. by using autoconfiguration or by manually wiring a "parameter_bag" in the service locator passed to the controller.', get_class($this))

(the name of the parameter is of no importance IMHO, but the controller class IS.)

javiereguiluz · 2018-05-31T16:04:04Z

Would you agree making this error message more verbose to give more context?

Original:

The "parameter_bag" service could not be located. Ensure that the controller is
either set to be autoconfigured or wired manually to inject it.

Proposal:

sprintf('The "getParameter()" method of the controller can\'t get the value of
the "%s" parameter because the "parameter_bag" service could not be located.
Either enable autowiring for the controller or inject the "parameter_bag"
service manually in the controller.', $name);

My concern with the current message is that the user wrote $this->getParameter('foo') and the error message is about something strange called parameter_bag that the user didn't use anywhere in their code.

weaverryan · 2018-05-31T17:14:34Z

You’re right - I missed that. I like your message more :)

curry684 · 2018-05-31T17:43:19Z

@javiereguiluz that suggestion is wrong on 2 counts: first off autowiring won't fix it, it needs autoconfigure. Secondly injecting parameter_bag into the controller manually won't work as it has to be in whatever is in $this->container, effectively meaning you need to have a ServiceLocator or make the parameter_bag service public, both of which are unfeasible for end-user developers not at home in DI innards.

In practice autoconfigure is the only viable solution to the problem. Realizing that makes you second guess whether this generic feature was a good idea to begin with even though autoconfigure+autowire is now recommended config - it's still not required.

edit: out of the box thinking - shouldn't we just rock hard fail somewhere at compile-time if an AbstractController is used but doesn't get its ServiceLocator injected in any way? I suspect the missing parameter_bag is just the first and smallest of issues one would run into (most subscribed services in its list are private).

nicolas-grekas · 2018-05-31T17:58:55Z

autoconfigure+autowire is now recommended config - it's still not required.

that's still the case, autoconfiguration+autowiring is not the only way to define a service locator for a service subscriber. Using the container.service_subscriber tag + its attributes are the really low level way to handle this configuration.

Of course in practice, esp. for AbstractController, it's still the most common way to configure them, so that the message is OK mentioning these imho.

shouldn't we just rock hard fail somewhere at compile-time if an AbstractController is used but doesn't get its ServiceLocator injected in any way

what would be the criteria? from the inside of the class, all service locators look the same :)
and at compile time, it's just a service subscriber. So yes, we could remove a few "?" in front of types returned by getSubscribedServices, but that's not possible for optional deps, and all are here. For AbstractControllers, because of the special way they are handled, I'm not sure we can do anything, do we?

curry684 · 2018-05-31T20:46:14Z

the really low level way to handle this configuration

Which makes it "unfeasible for end-user developers" like I said and only realistic for those that do not really require better DX in error messages like that in the first place.

For AbstractControllers, because of the special way they are handled, I'm not sure we can do anything, do we?

There's multiple things we could do actually. Reviewing the default subscriptions quickly we have 5 that could (should?) lose the optional as they're direct dependencies of FrameworkBundle:

yes 'router' => '?'.RouterInterface::class,
yes 'request_stack' => '?'.RequestStack::class,
yes 'http_kernel' => '?'.HttpKernelInterface::class,
no  'serializer' => '?'.SerializerInterface::class,
yes 'session' => '?'.SessionInterface::class,
no  'security.authorization_checker' => '?'.AuthorizationCheckerInterface::class,
no  'templating' => '?'.EngineInterface::class,
no  'twig' => '?'.Environment::class,
no  'doctrine' => '?'.ManagerRegistry::class,
no  'form.factory' => '?'.FormFactoryInterface::class,
no  'security.token_storage' => '?'.TokenStorageInterface::class,
no  'security.csrf.token_manager' => '?'.CsrfTokenManagerInterface::class,
yes 'parameter_bag' => '?'.ContainerBagInterface::class,
no  'message_bus' => '?'.MessageBusInterface::class,

This automatically makes them candidates for a late-phase compiler pass to check integrity - any service that derives from AbstractController can have its getSubscribedServices queried, and we can then fail hard at compile-time if any mandatory ones (including user added ones) cannot resolve.

It's the difference between getting "You've screwed up on how to use AbstractController for <10 reasons>" during compiletime for all of them at once, or 10 separate vague errors about missing parameter bags, request stacks and HTTP kernels at runtime.

(actually the compiler pass could generically check all ServiceSubscriberInterface implementers for this as at that point we know whether mandatory references can ever be resolved on this container, and tell the user to add the ? in front if it's intentional that we can't find it... but that could potentially be considered a BC break)

curry684 · 2018-05-31T20:52:06Z

Meanwhile, how about this for the issue at hand, mainly adapting Javier's suggestion to implementation:

sprintf('The "getParameter()" method of the controller can\'t get the value of
the "%s" parameter because the "parameter_bag" service could not be located.
Either enable autoconfigure for the controller service or inject a ServiceLocator
referencing a "parameter_bag" service manually in the controller.', $name);

nicolas-grekas · 2018-05-31T21:04:07Z

5 that could (should?) lose the optional

Could, but what would we gain doing so? Nothing related to the issue at glance. To which one then? This means "shouldn't" to me :)

curry684 · 2018-05-31T21:24:47Z

It's related to the second question: should having a ServiceLocator with a mandatory but undefined reference crash unexpectedly during development or right away at compiletime right after wrongly defining it?

I think for DX it'd be better to fail early, and it'd fix the entire issue at hand ages before we ever got to the getParameter function. TS would've been happy to get a "Controllers deriving from AbstractController must contain a ServiceLocator with valid references to 'router', 'request_stack', 'http_kernel', 'session' and 'parameter_bag' services. Enable autoconfigure for the controller service to handle this automatically" 30 seconds after upgrading to Symfony 4.1.

It'd even microscopically improve performance as it would mean getParameter can just be reduced to $this->container->get('parameter_bag')->get($parameter) without needing the runtime check for existence on every call first.

nicolas-grekas · 2018-05-31T21:39:30Z

But this wouldn't fail at compile time. Or based on what criteria would it fail? What would be the trigger to throw this when the issue is precisely that this controller was not registered for the compiler pass that looks for building locators for subscribers?

curry684 · 2018-05-31T22:13:18Z

If we can autoconfigure services that implement ServiceSubscriberInterface we can also enumerate them and check that we can resolve what they subscribe to? AbstractController implements it so it would automatically be subject to that check.

nicolas-grekas · 2018-06-01T07:12:03Z

Here is a proposal to fix the DX issue: #27462

curry684 · 2018-06-01T22:25:12Z

Good idea, but let's also fix the error message now as it's currently just wrong in 4.1. Just pushed my last proposal merging Javier's suggestion with mine.

nicolas-grekas · 2018-06-02T04:14:10Z

src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php

@@ -64,7 +65,8 @@ public function setContainer(ContainerInterface $container)
    protected function getParameter(string $name)
    {
        if (!$this->container->has('parameter_bag')) {
-            throw new \LogicException('The "parameter_bag" service is not available. Try running "composer require dependency-injection:^4.1"');
+            throw new ServiceNotFoundException('parameter_bag', null, null, array(),
+                sprintf('The "getParameter()" method of the controller can\'t get the value of the "%s" parameter because the "parameter_bag" service could not be located. Either enable autoconfigure for the controller service or inject a ServiceLocator referencing a "parameter_bag" service manually in the controller.', $name));


You missed my last comment here it is :) Also this should really be on one line, not two.

sprintf('The "%s::getParameter()" method is missing a parameter bag to work properly. Did you forget to register your controller as a service subscriber? This can be fixed e.g. by using autoconfiguration or by manually wiring a "parameter_bag" in the service locator passed to the controller.', get_class($this))

(the name of the parameter is of no importance IMHO, but the controller class IS.)

Thought you meant only the string with the previous one-line remark. Is the Symfony code style on layout stuff like that documented somewhere? It's confusing that most projects (including our internal ones) explicitly prohibit lines beyond common-screen limits (ie. ~120 characters) and Symfony 'prefers oneliners'.

xabbuh

with Nicolas' comment applied

nicolas-grekas

Long line for user message allow using grep to find where that exception/deprecation comes from. That's and important DX feature for ppl that use this workflow when debugging.

curry684 · 2018-06-03T09:00:39Z

Ah ok, understandable from that perspective.

chalasr · 2018-06-04T09:10:17Z

Thank you @curry684.

…getParameter fails (curry684) This PR was squashed before being merged into the 4.1 branch (closes #27443). Discussion ---------- [DX] Improve exception message when AbstractController::getParameter fails | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | no (DX) | New feature? | no | BC breaks? | no  | Deprecations? | no | Tests pass? | yes  | Fixed tickets | #27436 | License | MIT | Doc PR | symfony/symfony-docs#...  Improve exception message for situations where the `parameter_bag` is not present in `AbstractController`. Also fixed the exception to the correct type. Commits ------- a8f4128 [DX] Improve exception message when AbstractController::getParameter fails

carsonbot added the Status: Needs Review label May 31, 2018

curry684 changed the title ~~Improve exception message when AbstractController::getParameter fails~~ [DX] Improve exception message when AbstractController::getParameter fails May 31, 2018

Improve exception message when AbstractController::getParameter fails

a39bed8

Fixes symfony#27436

curry684 force-pushed the fix-27436 branch from 7045c4c to a39bed8 Compare May 31, 2018 10:10

nicolas-grekas added this to the 4.1 milestone May 31, 2018

nicolas-grekas reviewed May 31, 2018

View reviewed changes

Finetune exception message

f3e01e0

nicolas-grekas mentioned this pull request May 31, 2018

[4.1] The "parameter_bag" service is not available. #27436

Closed

chalasr reviewed May 31, 2018

View reviewed changes

curry684 added 2 commits May 31, 2018 17:42

Remove reference to upgrading DI as FrameworkBundle already required …

3dec31d

…^4.1

And Fabbot disagrees again with my spaces between all operators

90222db

carsonbot removed the Status: Needs Review label May 31, 2018

weaverryan approved these changes May 31, 2018

View reviewed changes

carsonbot added the Status: Reviewed label May 31, 2018

nicolas-grekas reviewed May 31, 2018

View reviewed changes

20uf approved these changes Jun 1, 2018

View reviewed changes

Change exception message once again

b34f72d

nicolas-grekas reviewed Jun 2, 2018

View reviewed changes

xabbuh approved these changes Jun 2, 2018

View reviewed changes

More message finetuning

1f400f6

nicolas-grekas approved these changes Jun 3, 2018

View reviewed changes

dunglas approved these changes Jun 4, 2018

View reviewed changes

chalasr approved these changes Jun 4, 2018

View reviewed changes

chalasr closed this Jun 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DX] Improve exception message when AbstractController::getParameter fails #27443

[DX] Improve exception message when AbstractController::getParameter fails #27443

[DX] Improve exception message when AbstractController::getParameter fails #27443

[DX] Improve exception message when AbstractController::getParameter fails #27443

Conversation

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

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