feature #22175 [DI] add ServiceLocatorTagPass::register() to share se…

…rvice locators (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] add ServiceLocatorTagPass::register() to share service locators

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

Right now, one service locator is created per controller / service subscriber. But since service locators are stateless, this is just wasting resources when several controllers have the exact same set of services managed by their locators (as would be the case when registering the new `AbstractController` as a service subscribers).

This PR fixes this issue, and a few related others found along the way.

Commits
-------

8ff764b [DI] add ServiceLocatorTagPass::register() to share service locators

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TranslatorPass.php

@@ -11,11 +11,10 @@
 
 namespace Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler;
 
-use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
-use Symfony\Component\DependencyInjection\ServiceLocator;
+use Symfony\Component\DependencyInjection\Compiler\ServiceLocatorTagPass;
 
 class TranslatorPass implements CompilerPassInterface
 {
@@ -46,7 +45,7 @@ public function process(ContainerBuilder $container)
 
         $container
             ->findDefinition('translator.default')
-            ->replaceArgument(0, (new Definition(ServiceLocator::class, array($loaderRefs)))->addTag('container.service_locator'))
+            ->replaceArgument(0, ServiceLocatorTagPass::register($container, $loaderRefs))
             ->replaceArgument(3, $loaders)
         ;
     }

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Compiler/AddConstraintValidatorsPassTest.php

@@ -41,11 +41,12 @@ public function testThatConstraintValidatorServicesAreProcessed()
         $addConstraintValidatorsPass = new AddConstraintValidatorsPass();
         $addConstraintValidatorsPass->process($container);
 
-        $this->assertEquals((new Definition(ServiceLocator::class, array(array(
+        $expected = (new Definition(ServiceLocator::class, array(array(
             Validator1::class => new ServiceClosureArgument(new Reference('my_constraint_validator_service1')),
             'my_constraint_validator_alias1' => new ServiceClosureArgument(new Reference('my_constraint_validator_service1')),
             Validator2::class => new ServiceClosureArgument(new Reference('my_constraint_validator_service2')),
-        ))))->addTag('container.service_locator'), $validatorFactory->getArgument(0));
+        ))))->addTag('container.service_locator')->setPublic(false);
+        $this->assertEquals($expected, $container->getDefinition((string) $validatorFactory->getArgument(0)));
     }
 
     public function testThatCompilerPassIsIgnoredIfThereIsNoConstraintValidatorFactoryDefinition()

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Compiler/FormPassTest.php

@@ -53,7 +53,7 @@ public function testAddTaggedTypes()
             (new Definition(ServiceLocator::class, array(array(
                 __CLASS__.'_Type1' => new ServiceClosureArgument(new Reference('my.type1')),
                 __CLASS__.'_Type2' => new ServiceClosureArgument(new Reference('my.type2')),
-            ))))->addTag('container.service_locator'),
+            ))))->addTag('container.service_locator')->setPublic(false),
             $extDefinition->getArgument(0)
         );
     }

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Compiler/TranslatorPassTest.php

@@ -12,45 +12,39 @@
 namespace Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\Compiler;
 
 use PHPUnit\Framework\TestCase;
+use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\TranslatorPass;
+use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Reference;
-use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\TranslatorPass;
-use Symfony\Component\DependencyInjection\ServiceLocator;
 
 class TranslatorPassTest extends TestCase
 {
     public function testValidCollector()
     {
-        $definition = $this->getMockBuilder('Symfony\Component\DependencyInjection\Definition')->getMock();
-        $definition->expects($this->at(0))
-            ->method('addMethodCall')
-            ->with('addLoader', array('xliff', new Reference('xliff')));
-        $definition->expects($this->at(1))
-            ->method('addMethodCall')
-            ->with('addLoader', array('xlf', new Reference('xliff')));
-
-        $container = $this->getMockBuilder('Symfony\Component\DependencyInjection\ContainerBuilder')->setMethods(array('hasDefinition', 'getDefinition', 'findTaggedServiceIds', 'findDefinition'))->getMock();
-        $container->expects($this->any())
-            ->method('hasDefinition')
-            ->will($this->returnValue(true));
-        $container->expects($this->once())
-            ->method('getDefinition')
-            ->will($this->returnValue($definition));
-        $container->expects($this->once())
-            ->method('findTaggedServiceIds')
-            ->will($this->returnValue(array('xliff' => array(array('alias' => 'xliff', 'legacy-alias' => 'xlf')))));
-        $container->expects($this->once())
-            ->method('findDefinition')
-            ->will($this->returnValue($translator = $this->getMockBuilder('Symfony\Component\DependencyInjection\Definition')->getMock()));
-        $translator->expects($this->at(0))
-            ->method('replaceArgument')
-            ->with(0, $this->equalTo((new Definition(ServiceLocator::class, array(array('xliff' => new Reference('xliff')))))->addTag('container.service_locator')))
-            ->willReturn($translator);
-        $translator->expects($this->at(1))
-            ->method('replaceArgument')
-            ->with(3, array('xliff' => array('xliff', 'xlf')))
-            ->willReturn($translator);
+        $loader = (new Definition())
+            ->addTag('translation.loader', array('alias' => 'xliff', 'legacy-alias' => 'xlf'));
+
+        $translator = (new Definition())
+            ->setArguments(array(null, null, null, null));
+
+        $container = new ContainerBuilder();
+        $container->setDefinition('translator.default', $translator);
+        $container->setDefinition('translation.loader', $loader);
+
         $pass = new TranslatorPass();
         $pass->process($container);
+
+        $expected = (new Definition())
+            ->addTag('translation.loader', array('alias' => 'xliff', 'legacy-alias' => 'xlf'))
+            ->addMethodCall('addLoader', array('xliff', new Reference('translation.loader')))
+            ->addMethodCall('addLoader', array('xlf', new Reference('translation.loader')))
+        ;
+        $this->assertEquals($expected, $loader);
+
+        $this->assertSame(array('translation.loader' => array('xliff', 'xlf')), $translator->getArgument(3));
+
+        $expected = array('translation.loader' => new ServiceClosureArgument(new Reference('translation.loader')));
+        $this->assertEquals($expected, $container->getDefinition((string) $translator->getArgument(0))->getArgument(0));
     }
 }

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

@@ -17,14 +17,13 @@
 use Symfony\Component\Console\Application;
 use Symfony\Component\DependencyInjection\Alias;
 use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
-use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
 use Symfony\Component\DependencyInjection\ChildDefinition;
+use Symfony\Component\DependencyInjection\Compiler\ServiceLocatorTagPass;
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\HttpKernel\DependencyInjection\Extension;
 use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Reference;
-use Symfony\Component\DependencyInjection\ServiceLocator;
 use Symfony\Component\Config\FileLocator;
 use Symfony\Component\Security\Core\Authorization\ExpressionLanguage;
 
@@ -262,10 +261,10 @@ private function createFirewalls($config, ContainerBuilder $container)
                 ->replaceArgument(2, new Reference($configId))
             ;
 
-            $contextRefs[$contextId] = new ServiceClosureArgument(new Reference($contextId));
+            $contextRefs[$contextId] = new Reference($contextId);
             $map[$contextId] = $matcher;
         }
-        $mapDef->replaceArgument(0, (new Definition(ServiceLocator::class, array($contextRefs)))->addTag('container.service_locator'));
+        $mapDef->replaceArgument(0, ServiceLocatorTagPass::register($container, $contextRefs));
         $mapDef->replaceArgument(1, new IteratorArgument($map));
 
         // add authentication providers to authentication manager

src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/RuntimeLoaderPass.php

@@ -11,12 +11,10 @@
 
 namespace Symfony\Bundle\TwigBundle\DependencyInjection\Compiler;
 
-use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+use Symfony\Component\DependencyInjection\Compiler\ServiceLocatorTagPass;
 use Symfony\Component\DependencyInjection\Reference;
