Merge pull request #169 from mnapoli/bug/168

Container::has() should return false for un-mapped interfaces/abstract classes
PHP-DI · Jul 12, 2014 · 1150b45 · 1150b45
2 parents d9d3b4d + d06958b
commit 1150b45
Show file tree

Hide file tree

Showing 11 changed files with 183 additions and 6 deletions.
diff --git a/src/DI/Container.php b/src/DI/Container.php
@@ -179,7 +179,18 @@ public function has($name)
             ));
         }
 
-        return array_key_exists($name, $this->singletonEntries) || $this->definitionManager->getDefinition($name);
+        if (array_key_exists($name, $this->singletonEntries)) {
+            return true;
+        }
+
+        $definition = $this->definitionManager->getDefinition($name);
+        if ($definition === null) {
+            return false;
+        }
+
+        $definitionResolver = $this->getDefinitionResolver($definition);
+
+        return $definitionResolver->isResolvable($definition);
     }
 
     /**

diff --git a/src/DI/Definition/Resolver/AliasDefinitionResolver.php b/src/DI/Definition/Resolver/AliasDefinitionResolver.php
@@ -58,6 +58,21 @@ public function resolve(Definition $definition, array $parameters = array())
         return $this->container->get($definition->getTargetEntryName());
     }
 
+    /**
+     * {@inheritdoc}
+     */
+    public function isResolvable(Definition $definition, array $parameters = array())
+    {
+        if (! $definition instanceof AliasDefinition) {
+            throw new \InvalidArgumentException(sprintf(
+                'This definition resolver is only compatible with AliasDefinition objects, %s given',
+                get_class($definition)
+            ));
+        }
+
+        return true;
+    }
+
     /**
      * @return ContainerInterface
      */

diff --git a/src/DI/Definition/Resolver/ClassDefinitionResolver.php b/src/DI/Definition/Resolver/ClassDefinitionResolver.php
@@ -85,6 +85,30 @@ public function resolve(Definition $definition, array $parameters = array())
         return $this->createInstance($definition, $parameters);
     }
 
+    /**
+     * The definition is not resolvable if the class is not instantiable (interface or abstract)
+     * or if the class doesn't exist.
+     *
+     * {@inheritdoc}
+     */
+    public function isResolvable(Definition $definition, array $parameters = array())
+    {
+        if (! $definition instanceof ClassDefinition) {
+            throw new \InvalidArgumentException(sprintf(
+                'This definition resolver is only compatible with ClassDefinition objects, %s given',
+                get_class($definition)
+            ));
+        }
+
+        if (! class_exists($definition->getClassName())) {
+            return false;
+        }
+
+        $classReflection = new ReflectionClass($definition->getClassName());
+
+        return $classReflection->isInstantiable();
+    }
+
     /**
      * Injects dependencies on an existing instance.
      *

diff --git a/src/DI/Definition/Resolver/DefinitionResolver.php b/src/DI/Definition/Resolver/DefinitionResolver.php
@@ -28,4 +28,14 @@ interface DefinitionResolver
      * @return mixed Value obtained from the definition.
      */
     public function resolve(Definition $definition, array $parameters = array());
+
+    /**
+     * Check if a definition can be resolved.
+     *
+     * @param Definition $definition Object that defines how the value should be obtained.
+     * @param array      $parameters Optional parameters to use to build the entry.
+     *
+     * @return bool
+     */
+    public function isResolvable(Definition $definition, array $parameters = array());
 }
diff --git a/src/DI/Definition/Resolver/FactoryDefinitionResolver.php b/src/DI/Definition/Resolver/FactoryDefinitionResolver.php
@@ -60,6 +60,21 @@ public function resolve(Definition $definition, array $parameters = array())
         return $callable($this->container);
     }
 
+    /**
+     * {@inheritdoc}
+     */
+    public function isResolvable(Definition $definition, array $parameters = array())
+    {
+        if (! $definition instanceof FactoryDefinition) {
+            throw new \InvalidArgumentException(sprintf(
+                'This definition resolver is only compatible with FactoryDefinition objects, %s given',
+                get_class($definition)
+            ));
+        }
+
+        return true;
+    }
+
     /**
      * @return ContainerInterface
      */

diff --git a/src/DI/Definition/Resolver/FunctionCallDefinitionResolver.php b/src/DI/Definition/Resolver/FunctionCallDefinitionResolver.php
@@ -77,4 +77,19 @@ public function resolve(Definition $definition, array $parameters = array())
             return $functionReflection->invokeArgs($args);
         }
     }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function isResolvable(Definition $definition, array $parameters = array())
+    {
+        if (! $definition instanceof FunctionCallDefinition) {
+            throw new \InvalidArgumentException(sprintf(
+                'This definition resolver is only compatible with FunctionCallDefinition objects, %s given',
+                get_class($definition)
+            ));
+        }
+
+        return true;
+    }
 }
