[DI] CheckTypeDeclarationsPass now checks if value is type of parameter type

Author: pfazzi

Compiler/CheckTypeDeclarationsPass.php

@@ -12,7 +12,9 @@
 namespace Symfony\Component\DependencyInjection\Compiler;
 
 use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
+use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
 use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
+use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;
 use Symfony\Component\DependencyInjection\Container;
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Exception\EnvNotFoundException;
@@ -40,7 +42,23 @@
  */
 final class CheckTypeDeclarationsPass extends AbstractRecursivePass
 {
-    private const SCALAR_TYPES = ['int', 'float', 'bool', 'string'];
+    private const SCALAR_TYPES = [
+        'int' => true,
+        'float' => true,
+        'bool' => true,
+        'string' => true,
+    ];
+
+    private const BUILTIN_TYPES = [
+        'array' => true,
+        'bool' => true,
+        'callable' => true,
+        'float' => true,
+        'int' => true,
+        'iterable' => true,
+        'object' => true,
+        'string' => true,
+    ];
 
     private $autoload;
     private $skippedIds;
@@ -160,33 +178,17 @@ private function checkType(Definition $checkedDefinition, $value, \ReflectionPar
             $type = $checkedDefinition->getClass();
         }
 
+        $class = null;
+
         if ($value instanceof Definition) {
             $class = $value->getClass();
 
-            if (!$class || (!$this->autoload && !class_exists($class, false) && !interface_exists($class, false))) {
-                return;
-            }
-
-            if ('callable' === $type && (\Closure::class === $class || method_exists($class, '__invoke'))) {
-                return;
-            }
-
-            if ('iterable' === $type && is_subclass_of($class, 'Traversable')) {
-                return;
-            }
-
-            if ('object' === $type) {
-                return;
-            }
-
-            if (is_a($class, $type, true)) {
+            if (isset(self::BUILTIN_TYPES[strtolower($class)])) {
+                $class = strtolower($class);
+            } elseif (!$class || (!$this->autoload && !class_exists($class, false) && !interface_exists($class, false))) {
                 return;
             }
-
-            throw new InvalidParameterTypeException($this->currentId, $class, $parameter);
-        }
-
-        if ($value instanceof Parameter) {
+        } elseif ($value instanceof Parameter) {
             $value = $this->container->getParameter($value);
         } elseif ($value instanceof Expression) {
             $value = $this->getExpressionLanguage()->evaluate($value, ['container' => $this->container]);
@@ -212,30 +214,53 @@ private function checkType(Definition $checkedDefinition, $value, \ReflectionPar
             return;
         }
 
-        if (\in_array($type, self::SCALAR_TYPES, true) && is_scalar($value)) {
+        if (null === $class) {
+            if ($value instanceof IteratorArgument) {
+                $class = RewindableGenerator::class;
+            } elseif ($value instanceof ServiceClosureArgument) {
+                $class = \Closure::class;
+            } elseif ($value instanceof ServiceLocatorArgument) {
+                $class = ServiceLocator::class;
+            } elseif (\is_object($value)) {
+                $class = \get_class($value);
+            } else {
+                $class = \gettype($value);
+                $class = ['integer' => 'int', 'double' => 'float', 'boolean' => 'bool'][$class] ?? $class;
+            }
+        }
+
+        if (isset(self::SCALAR_TYPES[$type]) && isset(self::SCALAR_TYPES[$class])) {
+            return;
+        }
+
+        if ('string' === $type && \is_callable([$class, '__toString'])) {
+            return;
+        }
+
+        if ('callable' === $type && (\Closure::class === $class || \is_callable([$class, '__invoke']))) {
             return;
         }
 
-        if ('callable' === $type && \is_array($value) && isset($value[0]) && ($value[0] instanceof Reference || $value[0] instanceof Definition)) {
+        if ('callable' === $type && \is_array($value) && isset($value[0]) && ($value[0] instanceof Reference || $value[0] instanceof Definition || \is_string($value[0]))) {
             return;
         }
 
-        if (\in_array($type, ['callable', 'Closure'], true) && $value instanceof ServiceClosureArgument) {
+        if ('iterable' === $type && (\is_array($value) || is_subclass_of($class, \Traversable::class))) {
             return;
         }
 
-        if ('iterable' === $type && (\is_array($value) || $value instanceof \Traversable || $value instanceof IteratorArgument)) {
+        if ('object' === $type && !isset(self::BUILTIN_TYPES[$class])) {
             return;
         }
 
-        if ('Traversable' === $type && ($value instanceof \Traversable || $value instanceof IteratorArgument)) {
+        if (is_a($class, $type, true)) {
             return;
         }
 
         $checkFunction = sprintf('is_%s', $parameter->getType()->getName());
 
         if (!$parameter->getType()->isBuiltin() || !$checkFunction($value)) {
-            throw new InvalidParameterTypeException($this->currentId, \is_object($value) ? \get_class($value) : \gettype($value), $parameter);
+            throw new InvalidParameterTypeException($this->currentId, \is_object($value) ? $class : \gettype($value), $parameter);
         }
     }
 

Tests/Compiler/CheckTypeDeclarationsPassTest.php

@@ -26,6 +26,8 @@
 use Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass\BarOptionalArgumentNotNull;
 use Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass\Foo;
 use Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass\FooObject;
+use Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass\Waldo;
+use Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass\Wobble;
 use Symfony\Component\ExpressionLanguage\Expression;
 
 /**
@@ -602,6 +604,21 @@ public function testProcessResolveExpressions()
         $this->addToAssertionCount(1);
     }
 
+    public function testProcessSuccessWhenExpressionReturnsObject()
+    {
+        $container = new ContainerBuilder();
+
+        $container->register('waldo', Waldo::class);
+
+        $container
+            ->register('wobble', Wobble::class)
+            ->setArguments([new Expression("service('waldo')")]);
+
+        (new CheckTypeDeclarationsPass(true))->process($container);
+
+        $this->addToAssertionCount(1);
+    }
+
     public function testProcessHandleMixedEnvPlaceholder()
     {
         $this->expectException(\Symfony\Component\DependencyInjection\Exception\InvalidArgumentException::class);

Tests/Fixtures/CheckTypeDeclarationsPass/Waldo.php

@@ -0,0 +1,7 @@
+<?php
+
+namespace Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass;
+
+class Waldo implements WaldoInterface
+{
+}

Tests/Fixtures/CheckTypeDeclarationsPass/WaldoInterface.php

@@ -0,0 +1,7 @@
+<?php
+
+namespace Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass;
+
+interface WaldoInterface
+{
+}

Tests/Fixtures/CheckTypeDeclarationsPass/Wobble.php

@@ -0,0 +1,13 @@
+<?php
+
+namespace Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass;
+
+class Wobble
+{
+    private $waldo;
+
+    public function __construct(WaldoInterface $waldo)
+    {
+        $this->waldo = $waldo;
+    }
+}