-use Symfony\Component\DependencyInjection\Definition;
-use Symfony\Component\DependencyInjection\ServiceLocator;
 
 /**
  * Registers Twig runtime services.
@@ -38,9 +36,9 @@ public function process(ContainerBuilder $container)
                 continue;
             }
 
-            $mapping[$def->getClass()] = new ServiceClosureArgument(new Reference($id));
+            $mapping[$def->getClass()] = new Reference($id);
         }
 
-        $definition->replaceArgument(0, (new Definition(ServiceLocator::class, array($mapping)))->addTag('container.service_locator'));
+        $definition->replaceArgument(0, ServiceLocatorTagPass::register($container, $mapping));
     }
 }

src/Symfony/Bundle/TwigBundle/Tests/DependencyInjection/TwigExtensionTest.php

@@ -244,7 +244,7 @@ public function testRuntimeLoader()
         $container->compile();
 
         $loader = $container->getDefinition('twig.runtime_loader');
-        $args = $loader->getArgument(0)->getArgument(0);
+        $args = $container->getDefinition((string) $loader->getArgument(0))->getArgument(0);
         $this->assertArrayHasKey('Symfony\Bridge\Twig\Form\TwigRenderer', $args);
         $this->assertArrayHasKey('FooClass', $args);
         $this->assertEquals('twig.form.renderer', $args['Symfony\Bridge\Twig\Form\TwigRenderer']->getValues()[0]);

src/Symfony/Component/DependencyInjection/Argument/ServiceClosureArgument.php

@@ -11,8 +11,8 @@
 
 namespace Symfony\Component\DependencyInjection\Argument;
 
-use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+use Symfony\Component\DependencyInjection\Reference;
 
 /**
  * Represents a service wrapped in a memoizing closure.
@@ -28,11 +28,17 @@ public function __construct(Reference $reference)
         $this->values = array($reference);
     }
 
+    /**
+     * {@inheritdoc}
+     */
     public function getValues()
     {
         return $this->values;
     }
 
+    /**
+     * {@inheritdoc}
+     */
     public function setValues(array $values)
     {
         if (array(0) !== array_keys($values) || !($values[0] instanceof Reference || null === $values[0])) {

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

@@ -11,13 +11,11 @@
 
 namespace Symfony\Component\DependencyInjection\Compiler;
 
-use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
 use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\DependencyInjection\ServiceSubscriberInterface;
-use Symfony\Component\DependencyInjection\ServiceLocator;
 use Symfony\Component\DependencyInjection\TypedReference;
 
 /**
@@ -87,7 +85,7 @@ protected function processValue($value, $isRoot = false)
                 $serviceMap[$key] = new Reference($type);
             }
 
-            $subscriberMap[$key] = new ServiceClosureArgument(new TypedReference((string) $serviceMap[$key], $type, $optionalBehavior ?: ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE));
+            $subscriberMap[$key] = new TypedReference((string) $serviceMap[$key], $type, $optionalBehavior ?: ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE);
             unset($serviceMap[$key]);
         }
 
@@ -97,12 +95,7 @@ protected function processValue($value, $isRoot = false)
         }
 
         $serviceLocator = $this->serviceLocator;
-        $this->serviceLocator = 'container.'.$this->currentId.'.'.md5(serialize($value));
-        $this->container->register($this->serviceLocator, ServiceLocator::class)
-            ->addArgument($subscriberMap)
-            ->setPublic(false)
-            ->setAutowired($value->isAutowired())
-            ->addTag('container.service_locator');
+        $this->serviceLocator = (string) ServiceLocatorTagPass::register($this->container, $subscriberMap, $value->getAutowired());
 
         try {
             return parent::processValue($value);

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

@@ -11,7 +11,9 @@
 
 namespace Symfony\Component\DependencyInjection\Compiler;
 
+use Symfony\Component\DependencyInjection\Alias;
 use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
 use Symfony\Component\DependencyInjection\Reference;
@@ -22,7 +24,7 @@
  *
  * @author Nicolas Grekas <p@tchwork.com>
  */
-class ServiceLocatorTagPass extends AbstractRecursivePass
+final class ServiceLocatorTagPass extends AbstractRecursivePass
 {
     protected function processValue($value, $isRoot = false)
     {
@@ -48,7 +50,52 @@ protected function processValue($value, $isRoot = false)
             }
             $arguments[0][$k] = new ServiceClosureArgument($v);
         }
+        ksort($arguments[0]);
 
-        return $value->setArguments($arguments);
+        $value->setArguments($arguments);
+
+        if ($public = $value->isPublic()) {
+            $value->setPublic(false);
+        }
+        $id = 'service_locator.'.md5(serialize($value));
+
+        if ($isRoot) {
+            if ($id !== $this->currentId) {
+                $this->container->setAlias($id, new Alias($this->currentId, $public));
+            }
+
+            return $value;
+        }
+
+        $this->container->setDefinition($id, $value);
+
+        return new Reference($id);
+    }
+
+    /**
+     * @param ContainerBuilder $container
+     * @param Reference[]      $refMap
+     * @param int|bool         $autowired
+     *
+     * @return Reference
+     */
+    public static function register(ContainerBuilder $container, array $refMap, $autowired = false)
+    {
+        foreach ($refMap as $id => $ref) {
+            $refMap[$id] = new ServiceClosureArgument($ref);
+        }
+        ksort($refMap);
+
+        $locator = (new Definition(ServiceLocator::class))
+            ->addArgument($refMap)
+            ->setPublic(false)
+            ->setAutowired($autowired)
+            ->addTag('container.service_locator');
+
+        if (!$container->has($id = 'service_locator.'.md5(serialize($locator)))) {
+            $container->setDefinition($id, $locator);
+        }
+
+        return new Reference($id);
     }
 }

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

