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] Detect circular references with ChildDefinition parent#28480
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
Seb33300 commentedSep 16, 2018
Oups, didn't see that someone already worked on it:#28452 |
Seb33300 commentedSep 16, 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.
Just pushed a test case since it looks to be the good way to fix this issue regarding comments on the other pull request. I pushed on master because it is a very similar issue as the previous one I fixed (#28315) which was merged on master. |
| } | ||
| /** | ||
| * @expectedException \Symfony\Component\DependencyInjection\Exception\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.
could be worth adding an@expectedExceptionMessage: right now, I get:Service "a": Service "a": Service "a": Service "a": Circular reference detected for service "c", path: "c -> b -> a -> c"., which might need some tweaks :)
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.
hmm, that'sresolveDefinition prefixing the exception message. And the multiple prefixing looks really weird though.
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.
Oups, didn't see that. I just pushed a fix.
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.
still worth adding@expectedExceptionMessage :)
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.
Maybe@expectedExceptionMessageRegExp to test if exception message is not prefixed?
nicolas-grekas commentedSep 17, 2018
(for 3.4!) |
nicolas-grekas commentedSep 18, 2018
Thank you@Seb33300. |
…t (Seb33300)This PR was submitted for the master branch but it was squashed and merged into the 3.4 branch instead (closes#28480).Discussion----------[DI] Detect circular references with ChildDefinition parent| Q | A| ------------- | ---| Branch? | 3.4 (be careful when merging)| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#28312| License | MIT| Doc PR |I will provide a test case if the fix looks good for you :)Commits-------2a59c8e [DI] Detect circular references with ChildDefinition parent
Uh oh!
There was an error while loading.Please reload this page.
I will provide a test case if the fix looks good for you :)