[DependencyInjection] check for circular refs caused by method calls #21642

Open
wants to merge 1 commit into
from

Projects

None yet

5 participants

@xabbuh
Member
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
@xabbuh xabbuh changed the base branch to symfony:2.7 from symfony:master Feb 17, 2017
@xabbuh
Member
xabbuh commented Feb 17, 2017

Tests pass on master too now (see https://travis-ci.org/symfony/symfony/builds/202523603) thanks to the changes @nicolas-grekas made in #21639.

+ ;
+
+ $container
+ ->register('foo', '\stdClass')
@theofidry
theofidry Feb 17, 2017 Contributor

should be bar no?

@xabbuh
xabbuh Feb 17, 2017 Member

indeed, good catch

@theofidry
theofidry Feb 17, 2017 Contributor

isn't it weird the tests still passed with that?

@xabbuh
xabbuh Feb 17, 2017 Member

Well, a circular reference is still a circular reference. I just wanted to explicitly have one with an intermediate service.

@xabbuh xabbuh check for circular refs caused by method calls
ab1ba1d
- $instance->setFoo($this->get('foo_with_inline'));
-
- return $instance;
+ return $this->services['baz'] = new \Baz();
@stof
stof Feb 17, 2017 Member

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

@stof
Member
stof commented Feb 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
Member
stof commented Feb 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
Contributor
theofidry commented Feb 17, 2017 edited

@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
Member
xabbuh commented Feb 17, 2017

Our latest releases already have a BC break because of this.

@stof I don't know what you mean. The commit that reverted the previous PR (68d6415) was part of the 3.2.4 release.

@stof
Member
stof commented Feb 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)

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Feb 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment