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#21642
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
xabbuh commentedFeb 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 |
xabbuh commentedFeb 17, 2017
Tests pass on |
| ; | ||
| $container | ||
| ->register('foo','\stdClass') |
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.
should bebar no?
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.
indeed, good catch
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.
isn't it weird the tests still passed with that?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This was a case where the circular reference wasworking, 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
stof commentedFeb 17, 2017
-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 |
stof commentedFeb 17, 2017
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. |
theofidry commentedFeb 17, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 =newX();$x2 =newX();$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 commentedFeb 17, 2017
stof commentedFeb 17, 2017
@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) |
xabbuh commentedApr 3, 2017
Closing as this is not the solution for the problem. But we still have#19362 that reminds us of the real issue. |