feature #22306 [DI] Restrict autowired registration to "same-vendor" …

…namespaces (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Restrict autowired registration to "same-vendor" namespaces

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Following #22295, it came to me that I've already been bitten hard by the auto-registration we decided to keep working: when one type-hints some class in the Symfony namespace (eg `Request`), then autoregistration creates a corresponding service. You see the issue. Hard time debugging that.

By restricting autoregistration to same-vendor (=same-root-namespace), we can keep all benefits of autoregistration, while closing this DX trap.

The patch is bigger than strictly required to implement this, but contains a few related fixes and cleanups.

Commits
-------

9c53b3d [DI] Restrict autowired registration to "same-vendor" namespaces

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

@@ -16,7 +16,6 @@
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Exception\RuntimeException;
 use Symfony\Component\DependencyInjection\LazyProxy\ProxyHelper;
-use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\DependencyInjection\TypedReference;
 
 /**
@@ -76,9 +75,9 @@ public static function createResourceForClass(\ReflectionClass $reflectionClass)
      */
     protected function processValue($value, $isRoot = false)
     {
-        if ($value instanceof TypedReference && !$this->container->has((string) $value)) {
-            if ($ref = $this->getAutowiredReference($value->getType(), $value->canBeAutoregistered())) {
-                $value = new TypedReference((string) $ref, $value->getType(), $value->getInvalidBehavior(), $value->canBeAutoregistered());
+        if ($value instanceof TypedReference) {
+            if ($ref = $this->getAutowiredReference($value)) {
+                $value = $ref;
             } else {
                 $this->container->log($this, $this->createTypeNotFoundMessage($value->getType(), 'typed reference'));
             }
@@ -242,7 +241,7 @@ private function autowireMethod(\ReflectionFunctionAbstract $reflectionMethod, a
                 continue;
             }
 
-            if (!$value = $this->getAutowiredReference($type, !$parameter->isOptional())) {
+            if (!$value = $this->getAutowiredReference(new TypedReference($type, $type, !$parameter->isOptional() ? $class : ''))) {
                 $failureMessage = $this->createTypeNotFoundMessage($type, sprintf('argument "$%s" of method "%s()"', $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method));
 
                 if ($parameter->isDefaultValueAvailable()) {
@@ -276,29 +275,33 @@ private function autowireMethod(\ReflectionFunctionAbstract $reflectionMethod, a
     }
 
     /**
-     * @return Reference|null A reference to the service matching the given type, if any
+     * @return TypedReference|null A reference to the service matching the given type, if any
      */
-    private function getAutowiredReference($type, $autoRegister)
+    private function getAutowiredReference(TypedReference $reference)
     {
-        if ($this->container->has($type) && !$this->container->findDefinition($type)->isAbstract()) {
-            return new Reference($type);
-        }
+        $type = $reference->getType();
 
-        if (isset($this->autowired[$type])) {
-            return $this->autowired[$type] ? new Reference($this->autowired[$type]) : null;
+        if ($type !== (string) $reference || ($this->container->has($type) && !$this->container->findDefinition($type)->isAbstract())) {
+            return $reference;
         }
 
         if (null === $this->types) {
             $this->populateAvailableTypes();
         }
 
         if (isset($this->definedTypes[$type])) {
-            return new Reference($this->types[$type]);
+            return new TypedReference($this->types[$type], $type);
         }
 
-        if ($autoRegister && !isset($this->types[$type]) && !isset($this->ambiguousServiceTypes[$type])) {
-            return $this->createAutowiredDefinition($type);
+        if (!$reference->canBeAutoregistered() || isset($this->types[$type]) || isset($this->ambiguousServiceTypes[$type])) {
+            return;
         }
+
+        if (isset($this->autowired[$type])) {
+            return $this->autowired[$type] ? new TypedReference($this->autowired[$type], $type) : null;
+        }
+
+        return $this->createAutowiredDefinition($type);
     }
 
     /**
@@ -384,7 +387,7 @@ private function set($type, $id)
      *
      * @param string $type
      *
-     * @return Reference|null A reference to the registered definition
+     * @return TypedReference|null A reference to the registered definition
      */
     private function createAutowiredDefinition($type)
     {
@@ -412,7 +415,7 @@ private function createAutowiredDefinition($type)
 
         $this->container->log($this, sprintf('Type "%s" has been auto-registered for service "%s".', $type, $this->currentId));
 
-        return new Reference($argumentId);
+        return new TypedReference($argumentId, $type);
     }
 
     private function createTypeNotFoundMessage($type, $label)

src/Symfony/Component/DependencyInjection/Compiler/RegisterServiceSubscribersPass.php

@@ -71,10 +71,11 @@ protected function processValue($value, $isRoot = false)
         }
         $this->container->addObjectResource($class);
         $subscriberMap = array();
+        $declaringClass = (new \ReflectionMethod($class, 'getSubscribedServices'))->class;
 
         foreach ($class::getSubscribedServices() as $key => $type) {
             if (!is_string($type) || !preg_match('/^\??[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+(?:\\\\[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+)*+$/', $type)) {
-                throw new InvalidArgumentException(sprintf('%s::getSubscribedServices() must return valid PHP types for service "%s" key "%s", "%s" returned.', $class, $this->currentId, $key, is_string($type) ? $type : gettype($type)));
+                throw new InvalidArgumentException(sprintf('"%s::getSubscribedServices()" must return valid PHP types for service "%s" key "%s", "%s" returned.', $class, $this->currentId, $key, is_string($type) ? $type : gettype($type)));
             }
             if ($optionalBehavior = '?' === $type[0]) {
                 $type = substr($type, 1);
@@ -85,18 +86,18 @@ protected function processValue($value, $isRoot = false)
             }
             if (!isset($serviceMap[$key])) {
                 if (!$autowire) {
-                    throw new InvalidArgumentException(sprintf('Service "%s" misses a "container.service_subscriber" tag with "key"/"id" attributes corresponding to entry "%s" as returned by %s::getSubscribedServices().', $this->currentId, $key, $class));
+                    throw new InvalidArgumentException(sprintf('Service "%s" misses a "container.service_subscriber" tag with "key"/"id" attributes corresponding to entry "%s" as returned by "%s::getSubscribedServices()".', $this->currentId, $key, $class));
                 }
                 $serviceMap[$key] = new Reference($type);
             }
 
-            $subscriberMap[$key] = new TypedReference((string) $serviceMap[$key], $type, $optionalBehavior ?: ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE);
+            $subscriberMap[$key] = new TypedReference((string) $serviceMap[$key], $type, $declaringClass, $optionalBehavior ?: ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE);
             unset($serviceMap[$key]);
         }
 
         if ($serviceMap = array_keys($serviceMap)) {
             $message = sprintf(1 < count($serviceMap) ? 'keys "%s" do' : 'key "%s" does', str_replace('%', '%%', implode('", "', $serviceMap)));
-            throw new InvalidArgumentException(sprintf('Service %s not exist in the map returned by %s::getSubscribedServices() for service "%s".', $message, $class, $this->currentId));
+            throw new InvalidArgumentException(sprintf('Service %s not exist in the map returned by "%s::getSubscribedServices()" for service "%s".', $message, $class, $this->currentId));
         }
 
         $serviceLocator = $this->serviceLocator;

src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php

@@ -368,9 +368,9 @@ public function testSomeSpecificArgumentsAreSet()
         $definition = $container->getDefinition('multiple');
         $this->assertEquals(
             array(
-                new Reference(A::class),
+                new TypedReference(A::class, A::class, MultipleArguments::class),
                 new Reference('foo'),
-                new Reference(Dunglas::class),
+                new TypedReference(Dunglas::class, Dunglas::class, MultipleArguments::class),
             ),
             $definition->getArguments()
         );
@@ -423,10 +423,10 @@ public function testOptionalScalarArgsDontMessUpOrder()
         $definition = $container->getDefinition('with_optional_scalar');
         $this->assertEquals(
             array(
-                new Reference(A::class),
+                new TypedReference(A::class, A::class, MultipleArgumentsOptionalScalar::class),
                 // use the default value
                 'default_val',
-                new Reference(Lille::class),
+                new TypedReference(Lille::class, Lille::class),
             ),
             $definition->getArguments()
         );
@@ -447,8 +447,8 @@ public function testOptionalScalarArgsNotPassedIfLast()
         $definition = $container->getDefinition('with_optional_scalar_last');
         $this->assertEquals(
             array(
-                new Reference(A::class),
-                new Reference(Lille::class),
+                new TypedReference(A::class, A::class, MultipleArgumentsOptionalScalarLast::class),
+                new TypedReference(Lille::class, Lille::class, MultipleArgumentsOptionalScalarLast::class),
             ),
             $definition->getArguments()
         );
@@ -486,7 +486,7 @@ public function testSetterInjection()
         );
         // test setFoo args
         $this->assertEquals(
-            array(new Reference(Foo::class)),
+            array(new TypedReference(Foo::class, Foo::class, SetterInjection::class)),
             $methodCalls[1][1]
         );
     }
@@ -515,7 +515,7 @@ public function testExplicitMethodInjection()
             array_column($methodCalls, 0)
         );
         $this->assertEquals(
-            array(new Reference(A::class)),
+            array(new TypedReference(A::class, A::class, SetterInjection::class)),
             $methodCalls[0][1]
         );
     }
@@ -526,7 +526,7 @@ public function testTypedReference()
 
         $container
             ->register('bar', Bar::class)
-            ->setProperty('a', array(new TypedReference(A::class, A::class)))
+            ->setProperty('a', array(new TypedReference(A::class, A::class, Bar::class)))
         ;
 
         $pass = new AutowirePass();
@@ -629,7 +629,7 @@ public function testEmptyStringIsKept()
         (new ResolveClassPass())->process($container);
         (new AutowirePass())->process($container);
 
-        $this->assertEquals(array(new Reference(A::class), '', new Reference(Lille::class)), $container->getDefinition('foo')->getArguments());
+        $this->assertEquals(array(new TypedReference(A::class, A::class, MultipleArgumentsOptionalScalar::class), '', new TypedReference(Lille::class, Lille::class)), $container->getDefinition('foo')->getArguments());
     }
 
     public function testWithFactory()
@@ -644,7 +644,7 @@ public function testWithFactory()
         (new ResolveClassPass())->process($container);
         (new AutowirePass())->process($container);
 
-        $this->assertEquals(array(new Reference(Foo::class)), $definition->getArguments());
+        $this->assertEquals(array(new TypedReference(Foo::class, Foo::class, A::class)), $definition->getArguments());
     }
 
     /**

src/Symfony/Component/DependencyInjection/Tests/Compiler/RegisterServiceSubscribersPassTest.php

@@ -18,6 +18,8 @@
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\DependencyInjection\ServiceLocator;
+use Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition;
+use Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber;
 use Symfony\Component\DependencyInjection\TypedReference;
 
 require_once __DIR__.'/../Fixtures/includes/classes.php';
@@ -32,7 +34,7 @@ public function testInvalidClass()
     {
         $container = new ContainerBuilder();
 
-        $container->register('foo', 'stdClass')
+        $container->register('foo', CustomDefinition::class)
             ->addTag('container.service_subscriber')
         ;
 
@@ -48,7 +50,7 @@ public function testInvalidAttributes()
     {
         $container = new ContainerBuilder();
 
-        $container->register('foo', 'TestServiceSubscriber')
+        $container->register('foo', TestServiceSubscriber::class)
             ->addTag('container.service_subscriber', array('bar' => '123'))
         ;
 
@@ -60,7 +62,7 @@ public function testNoAttributes()
     {
         $container = new ContainerBuilder();
 
-        $container->register('foo', 'TestServiceSubscriber')
+        $container->register('foo', TestServiceSubscriber::class)
             ->addArgument(new Reference('container'))
             ->addTag('container.service_subscriber')
         ;
@@ -75,10 +77,10 @@ public function testNoAttributes()
         $this->assertSame(ServiceLocator::class, $locator->getClass());
 
         $expected = array(
-            'TestServiceSubscriber' => new ServiceClosureArgument(new TypedReference('TestServiceSubscriber', 'TestServiceSubscriber')),
-            'stdClass' => new ServiceClosureArgument(new TypedReference('stdClass', 'stdClass', ContainerInterface::IGNORE_ON_INVALID_REFERENCE)),
-            'bar' => new ServiceClosureArgument(new TypedReference('stdClass', 'stdClass')),
-            'baz' => new ServiceClosureArgument(new TypedReference('stdClass', 'stdClass', ContainerInterface::IGNORE_ON_INVALID_REFERENCE)),
+            TestServiceSubscriber::class => new ServiceClosureArgument(new TypedReference(TestServiceSubscriber::class, TestServiceSubscriber::class, TestServiceSubscriber::class)),
+            CustomDefinition::class => new ServiceClosureArgument(new TypedReference(CustomDefinition::class, CustomDefinition::class, TestServiceSubscriber::class, ContainerInterface::IGNORE_ON_INVALID_REFERENCE)),
+            'bar' => new ServiceClosureArgument(new TypedReference(CustomDefinition::class, CustomDefinition::class, TestServiceSubscriber::class)),
+            'baz' => new ServiceClosureArgument(new TypedReference(CustomDefinition::class, CustomDefinition::class, TestServiceSubscriber::class, ContainerInterface::IGNORE_ON_INVALID_REFERENCE)),
         );
 
         $this->assertEquals($expected, $locator->getArgument(0));
@@ -88,7 +90,7 @@ public function testWithAttributes()
     {
         $container = new ContainerBuilder();
 
-        $container->register('foo', 'TestServiceSubscriber')
+        $container->register('foo', TestServiceSubscriber::class)
             ->setAutowired(true)
             ->addArgument(new Reference('container'))
             ->addTag('container.service_subscriber', array('key' => 'bar', 'id' => 'bar'))
@@ -105,31 +107,31 @@ public function testWithAttributes()
         $this->assertSame(ServiceLocator::class, $locator->getClass());
 
         $expected = array(
-            'TestServiceSubscriber' => new ServiceClosureArgument(new TypedReference('TestServiceSubscriber', 'TestServiceSubscriber')),
-            'stdClass' => new ServiceClosureArgument(new TypedReference('stdClass', 'stdClass', ContainerInterface::IGNORE_ON_INVALID_REFERENCE)),
-            'bar' => new ServiceClosureArgument(new TypedReference('bar', 'stdClass')),
-            'baz' => new ServiceClosureArgument(new TypedReference('stdClass', 'stdClass', ContainerInterface::IGNORE_ON_INVALID_REFERENCE)),
+            TestServiceSubscriber::class => new ServiceClosureArgument(new TypedReference(TestServiceSubscriber::class, TestServiceSubscriber::class, TestServiceSubscriber::class)),
+            CustomDefinition::class => new ServiceClosureArgument(new TypedReference(CustomDefinition::class, CustomDefinition::class, TestServiceSubscriber::class, ContainerInterface::IGNORE_ON_INVALID_REFERENCE)),
+            'bar' => new ServiceClosureArgument(new TypedReference('bar', CustomDefinition::class, TestServiceSubscriber::class)),
+            'baz' => new ServiceClosureArgument(new TypedReference(CustomDefinition::class, CustomDefinition::class, TestServiceSubscriber::class, ContainerInterface::IGNORE_ON_INVALID_REFERENCE)),
         );
 
         $this->assertEquals($expected, $locator->getArgument(0));
     }
 
     /**
      * @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
-     * @expectedExceptionMessage Service key "test" does not exist in the map returned by TestServiceSubscriber::getSubscribedServices() for service "foo_service".
+     * @expectedExceptionMessage Service key "test" does not exist in the map returned by "Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber::getSubscribedServices()" for service "foo_service".
      */
     public function testExtraServiceSubscriber()
     {
         $container = new ContainerBuilder();
-        $container->register('foo_service', 'TestServiceSubscriber')
+        $container->register('foo_service', TestServiceSubscriber::class)
             ->setAutowired(true)
             ->addArgument(new Reference('container'))
             ->addTag('container.service_subscriber', array(
                 'key' => 'test',
-                'id' => 'TestServiceSubscriber',
+                'id' => TestServiceSubscriber::class,
             ))
         ;
-        $container->register('TestServiceSubscriber', 'TestServiceSubscriber');
+        $container->register(TestServiceSubscriber::class, TestServiceSubscriber::class);
         $container->compile();
     }
 }

src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php

@@ -25,6 +25,7 @@
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Loader\YamlFileLoader;
 use Symfony\Component\DependencyInjection\ServiceLocator;
+use Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber;
 use Symfony\Component\DependencyInjection\Variable;
 use Symfony\Component\ExpressionLanguage\Expression;
 
@@ -557,15 +558,15 @@ public function testServiceLocator()
     public function testServiceSubscriber()
     {
         $container = new ContainerBuilder();
-        $container->register('foo_service', 'TestServiceSubscriber')
+        $container->register('foo_service', TestServiceSubscriber::class)
             ->setAutowired(true)
             ->addArgument(new Reference('container'))
             ->addTag('container.service_subscriber', array(
                 'key' => 'bar',
-                'id' => 'TestServiceSubscriber',
+                'id' => TestServiceSubscriber::class,
             ))
         ;
-        $container->register('TestServiceSubscriber', 'TestServiceSubscriber');
+        $container->register(TestServiceSubscriber::class, TestServiceSubscriber::class);
         $container->compile();
 
         $dumper = new PhpDumper($container);

src/Symfony/Component/DependencyInjection/Tests/Fixtures/TestServiceSubscriber.php

@@ -0,0 +1,22 @@
+<?php
+
+namespace Symfony\Component\DependencyInjection\Tests\Fixtures;
+
+use Symfony\Component\DependencyInjection\ServiceSubscriberInterface;
+
+class TestServiceSubscriber implements ServiceSubscriberInterface
+{
+    public function __construct($container)
+    {
+    }
+
+    public static function getSubscribedServices()
+    {
+        return array(
+            __CLASS__,
+            '?'.CustomDefinition::class,
+            'bar' => CustomDefinition::class,
+            'baz' => '?'.CustomDefinition::class,
+        );
+    }
+}

src/Symfony/Component/DependencyInjection/Tests/Fixtures/includes/classes.php

@@ -108,20 +108,3 @@ public function __construct($lazyValues)
         $this->lazyValues = $lazyValues;
     }
 }
-
-class TestServiceSubscriber implements ServiceSubscriberInterface
-{
-    public function __construct($container)
-    {
-    }
-
-    public static function getSubscribedServices()
-    {
-        return array(
-            __CLASS__,
-            '?stdClass',
-            'bar' => 'stdClass',
-            'baz' => '?stdClass',
-        );
-    }
-}