Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Add runtime service exceptions to improve the error message when controller arguments cannot be injected#26627
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
412866c to15bbba4Compare| { | ||
| if (null !==$reference) { | ||
| switch ($reference->getInvalidBehavior()) { | ||
| case ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_DEFINITION: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would break immediately instead of doing a fallthrough to the break in the next case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
updated
15bbba4 to8ee6428Compareweaverryan commentedMar 22, 2018
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 controllerclass, 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 |
stof commentedMar 22, 2018
what happens if multiple controllers are referencing the same broken typehint ? |
8ee6428 to754bccfComparenicolas-grekas commentedMar 22, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
754bccf tocf3ec2eCompare| if (ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE ===$invalidBehavior &&$valueinstanceof TypedReference) { | ||
| $e =newServiceNotFoundException($id = (string)$value,$this->currentId); | ||
| $this->container->register($id,$value->getType()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this cannot happen betweenResolveInvalidReferencesPass andCheckReferenceValidityPass, so that this case doesn't exist AFAIK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I mean, next services processed by this same compiler pass (this logic is in a loop over services)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, got it. Better now?
| $message =$this->createTypeNotFoundMessage($value,'it'); | ||
| if (ContainerBuilder::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE ===$value->getInvalidBehavior()) { | ||
| $this->container->register((string)$value,$value->getType()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
a next service referencing the same value in a stricter way would now use this service as it exists. This looks wrong to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 adifferent throwing definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The behavior is on the reference, not on the definition.DefinitionErrorExceptionPass will throw if any references exist that are notRUNTIME_EXCEPTION_ON_INVALID_REFERENCE.
Or I missed your point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes, the behavior is on the reference. but when processing anext autowired service, the service will exist in the container (due to registering it here). But then, when loading thesecond service at runtime, this errored definition will be loaded, triggering an exception talking about thefirst service (as that's the one for which this service was registered).
The issue is that even though thebehavior is defined at the reference level, theerror is registered at the definition level (and so multiple references will share the same definition)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok, fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added a comment instead.
29c6abc to5b0d4f3Compare| $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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
5b0d4f3 tof012c9aComparef012c9a to9e8e063Comparenicolas-grekas commentedMar 24, 2018
Comments addressed. |
fabpot commentedMar 28, 2018
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 | -Commits-------9e8e063 [DI] Add ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE
Uh oh!
There was an error while loading.Please reload this page.