[DependencyInjection] check for circular refs caused by method calls #21436
Conversation
|
Clearly not acceptable without tests |
|
@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 to
ee5881b
Compare
|
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 |
|
@stof Not sure I understand what you mean. Do you have an example of what you have in mind? |
| @@ -64,7 +64,6 @@ | |||
| ; | |||
| $container | |||
| ->register('baz', 'Baz') | |||
| ->addMethodCall('setFoo', array(new Reference('foo_with_inline'))) | |||
There was a problem hiding this comment.
Readding the method call leads to a circular reference detected by the added pass (foo_with_inline -> inlined -> baz -> foo_with_inline).
|
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
|
@xabbuh Can you have a look at the master branch as it breaks |
|
I am looking into it. |
|
@xabbuh What about reverting it for now? |
|
@fabpot Fine for me. I try to fix the tests tomorrow and can resubmit this PR then if you agree. |
|
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
|
Resubmitted in #21642 (I opened the PR against |
|
@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 ? |
|
The changelog is misleading as it doesn't consider the revert. |
|
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.