bug #22642 [DX] Making the RegisterControllerArgumentLocatorsPass throw exception on bad types (weaverryan)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DX] Making the RegisterControllerArgumentLocatorsPass throw exception on bad types

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | maybe
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | none
| License       | MIT
| Doc PR        | n/a

Suppose you type-hint a controller arg with a non-existent class:

```php
public function fooAction(FakeClass $foo)
```

Current error:

> Class AppBundle\Controller\FakeClass does not exist

(from `ParamConverterListener`, and only when you hit that route)

New error:

> Cannot determine controller argument for "AppBundle\Controller\BlogController::indexAction()
  ": the $foo argument is type-hinted with the non-existent class or interface: "AppBundle\Con
  troller\FakeClass". Did you forget to add a use statement?

(at build time)

The extra `Did you forget to add a use statement?` only shows up if it appears you likely forgot a `use` statement.

I think this will be a really common error (especially forgetting the `use` statement)... so let's make it a *really* nice error! An alternative would be to enhance the args resolver to throw a clearer exception when no arg can be wired and the type-hint is bad (we would also need to make the `ParamConverterListener` stop throwing the current error... so that the args resolver would have the opportunity to do that. Disadvantage would be that this error would only happen when you hit the route, not at build time.

Cheers!

Commits
-------

22b905226d Making the RegisterControllerArgumentLocatorsPass throw an exception on a bad type-hint

src/Symfony/Component/HttpKernel/DependencyInjection/RegisterControllerArgumentLocatorsPass.php

@@ -110,9 +110,12 @@ class RegisterControllerArgumentLocatorsPass implements CompilerPassInterface
             }
 
             foreach ($methods as list($r, $parameters)) {
+                /** @var \ReflectionMethod $r */
+
                 // create a per-method map of argument-names to service/type-references
                 $args = array();
                 foreach ($parameters as $p) {
+                    /** @var \ReflectionParameter $p */
                     $type = $target = ProxyHelper::getTypeHint($r, $p, true);
                     $invalidBehavior = ContainerInterface::IGNORE_ON_INVALID_REFERENCE;
 
@@ -129,6 +132,17 @@ class RegisterControllerArgumentLocatorsPass implements CompilerPassInterface
                         continue;
                     }
 
+                    if ($type && !$p->isOptional() && !$p->allowsNull() && !class_exists($type) && !interface_exists($type, false)) {
+                        $message = sprintf('Cannot determine controller argument for "%s::%s()": the $%s argument is type-hinted with the non-existent class or interface: "%s".', $class, $r->name, $p->name, $type);
+
+                        // see if the type-hint lives in the same namespace as the controller
+                        if (0 === strncmp($type, $class, strrpos($class, '\\'))) {
+                            $message .= ' Did you forget to add a use statement?';
+                        }
+
+                        throw new InvalidArgumentException($message);
+                    }
+
                     $args[$p->name] = $type ? new TypedReference($target, $type, $r->class, $invalidBehavior) : new Reference($target, $invalidBehavior);
                 }
                 // register the maps as a per-method service-locators

src/Symfony/Component/HttpKernel/Tests/DependencyInjection/RegisterControllerArgumentLocatorsPassTest.php

@@ -204,6 +204,68 @@ class RegisterControllerArgumentLocatorsPassTest extends TestCase
         $locator = $container->getDefinition((string) $resolver->getArgument(0))->getArgument(0);
         $this->assertSame(array('foo:fooAction'), array_keys($locator));
     }