diff --git a/src/DI/Definition/Resolver/ParameterResolver.php b/src/DI/Definition/Resolver/ParameterResolver.php
@@ -48,8 +48,7 @@ public function resolveParameters(
         AbstractFunctionCallDefinition $definition = null,
         \ReflectionFunctionAbstract $functionReflection = null,
         array $parameters = array()
-    )
-    {
+    ) {
         $args = array();
 
         if (! $functionReflection) {
@@ -78,10 +77,15 @@ public function resolveParameters(
             }
 
             if ($value instanceof EntryReference) {
-                $args[] = $this->container->get($value->getName());
-            } else {
-                $args[] = $value;
+                // If the container cannot produce the entry, we can use the default parameter value
+                if (!$this->container->has($value->getName()) && $parameter->isOptional()) {
+                    $value = $this->getParameterDefaultValue($parameter, $functionReflection);
+                } else {
+                    $value = $this->container->get($value->getName());
+                }
             }
+
+            $args[] = $value;
         }
 
         return $args;

diff --git a/src/DI/Definition/Resolver/ValueDefinitionResolver.php b/src/DI/Definition/Resolver/ValueDefinitionResolver.php
@@ -38,4 +38,19 @@ public function resolve(Definition $definition, array $parameters = array())
 
         return $definition->getValue();
     }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function isResolvable(Definition $definition, array $parameters = array())
+    {
+        if (! $definition instanceof ValueDefinition) {
+            throw new \InvalidArgumentException(sprintf(
+                'This definition resolver is only compatible with ValueDefinition objects, %s given',
+                get_class($definition)
+            ));
+        }
+
+        return true;
+    }
 }
diff --git a/tests/IntegrationTests/DI/Fixtures/definitions.php b/tests/IntegrationTests/DI/Fixtures/definitions.php
@@ -30,6 +30,7 @@
 
     'IntegrationTests\DI\Fixtures\Interface1' => DI\object('IntegrationTests\DI\Fixtures\Implementation1')
             ->scope(Scope::SINGLETON()),
+    'IntegrationTests\DI\Fixtures\Interface2' => DI\object('IntegrationTests\DI\Fixtures\Class3'),
 
     'namedDependency' => DI\object('IntegrationTests\DI\Fixtures\Class2'),
 

diff --git a/tests/IntegrationTests/DI/Issues/Issue168Test.php b/tests/IntegrationTests/DI/Issues/Issue168Test.php
@@ -0,0 +1,45 @@
+<?php
+/**
+ * PHP-DI
+ *
+ * @link      http://mnapoli.github.io/PHP-DI/
+ * @copyright Matthieu Napoli (http://mnapoli.fr/)
+ * @license   http://www.opensource.org/licenses/mit-license.php MIT (see the LICENSE file)
+ */
+
+namespace IntegrationTests\DI\Issues;
+
+use DI\ContainerBuilder;
+
+/**
+ * Test for constructor injection of parameters that are optional, and use an
+ * interface (or other uninstantiable) type hint.
+ *
+ * @link https://github.com/mnapoli/PHP-DI/pull/168
+ *
+ * @coversNothing
+ */
+class Issue168Test extends \PHPUnit_Framework_TestCase
+{
+    public function testInterfaceOptionalParameter()
+    {
+        $container = ContainerBuilder::buildDevContainer();
+        $object = $container->get('IntegrationTests\DI\Issues\TestClass');
+        $this->assertInstanceOf('IntegrationTests\DI\Issues\TestClass', $object);
+    }
+}
+
+class TestClass
+{
+    /**
+     * The parameter is optional. TestInterface is not instantiable, so `null` should
+     * be injected instead of getting an exception.
+     */
+    public function __construct(TestInterface $param = null)
+    {
+    }
+}
+
+interface TestInterface
+{
+}
diff --git a/tests/UnitTests/DI/Definition/Resolver/ClassDefinitionResolverTest.php b/tests/UnitTests/DI/Definition/Resolver/ClassDefinitionResolverTest.php
@@ -244,6 +244,20 @@ public function testInvalidDefinitionType()
         $resolver->resolve($definition);
     }
 
+    public function testIsResolvable()
+    {
+        $resolver = $this->buildResolver();
+
+        $classDefinition = new ClassDefinition('UnitTests\DI\Definition\Resolver\FixtureClass');
+        $this->assertTrue($resolver->isResolvable($classDefinition));
+
+        $interfaceDefinition = new ClassDefinition('UnitTests\DI\Definition\Resolver\FixtureInterface');
+        $this->assertFalse($resolver->isResolvable($interfaceDefinition));
+
+        $abstractClassDefinition = new ClassDefinition('UnitTests\DI\Definition\Resolver\FixtureAbstractClass');
+        $this->assertFalse($resolver->isResolvable($abstractClassDefinition));
+    }
+
     private function buildResolver()
     {
         /** @var \DI\Container $container */
@@ -281,3 +295,11 @@ public function methodDefaultValue($param = 'defaultValue')
 class NoConstructor
 {
 }
+
+interface FixtureInterface
+{
+}
+
+abstract class FixtureAbstractClass
+{
+}