Di circular dependancies #5447

Closed
wants to merge 6 commits into
from

Projects

None yet

5 participants

@nickpeirson
Contributor

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.

@Ocramius
Member
Ocramius commented Nov 8, 2013

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

If you can, please squash the PR

@Ocramius Ocramius commented on an outdated diff Nov 8, 2013
tests/ZendTest/Di/DiTest.php
+ $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();
@Ocramius
Ocramius Nov 8, 2013 Member

Dependancy?

@Ocramius
Member
Ocramius commented Nov 8, 2013

@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.

@nickpeirson
Contributor

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.

@nickpeirson
Contributor

PR squashed, typos fixed

@samsonasik samsonasik commented on an outdated diff Nov 8, 2013
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
@nickpeirson
Contributor

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.

@nickpeirson 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
@nickpeirson
Contributor

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.

@nickpeirson
Contributor

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.

@weierophinney
Member

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

@Maks3w
Member
Maks3w commented Feb 23, 2014

@Ocramius ping

@Ocramius
Member

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

@samsonasik samsonasik commented on an outdated diff Feb 23, 2014
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)
@samsonasik
samsonasik Feb 23, 2014 Contributor

now 2014

@Maks3w Maks3w added this to the 2.2.6 milestone Feb 23, 2014
@Maks3w Maks3w added the Di label Feb 23, 2014
@weierophinney weierophinney added a commit that closed this pull request Mar 3, 2014
@weierophinney weierophinney Merge branch 'hotfix/5447'
Close #5447
5cda9db
@weierophinney weierophinney added a commit that referenced this pull request Mar 3, 2014
@weierophinney weierophinney Merge branch 'hotfix/5447' into develop
Forward port #5447
71d965e
@weierophinney weierophinney added a commit to zendframework/zend-di that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge pull request zendframework/zendframework#5447 from nickpeirson/…
…DICircularDependancies

Di circular dependancies
114cc4d
@weierophinney weierophinney added a commit to zendframework/zend-di that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'hotfix/5447' 73816d4
@weierophinney weierophinney added a commit to zendframework/zend-di that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'hotfix/5447' into develop 0f36158
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment