-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
72576c9
to
877b660
Compare
877b660
to
4b27065
Compare
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.
@@ -1017,6 +1018,10 @@ public function setDefinition($id, Definition $definition) | |||
throw new BadMethodCallException('Adding definition to a compiled container is not allowed'); | |||
} | |||
|
|||
if ($definition instanceof 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 to ResolveChildDefinitionsPass
.
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 to ResolveInstanceofConditionalsPass
(here), but I think it's not a good option too...
Currently I can't find the way to move it to ResolveChildDefinitionsPass
. If you have an idea...
* | ||
* @author Nicolas LEFEVRE <weblefevre@gmail.com> | ||
*/ | ||
class ArgumentCircularReferenceException extends 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, throwing ServiceCircularReferenceException
should be a good idea...
Closing in favor of #28480, which is closer to the final patch. |
When a
ChildDefinition
references 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...