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 "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

Merged
fabpot merged 1 commit intosymfony:3.4fromnicolas-grekas:fix-circ-deps
Nov 5, 2017

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedNov 4, 2017
edited
Loading

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

$containerBuilder =newContainerBuilder();$container->register('foo', FooCircular::class)->setPublic(true)   ->addArgument(newReference('bar'));$container->register('bar', BarCircular::class)    ->addMethodCall('addFoobar',array(newReference('foobar')));$container->register('foobar', FoobarCircular::class)    ->addArgument(newReference('foo'));$foo =$containerBuilder->get('foo');

TomasVotruba reacted with thumbs up emoji
@nicolas-grekas
Copy link
MemberAuthor

(fabbot failure is false positive)

@ogizanagi
Copy link
Contributor

That meansFooCircular will get in its constructor aBarCircular instance not fully configured yet (withoutfoobar), right? Isn't it an issue?

@nicolas-grekas
Copy link
MemberAuthor

@ogizanagi how that? I don't think that's the case.

@ogizanagi
Copy link
Contributor

Just trying to dump$bar inFooCircular's constructor:

BarCircular {  +foobar: null}

but that's actually the only way to solve the circular dependency AFAIU. Or did I miss something?

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedNov 5, 2017
edited
Loading

@ogizanagi I'm sorry but I'm not able to reproduce your dump. I added type hints on the classes to ensure thatnull is not a possible value of thefoobar property, and everything still works.

Copy link
Contributor

@ogizanagiogizanagi left a 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
Copy link
Member

Thank you@nicolas-grekas.

@fabpotfabpot merged commitbeb4df7 intosymfony:3.4Nov 5, 2017
fabpot added a commit that referenced this pull requestNov 5, 2017
…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
@nicolas-grekasnicolas-grekas deleted the fix-circ-deps branchNovember 5, 2017 15:48
This was referencedNov 5, 2017
nicolas-grekas added a commit that referenced this pull requestNov 20, 2017
…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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

+1 more reviewer

@ogizanagiogizanagiogizanagi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

4 participants

@nicolas-grekas@ogizanagi@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp