Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] Adding an exception on circular reference#28452
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
nicolas-grekas left a comment
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.
Thank you for working on this!
Here are some comments to help move forward.
| thrownewBadMethodCallException('Adding definition to a compiled container is not allowed'); | ||
| } | ||
| if ($definitioninstanceof ChildDefinition &&$id ===$definition->getParent()) { |
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 suggest moving the logic toResolveChildDefinitionsPass.
Also, can't the loop involve intermediary ids? A => B => C => A?
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.
Thanks for your feedback.
I can move it toResolveInstanceofConditionalsPass (here), but I think it's not a good option too...
Currently I can't find the way to move it toResolveChildDefinitionsPass. If you have an idea...
| * | ||
| * @author Nicolas LEFEVRE <weblefevre@gmail.com> | ||
| */ | ||
| class ArgumentCircularReferenceExceptionextends RuntimeException |
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.
if we want this to make it into a bug fix release, we should not add any new class in the PR (otherwise that'd be a smell for a new feature).
Instead, what about throwing a ServiceCircularReferenceException?
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, throwingServiceCircularReferenceException should be a good idea...
nicolas-grekas commentedSep 17, 2018
Closing in favor of#28480, which is closer to the final patch. |
When a
ChildDefinitionreferences itself as parent (or as ancestor in case of longer cycle), an infinite loop happens in the resolution logic.It's a try to fix this "bug", thank's for your feedback...