+
+    /**
+     * @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
+     * @expectedExceptionMessage Cannot determine controller argument for "Symfony\Component\HttpKernel\Tests\DependencyInjection\NonExistentClassController::fooAction()": the $nonExistent argument is type-hinted with the non-existent class or interface: "Symfony\Component\HttpKernel\Tests\DependencyInjection\NonExistentClass". Did you forget to add a use statement?
+     */
+    public function testExceptionOnNonExistentTypeHint()
+    {
+        $container = new ContainerBuilder();
+        $container->register('argument_resolver.service')->addArgument(array());
+
+        $container->register('foo', NonExistentClassController::class)
+            ->addTag('controller.service_arguments');
+
+        $pass = new RegisterControllerArgumentLocatorsPass();
+        $pass->process($container);
+    }
+
+    /**
+     * @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
+     * @expectedExceptionMessage Cannot determine controller argument for "Symfony\Component\HttpKernel\Tests\DependencyInjection\NonExistentClassDifferentNamespaceController::fooAction()": the $nonExistent argument is type-hinted with the non-existent class or interface: "Acme\NonExistentClass".
+     */
+    public function testExceptionOnNonExistentTypeHintDifferentNamespace()
+    {
+        $container = new ContainerBuilder();
+        $container->register('argument_resolver.service')->addArgument(array());
+
+        $container->register('foo', NonExistentClassDifferentNamespaceController::class)
+            ->addTag('controller.service_arguments');
+
+        $pass = new RegisterControllerArgumentLocatorsPass();
+        $pass->process($container);
+    }
+
+    public function testNoExceptionOnNonExistentTypeHintOptionalArg()
+    {
+        $container = new ContainerBuilder();
+        $resolver = $container->register('argument_resolver.service')->addArgument(array());
+
+        $container->register('foo', NonExistentClassOptionalController::class)
+            ->addTag('controller.service_arguments');
+
+        $pass = new RegisterControllerArgumentLocatorsPass();
+        $pass->process($container);
+
+        $locator = $container->getDefinition((string) $resolver->getArgument(0))->getArgument(0);
+        $this->assertSame(array('foo:barAction', 'foo:fooAction'), array_keys($locator));
+    }
+
+    public function testArgumentWithNoTypeHintIsOk()
+    {
+        $container = new ContainerBuilder();
+        $resolver = $container->register('argument_resolver.service')->addArgument(array());
+
+        $container->register('foo', ArgumentWithoutTypeController::class)
+            ->addTag('controller.service_arguments');
+
+        $pass = new RegisterControllerArgumentLocatorsPass();
+        $pass->process($container);
+
+        $locator = $container->getDefinition((string) $resolver->getArgument(0))->getArgument(0);
+        $this->assertEmpty(array_keys($locator));
+    }
 }
 
 class RegisterTestController
@@ -233,3 +295,35 @@ class ContainerAwareRegisterTestController implements ContainerAwareInterface
 class ControllerDummy
 {
 }
+
+class NonExistentClassController
+{
+    public function fooAction(NonExistentClass $nonExistent)
+    {
+    }
+}
+
+class NonExistentClassDifferentNamespaceController
+{
+    public function fooAction(\Acme\NonExistentClass $nonExistent)
+    {
+    }
+}
+
+class NonExistentClassOptionalController
+{
+    public function fooAction(NonExistentClass $nonExistent = null)
+    {
+    }
+
+    public function barAction(NonExistentClass $nonExistent = null, $bar)
+    {
+    }
+}
+
+class ArgumentWithoutTypeController
+{
+    public function fooAction($someArg)
+    {
+    }
+}

src/Symfony/Component/HttpKernel/Tests/DependencyInjection/RemoveEmptyControllerArgumentLocatorsPassTest.php

@@ -120,7 +120,7 @@ class RemoveEmptyControllerArgumentLocatorsPassTest extends TestCase
 
 class RemoveTestController1
 {
-    public function fooAction(\stdClass $bar, NotFound $baz)
+    public function fooAction(\stdClass $bar, ClassNotInContainer $baz)
     {
     }
 }
@@ -131,7 +131,7 @@ class RemoveTestController2
     {
     }
 
-    public function fooAction(NotFound $bar)
+    public function fooAction(ClassNotInContainer $bar)
     {
     }
 }
@@ -142,3 +142,7 @@ class InvokableRegisterTestController
     {
     }
 }
+
+class ClassNotInContainer
+{
+}