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] Fix "almost-circular" dependencies handling#24822
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
56bf688 to71209deComparenicolas-grekas commentedNov 4, 2017
(fabbot failure is false positive) |
71209de to40c6f35Compareogizanagi commentedNov 5, 2017
That means |
nicolas-grekas commentedNov 5, 2017
@ogizanagi how that? I don't think that's the case. |
ogizanagi commentedNov 5, 2017
Just trying to dump BarCircular { +foobar: null}but that's actually the only way to solve the circular dependency AFAIU. Or did I miss something? |
40c6f35 to20aa347Comparenicolas-grekas commentedNov 5, 2017 • 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.
@ogizanagi I'm sorry but I'm not able to reproduce your dump. I added type hints on the classes to ensure that |
20aa347 to63d3cafCompare63d3caf tobeb4df7Compare
ogizanagi 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.
As discussed together on Slack, this is fine actually 👍
fabpot commentedNov 5, 2017
Thank you@nicolas-grekas. |
…grekas)This PR was merged into the 3.4 branch.Discussion----------[DI] Fix "almost-circular" dependencies handling| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#19362,#24775| License | MIT| Doc PR | -In a situation like the following one, we used to trigger a circular reference exception. But this was a false positive, as the reference is resolvable without hitting the circle. Fixing this exception could be considered as a new feature (because no existing config needs it, since it fails). But for 3.4, this should be considered a bug fix, as reported in#24775: not handling this situation now means creating broken service trees.``` php$containerBuilder = new ContainerBuilder();$container->register('foo', FooCircular::class)->setPublic(true) ->addArgument(new Reference('bar'));$container->register('bar', BarCircular::class) ->addMethodCall('addFoobar', array(new Reference('foobar')));$container->register('foobar', FoobarCircular::class) ->addArgument(new Reference('foo'));$foo = $containerBuilder->get('foo');```Commits-------beb4df7 [DI] Fix "almost-circular" dependencies handling
…grekas)This PR was merged into the 3.4 branch.Discussion----------[DI] Analyze setter-circular deps more precisely| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#24950| License | MIT| Doc PR | -This PR reverts the effect of#24828 and#24822 on fixtures, except for the new behavior these PRs introduced, which was mostly fine, but missed a few cases.This PR now uses the reference graph to precisely decide which services need circular dependency care, and does not touch the other ones.Commits-------9cc4a21 [DI] Analyze setter-circular deps more precisely
Uh oh!
There was an error while loading.Please reload this page.
In a situation like the following one, we used to trigger a circular reference exception. But this was a false positive, as the reference is resolvable without hitting the circle. Fixing this exception could be considered as a new feature (because no existing config needs it, since it fails). But for 3.4, this should be considered a bug fix, as reported in#24775: not handling this situation now means creating broken service trees.