@@ -99,12 +99,12 @@ protected function getFooServiceService()
     {
         return $this->services['foo_service'] = new \TestServiceSubscriber(new \Symfony\Component\DependencyInjection\ServiceLocator(array('TestServiceSubscriber' => function () {
             $f = function (\TestServiceSubscriber $v) { return $v; }; return $f(${($_ = isset($this->services['TestServiceSubscriber']) ? $this->services['TestServiceSubscriber'] : $this->get('TestServiceSubscriber')) && false ?: '_'});
-        }, 'stdClass' => function () {
-            $f = function (\stdClass $v = null) { return $v; }; return $f(${($_ = isset($this->services['autowired.stdClass']) ? $this->services['autowired.stdClass'] : $this->getAutowired_StdClassService()) && false ?: '_'});
         }, 'bar' => function () {
             $f = function (\stdClass $v) { return $v; }; return $f(${($_ = isset($this->services['TestServiceSubscriber']) ? $this->services['TestServiceSubscriber'] : $this->get('TestServiceSubscriber')) && false ?: '_'});
         }, 'baz' => function () {
             $f = function (\stdClass $v = null) { return $v; }; return $f(${($_ = isset($this->services['autowired.stdClass']) ? $this->services['autowired.stdClass'] : $this->getAutowired_StdClassService()) && false ?: '_'});
+        }, 'stdClass' => function () {
+            $f = function (\stdClass $v = null) { return $v; }; return $f(${($_ = isset($this->services['autowired.stdClass']) ? $this->services['autowired.stdClass'] : $this->getAutowired_StdClassService()) && false ?: '_'});
         })));
     }
 

src/Symfony/Component/Form/DependencyInjection/FormPass.php

@@ -12,14 +12,13 @@
 namespace Symfony\Component\Form\DependencyInjection;
 
 use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
-use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
 use Symfony\Component\DependencyInjection\Compiler\PriorityTaggedServiceTrait;
+use Symfony\Component\DependencyInjection\Compiler\ServiceLocatorTagPass;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
 use Symfony\Component\DependencyInjection\Reference;
-use Symfony\Component\DependencyInjection\ServiceLocator;
 
 /**
  * Adds all services with the tags "form.type", "form.type_extension" and
@@ -60,17 +59,16 @@ private function processFormTypes(ContainerBuilder $container, Definition $defin
     {
         // Get service locator argument
         $servicesMap = array();
-        $locator = $definition->getArgument(0);
 
         // Builds an array with fully-qualified type class names as keys and service IDs as values
         foreach ($container->findTaggedServiceIds($this->formTypeTag) as $serviceId => $tag) {
             $serviceDefinition = $container->getDefinition($serviceId);
 
             // Add form type service to the service locator
-            $servicesMap[$serviceDefinition->getClass()] = new ServiceClosureArgument(new Reference($serviceId));
+            $servicesMap[$serviceDefinition->getClass()] = new Reference($serviceId);
         }
 
-        return (new Definition(ServiceLocator::class, array($servicesMap)))->addTag('container.service_locator');
+        return ServiceLocatorTagPass::register($container, $servicesMap);
     }
 
     private function processFormTypeExtensions(ContainerBuilder $container)

src/Symfony/Component/Form/Tests/DependencyInjection/FormPassTest.php

@@ -51,7 +51,7 @@ public function testAddTaggedTypes()
             (new Definition(ServiceLocator::class, array(array(
                 __CLASS__.'_Type1' => new ServiceClosureArgument(new Reference('my.type1')),
                 __CLASS__.'_Type2' => new ServiceClosureArgument(new Reference('my.type2')),
-            ))))->addTag('container.service_locator'),
+            ))))->addTag('container.service_locator')->setPublic(false),
             $extDefinition->getArgument(0)
         );
     }