Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Di circular dependancies #5447

Closed
wants to merge 6 commits into from

5 participants

Nick Peirson Marco Pivetta Matthew Weier O'Phinney Maks3w Abdul Malik Ikhsan
Nick Peirson

I've noticed a situation where dependencies are sometimes incorrectly identified as circular when they're not.

If we a class and use the alias as parameters in other aliases multiple times for the same class we get the following exception:

Zend\Di\Exception\CircularDependencyException: Circular dependency detected: ZendTest\Di\TestAsset\CircularClasses\Y depends on ZendTest\Di\TestAsset\CircularClasses\Y and viceversa

If we get an instance of the all of the required aliased classes first, we don't get this exception. To help resolve this I've written a couple of tests that succinctly show the problem. We're currently working round this by instantiating the intermediary classes, which is not ideal, so a fix to this would be much appreciated. Please let me know if there's anything else I can do to help debug.

Marco Pivetta
Collaborator

@nickpeirson do not merge master into your stuff. Rebase instead.

If you can, please squash the PR

tests/ZendTest/Di/DiTest.php
((8 lines not shown))
+ $di->instanceManager()->addAlias('YA', 'ZendTest\Di\TestAsset\CircularClasses\Y');
+ $di->instanceManager()->addAlias('YB', 'ZendTest\Di\TestAsset\CircularClasses\Y', array('y' => 'YA'));
+ $di->instanceManager()->addAlias('YC', 'ZendTest\Di\TestAsset\CircularClasses\Y', array('y' => 'YB'));
+
+ return $di;
+ }
+
+ /**
+ * Test for correctly identifying no Circular Dependencies (case 1)
+ *
+ * YC -> YB, YB -> YA
+ * @group CircularDependencyCheck
+ */
+ public function testNoCircularDependancyDetectedIfWeGetIntermediaryClass()
+ {
+ $di = $this->configureNoneCircularDependancyTests();
Marco Pivetta Collaborator
Ocramius added a note

Dependancy?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Marco Pivetta
Collaborator

@nickpeirson not sure I follow...

YC -> YB, YB -> YA, YA -> YC is obviously a circular dependency issue. It can be solved only by handling one of the injections via a setter.

Do you imply that the problem is that dependencies are computed on aliases instead of the resolved "real" instances? IIRC, each alias is a new instance per-se, and not a reference to an already instantiated object.

Nick Peirson

I'll tidy up the PR shortly.

I completely agree YC -> YB, YB -> YA, YA -> YC, isn't expected to work without using setters.

The problem is that the first test works successfully with DI able to get YC, the second fails with the exception above.

It appears that $di->get('YB'); changes the internal state of DI somewhere that allows it to successfully $di->get('YB');. My expectation was that both scenarios would work or, less desirably, that both would fail. It was unexpected that the second scenario would fail with the first passing.

The DI config above should be equivalent to:

$YA = new Y;
$YB = new Y($YA);
$YC = new Y($YB);

which has no circular dependency.

Nick Peirson

PR squashed, typos fixed

tests/ZendTest/Di/TestAsset/CircularClasses/Y.php
@@ -0,0 +1,19 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Di

remove @package

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Nick Peirson

Is there anything else I can do to clarify the situation and unexpected behaviour we're encountering, or does that last explanation make it clear enough? I'm don't expect an immediate fix, but I want to make sure that there's enough information available here so that the problem is understood when someone does get round to it.

Nick Peirson nickpeirson Add a check for aliases when checking for circular dependencies. If a…
…liases are used, assume the dependencies aren't circular unless the same alias is used multiple times.
6db79b4
Nick Peirson

Just been working through this to see what the difference between the two scenarios is. The exception in the second case comes from this check:

class Di implements DependencyInjectionInterface
{
//...snip...
    protected function resolveMethodParameters($class, $method, array $callTimeUserParams, $alias, $methodRequirementType, $isInstantiator = false)
    {
    //...snip...
                if ($isInstantiator && in_array($computedParams['retrieval'][$fqParamPos][1], $this->currentDependencies)) {
                    throw new Exception\CircularDependencyException(
                        "Circular dependency detected: $class depends on {$value[1]} and viceversa"
                    );
                }

This check is somewhat simplistic and ignores the fact that while the resolved class name may already be in the current dependencies list it may be aliased and configured differently and in fact not be a circular dependency at all, as in this case.

The reason the first test doesn't fail is that the check for circular dependencies comes after the check to see if there's already a shared instance. In the first test case there is a shared instance so it happily returns that before being told it's trying to do something impossible.

Removing that exception being thrown and running that test passes, however, unsurprisingly, it blows up true circular dependencies. This indicates to me that if the conditional can be improved to detect this scenario, Di will be able to cope.

Nick Peirson

Not sure what to make of the results of the last travis build. The build against 5.3.3 was successful, but the rest failed with CS violations and, interestingly, different CS violations. Running bin/check-cs.sh locally returns Coding standards check passed!, so it all seems good to me.

Matthew Weier O'Phinney

@Ocramius can you do a final review and let me know if it's ready to merge, please?

Maks3w
Collaborator

@Ocramius ping

Marco Pivetta
Collaborator

@Maks3w this is mergeable. Wish it could be prettier, but it's not really possible atm.

tests/ZendTest/Di/TestAsset/CircularClasses/Y.php
@@ -0,0 +1,18 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)

now 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Maks3w Maks3w added this to the 2.2.6 milestone
Maks3w Maks3w added the Di label
Matthew Weier O'Phinney weierophinney referenced this pull request from a commit
Matthew Weier O'Phinney weierophinney Merge branch 'hotfix/5447' into develop
Forward port #5447
71d965e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 8, 2013
  1. Nick Peirson
  2. Nick Peirson

    Removed @package

    nickpeirson authored
Commits on Jan 2, 2014
  1. Nick Peirson

    Add a check for aliases when checking for circular dependencies. If a…

    nickpeirson authored
    …liases are used, assume the dependencies aren't circular unless the same alias is used multiple times.
Commits on Jan 3, 2014
  1. Nick Peirson
  2. Nick Peirson
Commits on Feb 24, 2014
  1. Nick Peirson

    Update copyright date

    nickpeirson authored
This page is out of date. Refresh to see the latest.
38 library/Zend/Di/Di.php
View
@@ -42,6 +42,13 @@ class Di implements DependencyInjectionInterface
protected $currentDependencies = array();
/**
+ * All the dependenent aliases
+ *
+ * @var array
+ */
+ protected $currentAliasDependenencies = array();
+
+ /**
* All the class references [dependency][source]
*
* @var array
@@ -767,13 +774,21 @@ protected function resolveMethodParameters($class, $method, array $callTimeUserP
$resolvedParams[$index] = $computedParams['value'][$fqParamPos];
} elseif (isset($computedParams['retrieval'][$fqParamPos])) {
// detect circular dependencies! (they can only happen in instantiators)
- if ($isInstantiator && in_array($computedParams['retrieval'][$fqParamPos][1], $this->currentDependencies)) {
- throw new Exception\CircularDependencyException(
- "Circular dependency detected: $class depends on {$value[1]} and viceversa"
- );
+ if ($isInstantiator && in_array($computedParams['retrieval'][$fqParamPos][1], $this->currentDependencies)
+ && (!isset($alias) || in_array($computedParams['retrieval'][$fqParamPos][0], $this->currentAliasDependenencies))
+ ) {
+ $msg = "Circular dependency detected: $class depends on {$value[1]} and viceversa";
+ if (isset($alias)) {
+ $msg .= " (Aliased as $alias)";
+ }
+ throw new Exception\CircularDependencyException($msg);
}
array_push($this->currentDependencies, $class);
+ if(isset($alias)) {
+ array_push($this->currentAliasDependenencies, $alias);
+ }
+
$dConfig = $this->instanceManager->getConfig($computedParams['retrieval'][$fqParamPos][0]);
try {
@@ -786,6 +801,9 @@ protected function resolveMethodParameters($class, $method, array $callTimeUserP
if ($methodRequirementType & self::RESOLVE_STRICT) {
//finally ( be aware to do at the end of flow)
array_pop($this->currentDependencies);
+ if(isset($alias)) {
+ array_pop($this->currentAliasDependenencies);
+ }
// if this item was marked strict,
// plus it cannot be resolve, and no value exist, bail out
throw new Exception\MissingPropertyException(sprintf(
@@ -797,6 +815,9 @@ protected function resolveMethodParameters($class, $method, array $callTimeUserP
} else {
//finally ( be aware to do at the end of flow)
array_pop($this->currentDependencies);
+ if(isset($alias)) {
+ array_pop($this->currentAliasDependenencies);
+ }
return false;
}
} catch (ServiceManagerException $e) {
@@ -804,6 +825,9 @@ protected function resolveMethodParameters($class, $method, array $callTimeUserP
if ($methodRequirementType & self::RESOLVE_STRICT) {
//finally ( be aware to do at the end of flow)
array_pop($this->currentDependencies);
+ if(isset($alias)) {
+ array_pop($this->currentAliasDependenencies);
+ }
// if this item was marked strict,
// plus it cannot be resolve, and no value exist, bail out
throw new Exception\MissingPropertyException(sprintf(
@@ -815,10 +839,16 @@ protected function resolveMethodParameters($class, $method, array $callTimeUserP
} else {
//finally ( be aware to do at the end of flow)
array_pop($this->currentDependencies);
+ if(isset($alias)) {
+ array_pop($this->currentAliasDependenencies);
+ }
return false;
}
}
array_pop($this->currentDependencies);
+ if(isset($alias)) {
+ array_pop($this->currentAliasDependenencies);
+ }
} elseif (!array_key_exists($fqParamPos, $computedParams['optional'])) {
if ($methodRequirementType & self::RESOLVE_STRICT) {
// if this item was not marked as optional,
111 tests/ZendTest/Di/DiTest.php
View
@@ -447,6 +447,117 @@ public function testNewInstanceThrowsExceptionWhenEnteringInMiddleOfCircularDepe
$di->newInstance('ZendTest\Di\TestAsset\CircularClasses\D');
}
+ protected function configureNoneCircularDependencyTests()
+ {
+ $di = new Di();
+
+ $di->instanceManager()->addAlias('YA', 'ZendTest\Di\TestAsset\CircularClasses\Y');
+ $di->instanceManager()->addAlias('YB', 'ZendTest\Di\TestAsset\CircularClasses\Y', array('y' => 'YA'));
+ $di->instanceManager()->addAlias('YC', 'ZendTest\Di\TestAsset\CircularClasses\Y', array('y' => 'YB'));
+
+ return $di;
+ }
+
+ /**
+ * Test for correctly identifying no Circular Dependencies (case 1)
+ *
+ * YC -> YB, YB -> YA
+ * @group CircularDependencyCheck
+ */
+ public function testNoCircularDependencyDetectedIfWeGetIntermediaryClass()
+ {
+ $di = $this->configureNoneCircularDependencyTests();
+
+ $yb = $di->get('YB');
+ $this->assertInstanceOf('ZendTest\Di\TestAsset\CircularClasses\Y', $yb);
+ $yc = $di->get('YC');
+ $this->assertInstanceOf('ZendTest\Di\TestAsset\CircularClasses\Y', $yc);
+ }
+
+ /**
+ * Test for correctly identifying no Circular Dependencies (case 2)
+ *
+ * YC -> YB, YB -> YA
+ * @group CircularDependencyCheck
+ */
+ public function testNoCircularDependencyDetectedIfWeDontGetIntermediaryClass()
+ {
+ $di = $this->configureNoneCircularDependencyTests();
+
+ $yc = $di->get('YC');
+ $this->assertInstanceOf('ZendTest\Di\TestAsset\CircularClasses\Y', $yc);
+ }
+
+ /**
+ * Test for correctly identifying a Circular Dependency in aliases (case 3)
+ *
+ * YA -> YB, YB -> YA
+ * @group CircularDependencyCheck
+ */
+ public function testCircularDependencyDetectedInAliases()
+ {
+ $di = new Di();
+
+ $di->instanceManager()->addAlias('YA', 'ZendTest\Di\TestAsset\CircularClasses\Y', array('y' => 'YC'));
+ $di->instanceManager()->addAlias('YB', 'ZendTest\Di\TestAsset\CircularClasses\Y', array('y' => 'YA'));
+ $di->instanceManager()->addAlias('YC', 'ZendTest\Di\TestAsset\CircularClasses\Y', array('y' => 'YB'));
+
+ $this->setExpectedException(
+ 'Zend\Di\Exception\CircularDependencyException',
+ 'Circular dependency detected: ZendTest\Di\TestAsset\CircularClasses\Y depends on ZendTest\Di\TestAsset\CircularClasses\Y and viceversa (Aliased as YA)'
+ );
+
+ $yc = $di->get('YC');
+ }
+
+ /**
+ * Test for correctly identifying a Circular Dependency with a self referencing alias
+ *
+ * YA -> YA
+ * @group CircularDependencyCheck
+ */
+ public function testCircularDependencyDetectedInSelfReferencingAlias()
+ {
+ $di = new Di();
+
+ $di->instanceManager()->addAlias(
+ 'YA',
+ 'ZendTest\Di\TestAsset\CircularClasses\Y',
+ array('y' => 'YA')
+ );
+
+ $this->setExpectedException(
+ 'Zend\Di\Exception\CircularDependencyException',
+ 'Circular dependency detected: ZendTest\Di\TestAsset\CircularClasses\Y depends on ZendTest\Di\TestAsset\CircularClasses\Y and viceversa (Aliased as YA)'
+ );
+
+ $y = $di->get('YA');
+ }
+
+ /**
+ * Test for correctly identifying a Circular Dependency with mixture of classes and aliases
+ *
+ * Y -> YA, YA -> Y
+ * @group CircularDependencyCheck
+ */
+ public function testCircularDependencyDetectedInMixtureOfAliasesAndClasses()
+ {
+ $di = new Di();
+
+ $di->instanceManager()->addAlias(
+ 'YA',
+ 'ZendTest\Di\TestAsset\CircularClasses\Y',
+ array('y' => 'ZendTest\Di\TestAsset\CircularClasses\Y')
+ );
+
+ $this->setExpectedException(
+ 'Zend\Di\Exception\CircularDependencyException',
+ 'Circular dependency detected: ZendTest\Di\TestAsset\CircularClasses\Y depends on ZendTest\Di\TestAsset\CircularClasses\Y and viceversa (Aliased as YA)'
+ );
+
+ $y = $di->get('ZendTest\Di\TestAsset\CircularClasses\Y', array('y' => 'YA'));
+ }
+
/**
* Fix for PHP bug in is_subclass_of
*
18 tests/ZendTest/Di/TestAsset/CircularClasses/Y.php
View
@@ -0,0 +1,18 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2014 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ */
+
+namespace ZendTest\Di\TestAsset\CircularClasses;
+
+class Y
+{
+ public function __construct(Y $y = null)
+ {
+
+ }
+}
Something went wrong with that request. Please try again.