Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] check for circular refs caused by method calls#21436
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
stof commentedJan 27, 2017
Clearly not acceptable without tests |
xabbuh commentedJan 27, 2017
@stof Sure, I just wanted to make sure first that there is no reason I did not think of that this is a stupid solution. |
20670f6 toee5881bComparestof commentedJan 27, 2017
Please add a test ensuring that circular references in method calls stay allowed when inlining the services (i.e. we can instantiate both objects and then make method calls on them before leaving the |
xabbuh commentedJan 28, 2017
@stof Not sure I understand what you mean. Do you have an example of what you have in mind? |
| ; | ||
| $container | ||
| ->register('baz','Baz') | ||
| ->addMethodCall('setFoo',array(newReference('foo_with_inline'))) |
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.
Why removing this ?
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.
Readding the method call leads to a circular reference detected by the added pass (foo_with_inline ->inlined ->baz ->foo_with_inline).
fabpot commentedFeb 16, 2017
Thank you@xabbuh. |
…thod calls (xabbuh)This PR was merged into the 2.7 branch.Discussion----------[DependencyInjection] check for circular refs caused by method calls| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#19362| License | MIT| Doc PR |Before we check for circular references, dependencies coming from method calls are not part of the dependency graph. That why the pass is not able to detect circular references like the one described in#19362 during compilation of the container.If we add another check after all the optimisation passes have been processed, we should be able to detect these circular references too.Commits-------fe4f7ec check for circular refs caused by method calls
fabpot commentedFeb 16, 2017
@xabbuh Can you have a look at the master branch as it breaks |
xabbuh commentedFeb 16, 2017
I am looking into it. |
fabpot commentedFeb 16, 2017
@xabbuh What about reverting it for now? |
xabbuh commentedFeb 16, 2017
@fabpot Fine for me. I try to fix the tests tomorrow and can resubmit this PR then if you agree. |
fabpot commentedFeb 16, 2017
ok, reverted for now, until we have a fix for the issue on master. |
* 2.7: Revert "bug#21436 [DependencyInjection] check for circular refs caused by method calls (xabbuh)" Static code analysis with Php Inspections (EA Extended) [VarDumper] Added missing persistent stream cast
* 2.8: Revert "bug#21436 [DependencyInjection] check for circular refs caused by method calls (xabbuh)" Static code analysis with Php Inspections (EA Extended) [VarDumper] Added missing persistent stream cast
* 3.2: Revert "bug#21436 [DependencyInjection] check for circular refs caused by method calls (xabbuh)" Static code analysis with Php Inspections (EA Extended) [VarDumper] Added missing persistent stream cast remove unused translation file reverted usage of isNan
xabbuh commentedFeb 17, 2017
Resubmitted in#21642 (I opened the PR against |
stof commentedFeb 17, 2017
@fabpot this PR has been reverted, but the revert is not part of the release. Should we do another set of releases including it rather than waiting 1 month with releases breaking existing projects ? |
xabbuh commentedFeb 17, 2017
xabbuh commentedFeb 17, 2017
The changelog is misleading as it doesn't consider the revert. |
stof commentedFeb 17, 2017
well, this explains it. |
Before we check for circular references, dependencies coming from method calls are not part of the dependency graph. That why the pass is not able to detect circular references like the one described in#19362 during compilation of the container.
If we add another check after all the optimisation passes have been processed, we should be able to detect these circular references too.