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 circular reference when using setters#25180
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
sroze commentedNov 28, 2017
@nicolas-grekas you can imagine that we definitely need tests for that 😄 Where is the application you played with? Which branch is failing without this patch? |
deguif commentedNov 28, 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.
Here's a reproducing test case. Service declarations <serviceid="foo"class="Foo"shared="false"> <callmethod="setFooBar"> <argumenttype="service"id="foo_bar" /> </call></service><serviceid="foo_bar"class="FooBar"> <argumenttype="collection"> <argumenttype="service"id="foo" /> </argument></service> This will lead to this container service dump (which causes an infinite loop). <?phpuseSymfony\Component\DependencyInjection\Argument\RewindableGenerator;// This file has been auto-generated by the Symfony Dependency Injection Component for internal use.// Returns the private 'foo_bar' shared service.$a =new \Foo();$a->setFooBar(${($_ =isset($this->services['foo_bar']) ?$this->services['foo_bar'] :$this->load(__DIR__.'/getFooBarService.php')) &&false ?:'_'});return$this->services['foo_bar'] =new \FooBar(array(0 =>$a)); |
stof commentedNov 28, 2017
Circular setter injection for shared services works fine in some cases, as the instance is registered in
What I called "main" services are all the services which can trigger the beginning of the cycle:
|
stof commentedNov 28, 2017
However, we need to be sure we don't break BC for cases working fine. |
stof commentedNov 28, 2017
and this means we should write test covering the working case to, to avoid breaking it |
deguif commentedNov 28, 2017
Thanks@stof for your explanation |
be5bff3 tob6eebc7Comparenicolas-grekas commentedNov 28, 2017
The issue is deeper than anticipated: there are much more cases where we can generate code that loop infinitely. |
sroze commentedNov 28, 2017 via email
I you need help re the tests, ping me Nico …On Tue, 28 Nov 2017 at 19:21, Symfony 4 rulez ***@***.***> wrote: The issue is deeper than anticipated: there are much more cases where we can generate code that loop infinitely. I just pushed a patch that should fix that, but is missing many tests for now (so not all situations are proven as OK yet.) I'll finish tomorrow. If you want to have a look meanwhile, that's possible. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#25180 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxHEfdWXuzswvtmicLBeZef1WLGUqNYks5s7F1WgaJpZM4QsSGw> . |
b6eebc7 toed3c919Compare1a467d5 toe01466dComparee01466d tode5eeccCompare
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.
PR is ready. It's bigger that I anticipated, yet it's very important as it fixes the dumped container. On 3.3, the issue also exists, but is less likely to hit for two independent reasons:
- the dumped container on 3.3 still uses
$this->get()internally for public services, which has circular ref bundled - much more services are public
| } | ||
| privatefunctiondoGet($id,$invalidBehavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE) | ||
| privatefunctiondoGet($id,$invalidBehavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE,array &$inlineServices =array()) |
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.
When a reference is used several times to construct one service, only one instance of non-shared services should be created. That's already the case for the dumped container. Here is the fix for ContainerBuilder. Tested below, with "foobar4" case.
| /** | ||
| * @expectedException \Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException | ||
| */ | ||
| publicfunctiontestCircularReference() |
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 previous failure is not legitimate anymore: there is no circular reference here
| } | ||
| $this->assertStringEqualsFile(self::$fixturesPath.'/php/container_inline_requires.php',$dump); | ||
| $this->assertStringEqualsFile(self::$fixturesPath.'/php/services_inline_requires.php',$dump); |
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.
naming was inconsistent with other files in the folder
…ekas)This PR was merged into the 3.4 branch.Discussion----------[DI] Fix circular reference when using setters| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -I did not manage to make a reproducing test case, yet@deguif provided me an app that I could play with to debug and fix.Commits-------de5eecc [DI] Fix circular reference when using setters
nicolas-grekas commentedNov 29, 2017
@symfony/deciders I merged this PR because it's ready on my side, and it's important to have before 4.0.0, so that I wanted to allow as many ppl as possible to test it, which is easier now. |
I did not manage to make a reproducing test case, yet@deguif provided me an app that I could play with to debug and fix.