Merge pull request #432 from predakanga/fix-431

Fix Factory parameter resolution order, incl. tests
PHP-DI · Jul 15, 2016 · 9845d40 · 9845d40
2 parents 64fa5b7 + 3bd9bf8
commit 9845d40
Show file tree

Hide file tree

Showing 4 changed files with 94 additions and 2 deletions.
diff --git a/src/DI/Definition/Resolver/FactoryResolver.php b/src/DI/Definition/Resolver/FactoryResolver.php
@@ -63,8 +63,8 @@ public function resolve(Definition $definition, array $parameters = [])
     {
         if (! $this->invoker) {
             $parameterResolver = new ResolverChain([
-               new FactoryParameterResolver($this->container),
                new AssociativeArrayResolver,
+               new FactoryParameterResolver($this->container),
                new NumericArrayResolver,
             ]);
 

diff --git a/src/DI/Invoker/FactoryParameterResolver.php b/src/DI/Invoker/FactoryParameterResolver.php
@@ -9,6 +9,9 @@
 /**
  * Inject the container, the definition or any other service using type-hints.
  *
+ * {@internal This class is similar to TypeHintingResolver and TypeHintingContainerResolver,
+ *            we use this instead for performance reasons}
+ *
  * @author Quim Calpe <quim@kalpe.com>
  * @author Matthieu Napoli <matthieu@mnapoli.fr>
  */
@@ -29,7 +32,14 @@ public function getParameters(
         array $providedParameters,
         array $resolvedParameters
     ) {
-        foreach ($reflection->getParameters() as $index => $parameter) {
+        $parameters = $reflection->getParameters();
+
+        // Skip parameters already resolved
+        if (! empty($resolvedParameters)) {
+            $parameters = array_diff_key($parameters, $resolvedParameters);
+        }
+
+        foreach ($parameters as $index => $parameter) {
             $parameterClass = $parameter->getClass();
 
             if (!$parameterClass) {

diff --git a/tests/IntegrationTest/Definitions/FactoryDefinitionTest.php b/tests/IntegrationTest/Definitions/FactoryDefinitionTest.php
@@ -4,6 +4,7 @@
 
 use DI\ContainerBuilder;
 use DI\Factory\RequestedEntry;
+use DI\Test\UnitTest\Definition\Resolver\Fixture\NoConstructor;
 use Interop\Container\ContainerInterface;
 
 /**
@@ -271,6 +272,34 @@ public function test_container_and_requested_entry_and_typehints_get_injected_in
         $this->assertInstanceOf(ContainerInterface::class, $factory[3]);
     }
 
+    public function test_parameters_take_priority_over_container()
+    {
+        $ncInst = new NoConstructor();
+
+        $container = $this->createContainer([
+            'factory' => \DI\factory(function (NoConstructor $nc) {
+                return $nc;
+            })->parameter('nc', $ncInst),
+        ]);
+
+        $factory = $container->get('factory');
+
+        $this->assertSame($ncInst, $factory);
+    }
+
+    public function test_parameters_take_priority_over_default_value()
+    {
+        $container = $this->createContainer([
+            'factory' => \DI\factory(function ($foo = 'Foo') {
+                return $foo;
+            })->parameter('foo', 'Bar'),
+        ]);
+
+        $factory = $container->get('factory');
+
+        $this->assertEquals('Bar', $factory);
+    }
+
     /**
      * @expectedException \DI\NotFoundException
      * @expectedExceptionMessage No entry or class found for 'missing'

diff --git a/tests/UnitTest/Definition/Resolver/FactoryParameterResolverTest.php b/tests/UnitTest/Definition/Resolver/FactoryParameterResolverTest.php
@@ -2,8 +2,10 @@
 
 namespace DI\Test\UnitTest\Definition\Resolver;
 
+use DI\Container;
 use DI\Factory\RequestedEntry;
 use DI\Invoker\FactoryParameterResolver;
+use DI\Test\UnitTest\Definition\Resolver\Fixture\NoConstructor;
 use EasyMock\EasyMock;
 use Interop\Container\ContainerInterface;
 
@@ -95,4 +97,55 @@ public function should_resolve_nothing()
 
         $this->assertCount(0, $parameters);
     }
+
+    /**
+     * @test
+     */
+    public function should_not_overwrite_resolved_with_container_or_entry()
+    {
+        $callable = function (ContainerInterface $container, RequestedEntry $entry, $other) {
+        };
+        $reflection = new \ReflectionFunction($callable);
+
+        $mockContainer = $this->easyMock(Container::class);
+        $mockEntry = $this->easyMock(RequestedEntry::class);
+
+        $resolvedParams = [$mockContainer, $mockEntry, 'Foo'];
+
+        $parameters = $this->resolver->getParameters($reflection, [$this->container, $this->requestedEntry], $resolvedParams);
+
+        $this->assertCount(3, $parameters);
+        $this->assertSame($parameters[0], $mockContainer);
+        $this->assertSame($parameters[1], $mockEntry);
+        $this->assertEquals($parameters[2], 'Foo');
+    }
+
+    /**
+     * @test
+     */
+    public function should_not_overwrite_resolved_from_container()
+    {
+        $callable = function (NoConstructor $nc) {
+        };
+        $reflection = new \ReflectionFunction($callable);
+
+        $ncMock = $this->easyMock(NoConstructor::class);
+        $ncReal = new NoConstructor();
+
+        $preparedContainer = clone $this->container;
+        $preparedContainer->expects($this->once())
+                          ->method('has')
+                          ->with(NoConstructor::class)
+                          ->willReturn(true);
+        $preparedContainer->expects($this->once())
+                          ->method('get')
+                          ->with(NoConstructor::class)
+                          ->willReturn($ncReal);
+
+        $resolvedParams = [$ncMock];
+        $parameters = $this->resolver->getParameters($reflection, [$this->container, $this->requestedEntry], $resolvedParams);
+
+        $this->assertCount(1, $parameters);
+        $this->assertSame($parameters[0], $ncMock);
+    }
 }