[DI] Add runtime service exceptions to improve the error message when controller arguments cannot be injected #26627

nicolas-grekas · 2018-03-21T21:49:51Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#23997
License	MIT
Doc PR	-

stof · 2018-03-22T11:30:29Z

src/Symfony/Component/DependencyInjection/Dumper/YamlDumper.php

@@ -266,6 +266,7 @@ private function getServiceCall(string $id, Reference $reference = null): string
    {
        if (null !== $reference) {
            switch ($reference->getInvalidBehavior()) {
+                case ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_DEFINITION:


I would break immediately instead of doing a fallthrough to the break in the next case.

weaverryan · 2018-03-22T12:45:34Z

I've tested this out - it works really well. Wow :p. The only imperfection (for the main use-case of controller args) is that the message contains only the controller class, not the method. I can see technically why that is - it's tricky. But, is this possible by either passing some extra context information from RegisterControllerArgumentLocatorsPass or by catching the RuntimeException in ServiceValueResolver and adding more context with a new exception?

stof · 2018-03-22T13:48:30Z

what happens if multiple controllers are referencing the same broken typehint ?

nicolas-grekas · 2018-03-22T14:16:42Z

~~PR embeds #26636 until it's merged.~~
@stof I was fixing that while you commented :)
@weaverryan I'm surprised you did not ask for the name of the argument also. So I added it ;)

stof · 2018-03-22T18:55:33Z

src/Symfony/Component/DependencyInjection/Compiler/ResolveInvalidReferencesPass.php

+            if (ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE === $invalidBehavior && $value instanceof TypedReference) {
+                $e = new ServiceNotFoundException($id = (string) $value, $this->currentId);
+
+                $this->container->register($id, $value->getType())


what if a next processed service was also trying to reference this service in the container, with a stricter behavior ? The service is now defined and will throw only at runtime.

this cannot happen between ResolveInvalidReferencesPass and CheckReferenceValidityPass, so that this case doesn't exist AFAIK

I mean, next services processed by this same compiler pass (this logic is in a loop over services)

Ok, got it. Better now?

stof · 2018-03-22T18:57:43Z

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

+            $message = $this->createTypeNotFoundMessage($value, 'it');
+
+            if (ContainerBuilder::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE === $value->getInvalidBehavior()) {
+                $this->container->register((string) $value, $value->getType())


a next service referencing the same value in a stricter way would now use this service as it exists. This looks wrong to me.

and even if it also use RUNTIME_EXCEPTION_ON_INVALID_REFERENCE and so would be fine with a throwing definition, it would still need a different error message for the other service, and so a different throwing definition.

The behavior is on the reference, not on the definition. DefinitionErrorExceptionPass will throw if any references exist that are not RUNTIME_EXCEPTION_ON_INVALID_REFERENCE.
Or I missed your point.

yes, the behavior is on the reference. but when processing a next autowired service, the service will exist in the container (due to registering it here). But then, when loading the second service at runtime, this errored definition will be loaded, triggering an exception talking about the first service (as that's the one for which this service was registered).

The issue is that even though the behavior is defined at the reference level, the error is registered at the definition level (and so multiple references will share the same definition)

please add tests covering this, to ensure that we keep getting warned with the right error message in the future, as this is a tricky case.

I added a comment instead.

stof · 2018-03-23T14:01:25Z

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

+            $message = $this->createTypeNotFoundMessage($value, 'it');
+
+            if (ContainerBuilder::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE === $value->getInvalidBehavior()) {
+                $this->container->register($id = sprintf('errored.%s.%s', $this->currentId, (string) $value), $value->getType())


As we consider service ids starting with a underscore as reserved since 4.0 (at least in the YAML file loader, not sure about the XML one and the PHP DSL), I suggest using _errored. as prefix instead to reduce the likeliness of conflicts.

nicolas-grekas · 2018-03-24T16:55:10Z

Comments addressed.

fabpot · 2018-03-28T18:04:50Z

Thank you @nicolas-grekas.

…or message when controller arguments cannot be injected (nicolas-grekas) This PR was merged into the 4.1-dev branch. Discussion ---------- [DI] Add runtime service exceptions to improve the error message when controller arguments cannot be injected | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23997 | License | MIT | Doc PR | - ![image](https://user-images.githubusercontent.com/243674/37775694-e5c9814c-2de3-11e8-8290-8fd05086da28.png) Commits ------- 9e8e063 [DI] Add ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE

nicolas-grekas added this to the 4.1 milestone Mar 21, 2018

carsonbot added Status: Needs Review DependencyInjection Feature labels Mar 21, 2018

nicolas-grekas force-pushed the di-runtime-exception branch 6 times, most recently from 412866c to 15bbba4 Compare March 22, 2018 11:04

stof reviewed Mar 22, 2018

View reviewed changes

nicolas-grekas force-pushed the di-runtime-exception branch from 15bbba4 to 8ee6428 Compare March 22, 2018 11:47

nicolas-grekas changed the title ~~[DI] Add ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_DEFINITION~~ [DI] Add ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE Mar 22, 2018

nicolas-grekas force-pushed the di-runtime-exception branch from 8ee6428 to 754bccf Compare March 22, 2018 14:15

nicolas-grekas force-pushed the di-runtime-exception branch from 754bccf to cf3ec2e Compare March 22, 2018 14:57

stof requested changes Mar 22, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Mar 22, 2018

nicolas-grekas added Status: Needs Review and removed Status: Needs Work labels Mar 22, 2018

nicolas-grekas force-pushed the di-runtime-exception branch 3 times, most recently from 29c6abc to 5b0d4f3 Compare March 23, 2018 11:35

stof reviewed Mar 23, 2018

View reviewed changes

nicolas-grekas force-pushed the di-runtime-exception branch from 5b0d4f3 to f012c9a Compare March 24, 2018 16:52

[DI] Add ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE

9e8e063

nicolas-grekas force-pushed the di-runtime-exception branch from f012c9a to 9e8e063 Compare March 24, 2018 16:54

nicolas-grekas changed the title ~~[DI] Add ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE~~ [DI] Add runtime service exceptions to improve the error message when controller arguments cannot be injected Mar 26, 2018

stof approved these changes Mar 28, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Mar 28, 2018

fabpot approved these changes Mar 28, 2018

View reviewed changes

fabpot merged commit 9e8e063 into symfony:master Mar 28, 2018

nicolas-grekas deleted the di-runtime-exception branch April 2, 2018 12:07

javiereguiluz mentioned this pull request Apr 20, 2018

[DependencyInjection] Add runtime service exceptions symfony/symfony-docs#9648

Closed

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

nicolas-grekas mentioned this pull request Jul 1, 2018

[DI] ContainerInterface lacks documentation regarding behavior constants symfony/symfony-docs#8759

Closed

alamirault mentioned this pull request Feb 13, 2023

[DependencyInjection] Document Container invalid behavior symfony/symfony-docs#17903

Merged

MatTheCat mentioned this pull request Jun 20, 2023

#[MapRequestPayload] does not work if a DTO contains **only** nested DTOs #50708

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Add runtime service exceptions to improve the error message when controller arguments cannot be injected #26627

[DI] Add runtime service exceptions to improve the error message when controller arguments cannot be injected #26627

nicolas-grekas commented Mar 21, 2018 •

edited

Loading

stof Mar 22, 2018

nicolas-grekas Mar 22, 2018

weaverryan commented Mar 22, 2018

stof commented Mar 22, 2018

nicolas-grekas commented Mar 22, 2018 •

edited

Loading

stof Mar 22, 2018

nicolas-grekas Mar 22, 2018

stof Mar 23, 2018

nicolas-grekas Mar 23, 2018

stof Mar 22, 2018

stof Mar 22, 2018

nicolas-grekas Mar 22, 2018

stof Mar 23, 2018 •

edited

Loading

nicolas-grekas Mar 23, 2018

stof Mar 23, 2018

nicolas-grekas Mar 24, 2018

stof Mar 23, 2018

nicolas-grekas Mar 24, 2018

nicolas-grekas commented Mar 24, 2018

fabpot commented Mar 28, 2018

[DI] Add runtime service exceptions to improve the error message when controller arguments cannot be injected #26627

[DI] Add runtime service exceptions to improve the error message when controller arguments cannot be injected #26627

Conversation

nicolas-grekas commented Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Mar 22, 2018

stof commented Mar 22, 2018

nicolas-grekas commented Mar 22, 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

stof Mar 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

nicolas-grekas commented Mar 24, 2018

fabpot commented Mar 28, 2018

nicolas-grekas commented Mar 21, 2018 •

edited

Loading

nicolas-grekas commented Mar 22, 2018 •

edited

Loading

stof Mar 23, 2018 •

edited

Loading