feature #22024 [DI] Introduce "container.service_locator" tag, replaces ServiceLocatorArgument (nicolas-grekas)

fabpot · fabpot · commit bfb5f8e3d740 · 2017-03-17T10:07:04.000-07:00
This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Introduce "container.service_locator" tag, replaces ServiceLocatorArgument

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

I first started working on adding this new "container.service_locator" tag, so here it is.
It allows defining and dumping service-locator services properly, where it wasn't possible previously (you had to create a DI extension to do so.)

Then I realized that this allowed us to entirely drop `ServiceLocatorArgument` and replace it with the more flexible `ServiceClosureArgument`.

This makes things simpler overall, see diff stat.

Commits
-------

5d230b5871 [DI] Introduce "container.service_locator" tag, replaces ServiceLocatorArgument
diff --git a/Console/Descriptor/TextDescriptor.php b/Console/Descriptor/TextDescriptor.php
@@ -17,7 +17,6 @@
 use Symfony\Component\DependencyInjection\Argument\ClosureProxyArgument;
 use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
 use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
-use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\ParameterBag\ParameterBag;
@@ -332,8 +331,6 @@ protected function describeContainerDefinition(Definition $definition, array $op
                     $argumentsInformation[] = sprintf('Service(%s)', (string) $argument);
                 } elseif ($argument instanceof IteratorArgument) {
                     $argumentsInformation[] = sprintf('Iterator (%d element(s))', count($argument->getValues()));
-                } elseif ($argument instanceof ServiceLocatorArgument) {
-                    $argumentsInformation[] = sprintf('ServiceLocator (%d service(s))', count($argument->getValues()));
                 } elseif ($argument instanceof ClosureProxyArgument) {
                     list($reference, $method) = $argument->getValues();
                     $argumentsInformation[] = sprintf('ClosureProxy(Service(%s)::%s())', $reference, $method);
diff --git a/Console/Descriptor/XmlDescriptor.php b/Console/Descriptor/XmlDescriptor.php
@@ -15,7 +15,6 @@
 use Symfony\Component\DependencyInjection\Argument\ClosureProxyArgument;
 use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
 use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
-use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\ParameterBag\ParameterBag;
@@ -436,12 +435,6 @@ private function getArgumentNodes(array $arguments, \DOMDocument $dom)
             } elseif ($argument instanceof IteratorArgument) {
                 $argumentXML->setAttribute('type', 'iterator');
 
-                foreach ($this->getArgumentNodes($argument->getValues(), $dom) as $childArgumentXML) {
-                    $argumentXML->appendChild($childArgumentXML);
-                }
-            } elseif ($argument instanceof ServiceLocatorArgument) {
-                $argumentXML->setAttribute('type', 'service-locator');
-
                 foreach ($this->getArgumentNodes($argument->getValues(), $dom) as $childArgumentXML) {
                     $argumentXML->appendChild($childArgumentXML);
                 }
diff --git a/DependencyInjection/Compiler/AddConstraintValidatorsPass.php b/DependencyInjection/Compiler/AddConstraintValidatorsPass.php
@@ -11,10 +11,12 @@
 
 namespace Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler;
 
-use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;
+use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\DependencyInjection\ServiceLocator;
 
 class AddConstraintValidatorsPass implements CompilerPassInterface
 {
@@ -33,12 +35,12 @@ public function process(ContainerBuilder $container)
             }
 
             if (isset($attributes[0]['alias'])) {
-                $validators[$attributes[0]['alias']] = new Reference($id);
+                $validators[$attributes[0]['alias']] = new ServiceClosureArgument(new Reference($id));
             }
 
-            $validators[$definition->getClass()] = new Reference($id);
+            $validators[$definition->getClass()] = new ServiceClosureArgument(new Reference($id));
         }
 
-        $container->getDefinition('validator.validator_factory')->replaceArgument(0, new ServiceLocatorArgument($validators));
+        $container->getDefinition('validator.validator_factory')->replaceArgument(0, (new Definition(ServiceLocator::class, array($validators)))->addTag('container.service_locator'));
     }
 }
diff --git a/Resources/config/form.xml b/Resources/config/form.xml
@@ -30,7 +30,7 @@
 
         <!-- DependencyInjectionExtension -->
         <service id="form.extension" class="Symfony\Component\Form\Extension\DependencyInjection\DependencyInjectionExtension" public="false">
-            <argument type="service-locator" /><!-- All services with tag "form.type" are stored in a service locator by FormPass -->
+            <argument /><!-- All services with tag "form.type" are stored in a service locator by FormPass -->
             <argument type="collection" /><!-- All services with tag "form.type_extension" are stored here by FormPass -->
             <argument type="iterator" /><!-- All services with tag "form.type_guesser" are stored here by FormPass -->
         </service>
diff --git a/Resources/config/session.xml b/Resources/config/session.xml
@@ -49,8 +49,13 @@
 
         <service id="session_listener" class="Symfony\Component\HttpKernel\EventListener\SessionListener">
             <tag name="kernel.event_subscriber" />
-            <argument type="service-locator">
-                <argument key="session" type="service" id="session" on-invalid="null" />
+            <argument type="service">
+                <service class="Symfony\Component\DependencyInjection\ServiceLocator">
+                    <tag name="container.service_locator" />
+                    <argument type="collection">
+                        <argument key="session" type="service" id="session" on-invalid="ignore" />
+                    </argument>
+                </service>
             </argument>
         </service>
 
diff --git a/Resources/config/test.xml b/Resources/config/test.xml
@@ -22,8 +22,13 @@
 
         <service id="test.session.listener" class="Symfony\Component\HttpKernel\EventListener\TestSessionListener">
             <tag name="kernel.event_subscriber" />
-            <argument type="service-locator">
-                <argument key="session" type="service" id="session" on-invalid="null" />
+            <argument type="service">
+                <service class="Symfony\Component\DependencyInjection\ServiceLocator">
+                    <tag name="container.service_locator" />
+                    <argument type="collection">
+                        <argument key="session" type="service" id="session" on-invalid="ignore" />
+                    </argument>
+                </service>
             </argument>
         </service>
     </services>
diff --git a/Resources/config/validator.xml b/Resources/config/validator.xml
@@ -57,7 +57,7 @@
         </service>
 
         <service id="validator.validator_factory" class="Symfony\Bundle\FrameworkBundle\Validator\ConstraintValidatorFactory" public="false">
-            <argument type="service-locator" /> <!-- Constraint validators locator -->
+            <argument /> <!-- Constraint validators locator -->
         </service>
 
         <service id="validator.expression" class="Symfony\Component\Validator\Constraints\ExpressionValidator">
diff --git a/Tests/Console/Descriptor/AbstractDescriptorTest.php b/Tests/Console/Descriptor/AbstractDescriptorTest.php
@@ -192,7 +192,7 @@ private function assertDescription($expectedDescription, $describedObject, array
         $this->getDescriptor()->describe($output, $describedObject, $options);
 
         if ('json' === $this->getFormat()) {
-            $this->assertEquals(json_decode($expectedDescription), json_decode($output->fetch()));
+            $this->assertEquals(json_encode(json_decode($expectedDescription), JSON_PRETTY_PRINT), json_encode(json_decode($output->fetch()), JSON_PRETTY_PRINT));
         } else {
             $this->assertEquals(trim($expectedDescription), trim(str_replace(PHP_EOL, "\n", $output->fetch())));
         }
diff --git a/Tests/Console/Descriptor/ObjectsProvider.php b/Tests/Console/Descriptor/ObjectsProvider.php
@@ -14,7 +14,6 @@
 use Symfony\Component\DependencyInjection\Alias;
 use Symfony\Component\DependencyInjection\Argument\ClosureProxyArgument;
 use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
-use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\ParameterBag\ParameterBag;
@@ -121,10 +120,6 @@ public static function getContainerDefinitions()
                     new Reference('definition_2'),
                 )))
                 ->addArgument(new ClosureProxyArgument('definition1', 'get'))
-                ->addArgument(new ServiceLocatorArgument(array(
-                    'def1' => new Reference('definition_1'),
-                    'def2' => new Reference('definition_2'),
-                )))
                 ->setFactory(array('Full\\Qualified\\FactoryClass', 'get')),
             'definition_2' => $definition2
                 ->setPublic(false)
diff --git a/Tests/DependencyInjection/Compiler/AddConstraintValidatorsPassTest.php b/Tests/DependencyInjection/Compiler/AddConstraintValidatorsPassTest.php
@@ -13,17 +13,19 @@
 
 use PHPUnit\Framework\TestCase;
 use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddConstraintValidatorsPass;
-use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;
+use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\DependencyInjection\ServiceLocator;
 
 class AddConstraintValidatorsPassTest extends TestCase
 {
     public function testThatConstraintValidatorServicesAreProcessed()
     {
         $container = new ContainerBuilder();
         $validatorFactory = $container->register('validator.validator_factory')
-            ->setArguments(array(new ServiceLocatorArgument(array())));
+            ->addArgument(array());
 
         $container->register('my_constraint_validator_service1', Validator1::class)
             ->addTag('validator.constraint_validator', array('alias' => 'my_constraint_validator_alias1'));
@@ -36,11 +38,11 @@ public function testThatConstraintValidatorServicesAreProcessed()
         $addConstraintValidatorsPass = new AddConstraintValidatorsPass();
         $addConstraintValidatorsPass->process($container);
 
-        $this->assertEquals(new ServiceLocatorArgument(array(
-            Validator1::class => new Reference('my_constraint_validator_service1'),
-            'my_constraint_validator_alias1' => new Reference('my_constraint_validator_service1'),
-            Validator2::class => new Reference('my_constraint_validator_service2'),
-        )), $validatorFactory->getArgument(0));
+        $this->assertEquals((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));
     }
 
     public function testThatCompilerPassIsIgnoredIfThereIsNoConstraintValidatorFactoryDefinition()
diff --git a/Tests/DependencyInjection/Compiler/FormPassTest.php b/Tests/DependencyInjection/Compiler/FormPassTest.php
@@ -14,10 +14,11 @@
 use PHPUnit\Framework\TestCase;
 use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\FormPass;
 use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
-use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;
+use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\DependencyInjection\ServiceLocator;
 use Symfony\Component\Form\AbstractType;
 
 /**
@@ -49,10 +50,10 @@ public function testAddTaggedTypes()
         $extDefinition = $container->getDefinition('form.extension');
 
         $this->assertEquals(
-            new ServiceLocatorArgument(array(
-                __CLASS__.'_Type1' => new Reference('my.type1'),
-                __CLASS__.'_Type2' => new Reference('my.type2'),
-            )),
+            (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'),
             $extDefinition->getArgument(0)
         );
     }
@@ -196,7 +197,7 @@ private function createExtensionDefinition()
     {
         $definition = new Definition('Symfony\Component\Form\Extension\DependencyInjection\DependencyInjectionExtension');
         $definition->setArguments(array(
-            new ServiceLocatorArgument(array()),
+            array(),
             array(),
             new IteratorArgument(array()),
         ));
diff --git a/Tests/DependencyInjection/FrameworkExtensionTest.php b/Tests/DependencyInjection/FrameworkExtensionTest.php
@@ -15,6 +15,7 @@
 use Symfony\Bundle\FullStack;
 use Symfony\Bundle\FrameworkBundle\Tests\TestCase;
 use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddAnnotationsCachedReaderPass;
+use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddConstraintValidatorsPass;
 use Symfony\Bundle\FrameworkBundle\DependencyInjection\FrameworkExtension;
 use Symfony\Component\Cache\Adapter\AdapterInterface;
 use Symfony\Component\Cache\Adapter\ApcuAdapter;
@@ -921,7 +922,7 @@ protected function createContainerFromFile($file, $data = array(), $resetCompile
             $container->getCompilerPassConfig()->setOptimizationPasses(array());
             $container->getCompilerPassConfig()->setRemovingPasses(array());
         }
-        $container->getCompilerPassConfig()->setBeforeRemovingPasses(array(new AddAnnotationsCachedReaderPass()));
+        $container->getCompilerPassConfig()->setBeforeRemovingPasses(array(new AddAnnotationsCachedReaderPass(), new AddConstraintValidatorsPass()));
         $container->compile();
 
         return self::$containerCache[$cacheKey] = $container;
diff --git a/Tests/Fixtures/Descriptor/builder_1_arguments.json b/Tests/Fixtures/Descriptor/builder_1_arguments.json
@@ -64,17 +64,7 @@
                         "id": "definition1"
                     },
                     "get"
-                ],
-                {
-                    "def1": {
-                        "type": "service",
-                        "id": "definition_1"
-                    },
-                    "def2": {
-                        "type": "service",
-                        "id": "definition_2"
-                    }
-                }
+                ]
             ],
             "file": null,
             "factory_class": "Full\\Qualified\\FactoryClass",
diff --git a/Tests/Fixtures/Descriptor/builder_1_arguments.xml b/Tests/Fixtures/Descriptor/builder_1_arguments.xml
@@ -24,10 +24,6 @@
       <argument type="service" id="definition_2"/>
     </argument>
     <argument type="closure-proxy" id="definition1" method="get"/>
-    <argument type="service-locator">
-      <argument key="def1" type="service" id="definition_1"/>
-      <argument key="def2" type="service" id="definition_2"/>
-    </argument>
   </definition>
   <service id="service_container" class="Symfony\Component\DependencyInjection\ContainerBuilder"/>
 </container>
diff --git a/Tests/Fixtures/Descriptor/definition_arguments_1.json b/Tests/Fixtures/Descriptor/definition_arguments_1.json
@@ -62,17 +62,7 @@
                 "id": "definition1"
             },
             "get"
-        ],
-        {
-            "def1": {
-                "type": "service",
-                "id": "definition_1"
-            },
-            "def2": {
-                "type": "service",
-                "id": "definition_2"
-            }
-        }
+        ]
     ],
     "file": null,
     "factory_class": "Full\\Qualified\\FactoryClass",
diff --git a/Tests/Fixtures/Descriptor/definition_arguments_1.txt b/Tests/Fixtures/Descriptor/definition_arguments_1.txt
@@ -18,6 +18,5 @@
                    Array (3 element(s))                       
                    Iterator (2 element(s))                    
                    ClosureProxy(Service(definition1)::get())  
-                   ServiceLocator (2 service(s))              
  ---------------- ------------------------------------------- 
 
diff --git a/Tests/Fixtures/Descriptor/definition_arguments_1.xml b/Tests/Fixtures/Descriptor/definition_arguments_1.xml
@@ -21,8 +21,4 @@
     <argument type="service" id="definition_2"/>
   </argument>
   <argument type="closure-proxy" id="definition1" method="get"/>
-  <argument type="service-locator">
-    <argument key="def1" type="service" id="definition_1"/>
-    <argument key="def2" type="service" id="definition_2"/>
-  </argument>
 </definition>

Original file line number	Diff line number	Diff line change
`@@ -11,10 +11,12 @@`
`11`	`11`
`12`	`12`	`namespace Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler;`
`13`	`13`
`14`		`-use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;`
	`14`	`+use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;`
`15`	`15`	`use Symfony\Component\DependencyInjection\ContainerBuilder;`
`16`	`16`	`use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;`
	`17`	`+use Symfony\Component\DependencyInjection\Definition;`
`17`	`18`	`use Symfony\Component\DependencyInjection\Reference;`
	`19`	`+use Symfony\Component\DependencyInjection\ServiceLocator;`
`18`	`20`
`19`	`21`	`class AddConstraintValidatorsPass implements CompilerPassInterface`
`20`	`22`	`{`
`@@ -33,12 +35,12 @@ public function process(ContainerBuilder $container)`
`33`	`35`	`}`
`34`	`36`
`35`	`37`	`if (isset($attributes[0]['alias'])) {`
`36`		`- $validators[$attributes[0]['alias']] = new Reference($id);`
	`38`	`+ $validators[$attributes[0]['alias']] = new ServiceClosureArgument(new Reference($id));`
`37`	`39`	`}`
`38`	`40`
`39`		`- $validators[$definition->getClass()] = new Reference($id);`
	`41`	`+ $validators[$definition->getClass()] = new ServiceClosureArgument(new Reference($id));`
`40`	`42`	`}`
`41`	`43`
`42`		`- $container->getDefinition('validator.validator_factory')->replaceArgument(0, new ServiceLocatorArgument($validators));`
	`44`	`+ $container->getDefinition('validator.validator_factory')->replaceArgument(0, (new Definition(ServiceLocator::class, array($validators)))->addTag('container.service_locator'));`
`43`	`45`	`}`
`44`	`46`	`}`
Original file line number	Diff line number	Diff line change
`@@ -192,7 +192,7 @@ private function assertDescription($expectedDescription, $describedObject, array`
`192`	`192`	`$this->getDescriptor()->describe($output, $describedObject, $options);`
`193`	`193`
`194`	`194`	`if ('json' === $this->getFormat()) {`
`195`		`- $this->assertEquals(json_decode($expectedDescription), json_decode($output->fetch()));`
	`195`	`+ $this->assertEquals(json_encode(json_decode($expectedDescription), JSON_PRETTY_PRINT), json_encode(json_decode($output->fetch()), JSON_PRETTY_PRINT));`
`196`	`196`	`} else {`
`197`	`197`	`$this->assertEquals(trim($expectedDescription), trim(str_replace(PHP_EOL, "\n", $output->fetch())));`
`198`	`198`	`}`