Improve return type detection

bnf · Sep 12, 2018 · 202e37b · 202e37b
1 parent b66b491
commit 202e37b
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 33 deletions.
diff --git a/src/ServiceProviderCompilationPass.php b/src/ServiceProviderCompilationPass.php
@@ -6,7 +6,6 @@
 
 use Interop\Container\ServiceProviderInterface;
 use Symfony\Component\DependencyInjection\Alias;
-use Symfony\Component\DependencyInjection\ChildDefinition;
 use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Definition;
@@ -111,64 +110,50 @@ private function addServiceDefinitionFromCallable($serviceName, $serviceProvider
             // TODO: plug the definition-interop converter here!
         }*/
 
+        $finalServiceName = $serviceName;
         $innerName = null;
-        $decoratedServiceName = null;
+
         $factoryDefinition = new Definition($this->getReturnType($callable, $serviceName));
         $factoryDefinition->setPublic(true);
+
         if ($extension && $container->has($serviceName)) {
-            // TODO: Use a ChildDefinition? $factoryDefinition = new ChildDefinition($serviceName);
-            list($decoratedServiceName, $previousServiceName) = $this->getDecoratedServiceName($serviceName, $container);
-            $innerName = $decoratedServiceName . '.inner';
+            list($finalServiceName, $previousServiceName) = $this->getDecoratedServiceName($serviceName, $container);
+            $innerName = $finalServiceName . '.inner';
 
             $factoryDefinition->setDecoratedService($previousServiceName, $innerName);
         }
 
-        $containerDefinition = new Reference('service_container');
-
         if ((is_array($callable) && is_string($callable[0])) || is_string($callable)) {
             $factoryDefinition->setFactory($callable);
-            $factoryDefinition->addArgument($containerDefinition);
         } else {
             $registryMethod = $extension ? 'extendService' : 'createService';
             $factoryDefinition->setFactory([ new Reference('service_provider_registry_'.$this->registryId), $registryMethod ]);
             $factoryDefinition->addArgument($serviceProviderKey);
             $factoryDefinition->addArgument($serviceName);
-            $factoryDefinition->addArgument($containerDefinition);
         }
 
-        if ($innerName) {
+        $factoryDefinition->addArgument(new Reference('service_container'));
+        if ($innerName !== null) {
             $factoryDefinition->addArgument(new Reference($innerName));
         }
 
-        if ($decoratedServiceName) {
-            $container->setDefinition($decoratedServiceName, $factoryDefinition);
-        } else {
-            $container->setDefinition($serviceName, $factoryDefinition);
-        }
-
-        return $factoryDefinition;
+        $container->setDefinition($finalServiceName, $factoryDefinition);
     }
 
     private function getReturnType(callable $callable, string $serviceName): string
     {
-        $reflection = null;
-        if (is_array($callable)) {
-            $reflection = new \ReflectionMethod($callable[0], $callable[1]);
-        } elseif (is_object($callable)) {
-            if ($callable instanceof \Closure) {
-                $reflection = new \ReflectionFunction($callable);
-            } else {
-                $reflection = new \ReflectionMethod($callable, '__invoke');
-            }
-        } elseif (is_string($callable)) {
-            $reflection = new \ReflectionFunction($callable);
-        }
+        return $this->getReflection($callable)->getReturnType() ?: $serviceName;
+    }
 
-        if ($reflection && ($returnType = $reflection->getReturnType())) {
-            return (string) $returnType;
+    private function getReflection(callable $callable): \ReflectionFunctionAbstract
+    {
+        if (is_array($callable) && count($callable) === 2) {
+            return new \ReflectionMethod($callable[0], $callable[1]);
+        }
+        if (is_object($callable) && !$callable instanceof \Closure) {
+            return new \ReflectionMethod($callable, '__invoke');
         }
 
-        // If we cannot reflect a return type, assume the serviceName is the FQCN
-        return $serviceName;
+        return new \ReflectionFunction($callable);
     }
 }
diff --git a/tests/Fixtures/TestServiceProvider.php b/tests/Fixtures/TestServiceProvider.php
@@ -24,6 +24,11 @@ public function getFactories()
             'serviceC' => function (ContainerInterface $container): \stdClass {
                 return new \stdClass();
             },
+            'serviceD' => new class {
+                public function __invoke(ContainerInterface $container): \stdClass {
+                    return new \stdClass();
+                }
+            },
             'function' => 'Bnf\\Interop\\ServiceProviderBridgeBundle\\Tests\\Fixtures\\myFunctionFactory'
         ];
     }

diff --git a/tests/ServiceProviderCompilationPassTest.php b/tests/ServiceProviderCompilationPassTest.php
@@ -4,6 +4,7 @@
 namespace Bnf\Interop\ServiceProviderBridgeBundle\Tests;
 
 
+use Interop\Container\ServiceProviderInterface;
 use PHPUnit\Framework\TestCase;
 use Psr\Log\LoggerInterface;
 use Psr\Log\NullLogger;
@@ -39,8 +40,10 @@ public function testSimpleServiceProvider()
         ]);
 
         $serviceA = $container->get('serviceA');
+        $serviceD = $container->get('serviceD');
 
         $this->assertInstanceOf(\stdClass::class, $serviceA);
+        $this->assertInstanceOf(\stdClass::class, $serviceD);
         $this->assertEquals(42, $container->get('function'));
     }
 
@@ -61,6 +64,31 @@ public function testServiceProviderOverrides()
         $this->assertEquals('localhost', $serviceC->serviceB->parameter);
     }
 
+    /**
+     * @expectedException \TypeError
+     */
+    public function testExceptionForInvalidFactories()
+    {
+        $bundle = new InteropServiceProviderBridgeBundle([
+            new class implements ServiceProviderInterface {
+                public function getFactories()
+                {
+                    return [
+                        'invalid' => 2
+                    ];
+                }
+                public function getExtensions()
+                {
+                    return [];
+                }
+
+            }
+        ]);
+        $container = new ContainerBuilder();
+        $bundle->build($container);
+        $container->compile();
+    }
+
     /**
      * @expectedException \Bnf\Interop\ServiceProviderBridgeBundle\Exception\InvalidArgumentException
      */