[DependencyInjection] check for circular refs caused by method calls #21642
Conversation
xabbuh
commented
Feb 17, 2017
| 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 |
|
Tests pass on |
| ; | ||
|
|
||
| $container | ||
| ->register('foo', '\stdClass') |
There was a problem hiding this comment.
isn't it weird the tests still passed with that?
There was a problem hiding this comment.
Well, a circular reference is still a circular reference. I just wanted to explicitly have one with an intermediate service.
| $instance->setFoo($this->get('foo_with_inline')); | ||
|
|
||
| return $instance; | ||
| return $this->services['baz'] = new \Baz(); |
There was a problem hiding this comment.
This was a case where the circular reference was working, as the service can be instantiated and registered in $this->services before the point triggering the call to the other service, meaning it will receive the instance.
Forbidding a case which was working previously is a BC break. So this is not acceptable, especially for a patch release
|
-1 for this PR as is. It removes a supported feature. Tests are passing now because you remove the test ensuring that this feature keeps working |
|
And given the potential to break existing projects, should we really merge this again in a patch release ? Our latest releases already have a BC break because of this. |
|
@xabbuh tbh I fail to see how this circular dependency is a problem in the first place, it sure can be a big smell, but it's a legitimate thing as well. For example if you have: class X {
private $y;
setY(self $y) {
$this->y = $y;
}
}
$x1 = new X();
$x2 = new X();
$x1->setY($x2);
$x2->setY($x1);it's perfectly ok in PHP, I don't see why you shouldn't be able to do this with the DIC. There is legitimate use cases for it (I don't have on top of my head though soz) |
|
@xabbuh see my comment in the issue giving more details about the work needed. This PR as is is not the right fix (it has false positives, which are worse than false negatives) |
|
Closing as this is not the solution for the problem. But we still have #19362 that reminds us of the real issue. |