Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
nicolas-grekas merged 1 commit intosymfony:3.4fromnicolas-grekas:di-fix-circular
Nov 29, 2017

Conversation

@nicolas-grekas
Copy link
Member

QA
Branch?3.4
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
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.

@sroze
Copy link
Contributor

@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
Copy link
Contributor

deguif commentedNov 28, 2017
edited
Loading

Here's a reproducing test case.
Thanks@nicolas-grekas for your time yesterday on this. I was wrong, the service we were playing with were marked asshared = false by a compiler pass. So the bug you encounter by declaring an unshared service is the same.

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
Copy link
Member

Circular setter injection for shared services works fine in some cases, as the instance is registered in$this->servicesbefore calling the setter on it. But this is not always the case. Here are a few case which break things:

  • injecting the service in the constructor of the main service (as done here forfoo_bar), as this requires building the dependency before injecting it in the setter
  • using only non-shared services in the cycle (we need a shared service in the cycle to break the instantiation loop)
  • using a non-shared service as main service would create weird things (as this instance would not be registered, and so the loop would instantiate it again in the cycle). Note that the first shared service reached in the cycle inherits the restrictions of the "main" service.

What I called "main" services are all the services which can trigger the beginning of the cycle:
Here is what identifies the "main" services of a cycle, for which we have the restriction about not involving its constructor in the cycle:

  • all public services (as they can be retrieved throughget)
  • all shared services referenced from outside this cycle (as the cycle can start when resolving the reference)

@stof
Copy link
Member

However, we need to be sure we don't break BC for cases working fine.

@stof
Copy link
Member

and this means we should write test covering the working case to, to avoid breaking it

@deguif
Copy link
Contributor

Thanks@stof for your explanation
I got this issue when upgrading from Symfony 3.3 to 3.4.
So currently there is a BC break as I'm not able to upgrade to Symfony 3.4 without adapting the service definitions.

@nicolas-grekas
Copy link
MemberAuthor

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.

@sroze
Copy link
Contributor

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> .
xabbuh reacted with thumbs up emoji

Copy link
MemberAuthor

@nicolas-grekasnicolas-grekas left a 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())
Copy link
MemberAuthor

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()
Copy link
MemberAuthor

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);
Copy link
MemberAuthor

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

@nicolas-grekasnicolas-grekas merged commitde5eecc intosymfony:3.4Nov 29, 2017
nicolas-grekas added a commit that referenced this pull requestNov 29, 2017
…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-grekasnicolas-grekas deleted the di-fix-circular branchNovember 29, 2017 16:58
@nicolas-grekas
Copy link
MemberAuthor

@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.
But any review is still welcomed, I'll respond quickly to any report/comments until tomorrow, before the release.

mateuszsip, deguif, and rvanlaak reacted with hooray emoji

This was referencedNov 30, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

5 participants

@nicolas-grekas@sroze@deguif@stof@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp