bug #22481 [FrameworkBundle] Restore 3.2-like behavior for FormPass, …

…to help BC with Sonata (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[FrameworkBundle] Restore 3.2-like behavior for FormPass, to help BC with Sonata

| 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        | -

I tried updating a Sonata project to 3.3, and found it broken.
The issue is that Sonata uses the constructor arguments of the `form.extension` to create its own `form.extension` service - but borrows its first args from the Symfony one.

Here is the form pass doing that:
https://github.com/sonata-project/SonataCoreBundle/blob/3.x/DependencyInjection/Compiler/FormFactoryCompilerPass.php
And the implementation of the form extension:
https://github.com/sonata-project/SonataCoreBundle/blob/3.x/DependencyInjection/SonataCoreExtension.php

Question: is this covered by the BC policy? It shouldn't to me, because that would prevent *any* service reconfiguration.

Thus, I'm proposing the attached patch, which basically reverts the deprecated `FormPass` in FrameworkBundle to its 3.2 state.

I added a check to the new `FormPass` in the Form component so that it doesn't overwrite such compatibility configurations.

See for corresponding fix on sonata-project/SonataCoreBundle#399

Commits
-------

c97b08e [FrameworkBundle] Restore 3.2-like behavior for FormPass, to help BC with Sonata

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

@@ -13,16 +13,76 @@
 
 @trigger_error(sprintf('The %s class is deprecated since version 3.3 and will be removed in 4.0. Use Symfony\Component\Form\DependencyInjection\FormPass instead.', FormPass::class), E_USER_DEPRECATED);
 
-use Symfony\Component\Form\DependencyInjection\FormPass as BaseFormPass;
+use Symfony\Component\DependencyInjection\Compiler\PriorityTaggedServiceTrait;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
 
 /**
  * Adds all services with the tags "form.type" and "form.type_guesser" as
  * arguments of the "form.extension" service.
  *
- * @deprecated since version 3.3, to be removed in 4.0. Use {@link BaseFormPass} instead.
- *
  * @author Bernhard Schussek <bschussek@gmail.com>
+ *
+ * @deprecated since version 3.3, to be removed in 4.0. Use FormPass in the Form component instead.
  */
-class FormPass extends BaseFormPass
+class FormPass implements CompilerPassInterface
 {
+    use PriorityTaggedServiceTrait;
+
+    public function process(ContainerBuilder $container)
+    {
+        if (!$container->hasDefinition('form.extension')) {
+            return;
+        }
+
+        $definition = $container->getDefinition('form.extension');
+
+        // Builds an array with fully-qualified type class names as keys and service IDs as values
+        $types = array();
+
+        foreach ($container->findTaggedServiceIds('form.type') as $serviceId => $tag) {
+            $serviceDefinition = $container->getDefinition($serviceId);
+            if (!$serviceDefinition->isPublic()) {
+                $serviceDefinition->setPublic(true);
+            }
+
+            // Support type access by FQCN
+            $types[$serviceDefinition->getClass()] = $serviceId;
+        }
+
+        $definition->replaceArgument(1, $types);
+
+        $typeExtensions = array();
+
+        foreach ($this->findAndSortTaggedServices('form.type_extension', $container) as $reference) {
+            $serviceId = (string) $reference;
+            $serviceDefinition = $container->getDefinition($serviceId);
+            if (!$serviceDefinition->isPublic()) {
+                $serviceDefinition->setPublic(true);
+            }
+
+            $tag = $serviceDefinition->getTag('form.type_extension');
+            if (isset($tag[0]['extended_type'])) {
+                $extendedType = $tag[0]['extended_type'];
+            } else {
+                throw new InvalidArgumentException(sprintf('Tagged form type extension must have the extended type configured using the extended_type/extended-type attribute, none was configured for the "%s" service.', $serviceId));
+            }
+
+            $typeExtensions[$extendedType][] = $serviceId;
+        }
+
+        $definition->replaceArgument(2, $typeExtensions);
+
+        // Find all services annotated with "form.type_guesser"
+        $guessers = array_keys($container->findTaggedServiceIds('form.type_guesser'));
+        foreach ($guessers as $serviceId) {
+            $serviceDefinition = $container->getDefinition($serviceId);
+            if (!$serviceDefinition->isPublic()) {
+                $serviceDefinition->setPublic(true);
+            }
+        }
+
+        $definition->replaceArgument(3, $guessers);
+    }
 }

src/Symfony/Bundle/FrameworkBundle/Resources/config/form.xml

@@ -36,6 +36,7 @@
             <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 -->
+            <argument>null</argument><!-- @deprecated argument in 3.3, to be removed in 4.0 -->
         </service>
 
         <!-- ValidatorTypeGuesser -->

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

@@ -13,12 +13,9 @@
 
 use PHPUnit\Framework\TestCase;
 use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\FormPass;
-use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
-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;
 
 /**
@@ -30,7 +27,8 @@ class FormPassTest extends TestCase
 {
     public function testDoNothingIfFormExtensionNotLoaded()
     {
-        $container = $this->createContainerBuilder();
+        $container = new ContainerBuilder();
+        $container->addCompilerPass(new FormPass());
 
         $container->compile();
 
@@ -39,33 +37,47 @@ public function testDoNothingIfFormExtensionNotLoaded()
 
     public function testAddTaggedTypes()
     {
-        $container = $this->createContainerBuilder();
+        $container = new ContainerBuilder();
+        $container->addCompilerPass(new FormPass());
 
-        $container->setDefinition('form.extension', $this->createExtensionDefinition());
+        $extDefinition = new Definition('Symfony\Component\Form\Extension\DependencyInjection\DependencyInjectionExtension');
+        $extDefinition->setArguments(array(
+            new Reference('service_container'),
+            array(),
+            array(),
+            array(),
+        ));
+
+        $container->setDefinition('form.extension', $extDefinition);
         $container->register('my.type1', __CLASS__.'_Type1')->addTag('form.type');
         $container->register('my.type2', __CLASS__.'_Type2')->addTag('form.type');
 
         $container->compile();
 
         $extDefinition = $container->getDefinition('form.extension');
 
-        $this->assertEquals(
-            (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')->setPublic(false),
-            $extDefinition->getArgument(0)
-        );
+        $this->assertEquals(array(
+            __CLASS__.'_Type1' => 'my.type1',
+            __CLASS__.'_Type2' => 'my.type2',
+        ), $extDefinition->getArgument(1));
     }
 
     /**
      * @dataProvider addTaggedTypeExtensionsDataProvider
      */
     public function testAddTaggedTypeExtensions(array $extensions, array $expectedRegisteredExtensions)
     {
-        $container = $this->createContainerBuilder();
+        $container = new ContainerBuilder();
+        $container->addCompilerPass(new FormPass());
 
-        $container->setDefinition('form.extension', $this->createExtensionDefinition());
+        $extDefinition = new Definition('Symfony\Component\Form\Extension\DependencyInjection\DependencyInjectionExtension', array(
+            new Reference('service_container'),
+            array(),
+            array(),
+            array(),
+        ));
+
+        $container->setDefinition('form.extension', $extDefinition);
 
         foreach ($extensions as $serviceId => $tag) {
             $container->register($serviceId, 'stdClass')->addTag('form.type_extension', $tag);
@@ -74,7 +86,7 @@ public function testAddTaggedTypeExtensions(array $extensions, array $expectedRe
         $container->compile();
 
         $extDefinition = $container->getDefinition('form.extension');
-        $this->assertEquals($expectedRegisteredExtensions, $extDefinition->getArgument(1));
+        $this->assertSame($expectedRegisteredExtensions, $extDefinition->getArgument(2));
     }
 
     /**
@@ -90,11 +102,8 @@ public function addTaggedTypeExtensionsDataProvider()
                     'my.type_extension3' => array('extended_type' => 'type2'),
                 ),
                 array(
-                    'type1' => new IteratorArgument(array(
-                        new Reference('my.type_extension1'),
-                        new Reference('my.type_extension2'),
-                    )),
-                    'type2' => new IteratorArgument(array(new Reference('my.type_extension3'))),
+                    'type1' => array('my.type_extension1', 'my.type_extension2'),
+                    'type2' => array('my.type_extension3'),
                 ),
             ),
             array(
@@ -107,16 +116,8 @@ public function addTaggedTypeExtensionsDataProvider()
                     'my.type_extension6' => array('extended_type' => 'type2', 'priority' => 1),
                 ),
                 array(
-                    'type1' => new IteratorArgument(array(
-                        new Reference('my.type_extension2'),
-                        new Reference('my.type_extension1'),
-                        new Reference('my.type_extension3'),
-                    )),
-                    'type2' => new IteratorArgument(array(
-                        new Reference('my.type_extension4'),
-                        new Reference('my.type_extension5'),
-                        new Reference('my.type_extension6'),
-                    )),
+                    'type1' => array('my.type_extension2', 'my.type_extension1', 'my.type_extension3'),
+                    'type2' => array('my.type_extension4', 'my.type_extension5', 'my.type_extension6'),
                 ),
             ),
         );
@@ -128,9 +129,17 @@ public function addTaggedTypeExtensionsDataProvider()
      */
     public function testAddTaggedFormTypeExtensionWithoutExtendedTypeAttribute()
     {
-        $container = $this->createContainerBuilder();
+        $container = new ContainerBuilder();
+        $container->addCompilerPass(new FormPass());
+
+        $extDefinition = new Definition('Symfony\Component\Form\Extension\DependencyInjection\DependencyInjectionExtension', array(
+            new Reference('service_container'),
+            array(),
+            array(),
+            array(),
+        ));
 
-        $container->setDefinition('form.extension', $this->createExtensionDefinition());
+        $container->setDefinition('form.extension', $extDefinition);
         $container->register('my.type_extension', 'stdClass')
             ->addTag('form.type_extension');
 
@@ -139,102 +148,67 @@ public function testAddTaggedFormTypeExtensionWithoutExtendedTypeAttribute()
 
     public function testAddTaggedGuessers()
     {
-        $container = $this->createContainerBuilder();
+        $container = new ContainerBuilder();
+        $container->addCompilerPass(new FormPass());
+
+        $extDefinition = new Definition('Symfony\Component\Form\Extension\DependencyInjection\DependencyInjectionExtension');
+        $extDefinition->setArguments(array(
+            new Reference('service_container'),
+            array(),
+            array(),
+            array(),
+        ));
 
         $definition1 = new Definition('stdClass');
         $definition1->addTag('form.type_guesser');
         $definition2 = new Definition('stdClass');
         $definition2->addTag('form.type_guesser');
 
-        $container->setDefinition('form.extension', $this->createExtensionDefinition());
+        $container->setDefinition('form.extension', $extDefinition);
         $container->setDefinition('my.guesser1', $definition1);
         $container->setDefinition('my.guesser2', $definition2);
 
         $container->compile();
 
         $extDefinition = $container->getDefinition('form.extension');
 
-        $this->assertEquals(
-            new IteratorArgument(array(
-                new Reference('my.guesser1'),
-                new Reference('my.guesser2'),
-            )),
-            $extDefinition->getArgument(2)
-        );
+        $this->assertSame(array(
+            'my.guesser1',
+            'my.guesser2',
+        ), $extDefinition->getArgument(3));
     }
 
     /**
      * @dataProvider privateTaggedServicesProvider
      */
-    public function testPrivateTaggedServices($id, $tagName, callable $assertion, array $tagAttributes = array())
+    public function testPrivateTaggedServices($id, $tagName)
     {
-        $formPass = new FormPass();
         $container = new ContainerBuilder();
+        $container->addCompilerPass(new FormPass());
 
-        $container->setDefinition('form.extension', $this->createExtensionDefinition());
-        $container->register($id, 'stdClass')->setPublic(false)->addTag($tagName, $tagAttributes);
+        $extDefinition = new Definition('Symfony\Component\Form\Extension\DependencyInjection\DependencyInjectionExtension');
+        $extDefinition->setArguments(array(
+            new Reference('service_container'),
+            array(),
+            array(),
+            array(),
+        ));
 
-        $formPass->process($container);
+        $container->setDefinition('form.extension', $extDefinition);
+        $container->register($id, 'stdClass')->setPublic(false)->addTag($tagName, array('extended_type' => 'Foo'));
 
-        $assertion($container);
+        $container->compile();
+        $this->assertTrue($container->getDefinition($id)->isPublic());
     }
 
     public function privateTaggedServicesProvider()
     {
         return array(
-            array(
-                'my.type',
-                'form.type',
-                function (ContainerBuilder $container) {
-                    $formTypes = $container->getDefinition('form.extension')->getArgument(0);
-
-                    $this->assertInstanceOf(Reference::class, $formTypes);
-
-                    $locator = $container->getDefinition((string) $formTypes);
-                    $expectedLocatorMap = array(
-                        'stdClass' => new ServiceClosureArgument(new Reference('my.type')),
-                    );
-
-                    $this->assertInstanceOf(Definition::class, $locator);
-                    $this->assertEquals($expectedLocatorMap, $locator->getArgument(0));
-                },
-            ),
-            array(
-                'my.type_extension',
-                'form.type_extension',
-                function (ContainerBuilder $container) {
-                    $this->assertEquals(
-                        array('Symfony\Component\Form\Extension\Core\Type\FormType' => new IteratorArgument(array(new Reference('my.type_extension')))),
-                        $container->getDefinition('form.extension')->getArgument(1)
-                    );
-                },
-                array('extended_type' => 'Symfony\Component\Form\Extension\Core\Type\FormType'),
-            ),
-            array('my.guesser', 'form.type_guesser', function (ContainerBuilder $container) {
-                $this->assertEquals(new IteratorArgument(array(new Reference('my.guesser'))), $container->getDefinition('form.extension')->getArgument(2));
-            }),
+            array('my.type', 'form.type'),
+            array('my.type_extension', 'form.type_extension'),
+            array('my.guesser', 'form.type_guesser'),
         );
     }
-
-    private function createContainerBuilder()
-    {
-        $container = new ContainerBuilder();
-        $container->addCompilerPass(new FormPass());
-
-        return $container;
-    }
-
-    private function createExtensionDefinition()
-    {
-        $definition = new Definition('Symfony\Component\Form\Extension\DependencyInjection\DependencyInjectionExtension');
-        $definition->setArguments(array(
-            array(),
-            array(),
-            new IteratorArgument(array()),
-        ));
-
-        return $definition;
-    }
 }
 
 class FormPassTest_Type1 extends AbstractType

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

@@ -50,6 +50,9 @@ public function process(ContainerBuilder $container)
         }
 
         $definition = $container->getDefinition($this->formExtensionService);
+        if (new IteratorArgument(array()) != $definition->getArgument(2)) {
+            return;
+        }
         $definition->replaceArgument(0, $this->processFormTypes($container, $definition));
         $definition->replaceArgument(1, $this->processFormTypeExtensions($container));
         $definition->replaceArgument(2, $this->processFormTypeGuessers($container));