[Form] allow form types + form type extensions + form type guessers to be private services #21690

hhamon · 2017-02-20T16:01:06Z

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

This pull request is about making internal form services (aka form types, form type extensions and form type guessers) private. They used to be public until Symfony 3.2 for one valid reason: lazyness. However, Symfony 3.3 now comes with built-in mechanism to support effective lazy loading of private services with service locators and proxies.

This PR makes the DependencyInjectionExtension class of the Form component leverage these new DI component mechanisms. Form types, form type extensions and form type guessers can now be declared private as a best practice. We decided to make these services private as of Symfony 3.3 and of course it would break BC. But this PR introduces a BC layer using a Symfony trick to keep internal form services public. The service container currently has a known issue where private services are not really private if they're referenced by at least two other services in the container. We use this trick to maintain the legacy services public even though the new API relies on private ones. This trick is done thanks to the deprecated.form.registry and deprecated.form.registry.csrf fake services that will be removed in Symfony 4.0.

hhamon · 2017-02-20T16:01:41Z

Related to #21553.

chalasr · 2017-02-20T19:42:49Z

src/Symfony/Component/Form/Extension/DependencyInjection/DependencyInjectionExtension.php

+     * @param iterable[]         $typeExtensionServices
+     * @param iterable[]         $guesserServices
+     */
+    public function __construct(ContainerInterface $typeContainer, array $typeExtensionServices, array $guesserServices, array $guesserServiceIds = null)


I would not rename $container to reduce the diff

chalasr · 2017-02-20T19:49:40Z

src/Symfony/Bundle/FrameworkBundle/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" id="service_container" />
+            <argument type="service-locator" />


According to the current deprecation the fourth argument should be removed

Yep good catch!

chalasr · 2017-02-20T20:06:03Z

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

        }

+        $definition->replaceArgument(0, new ServiceLocatorArgument($servicesMap));


I'm wondering if mixing types/guessers/extensions in the same locator is a good thing here, as we have still 3 constructor arguments that's a bit confusing to me.
Instead, couldn't guesserServices be an IteratorArgument of guessers? It seems we only iterate over them to build a chain one, no need for having them identified right?

Yes It's on the way! This is what I'm currently doing :)

Why not 2 service locators, and one iterator?

So the DependencyInjectionExtension::__construct() method signature would be:

public function __construct(ContainerInterface $typesLocator, ContainerInterface $typeExtensionsLocator, iterator $guessers)

Might not be the easiest for BC though.

Oh, actually $typeExtensionServiceIds cannot easily be a locator, as it is a map of multiple type extension services indexed by type. Symfony's ServiceLocator does not handle collection of services as values.
But couldn't it be a collection of iterables indexed by type?

Edit: actually in order to keep things simple, I think the current implementation is probably safer.

chalasr · 2017-02-20T20:48:09Z

src/Symfony/Component/Form/Extension/DependencyInjection/DependencyInjectionExtension.php

+     *
+     * @param ContainerInterface $typeContainer
+     * @param iterable[]         $typeExtensionServices
+     * @param iterable[]         $guesserServices


iterable[] make me understand that the value should be an array of iterables.
iterable|FormTypeGuesserInterface[] instead?

Yes it can be an array or an iterator.

chalasr · 2017-02-20T20:52:01Z

src/Symfony/Component/Form/Extension/DependencyInjection/DependencyInjectionExtension.php

+                    $service = $this->typeContainer->get($serviceId);
+                }
+
+                $guessers[] = $service;


In fact, guessers do not need to be instantiated at this point.
I think we should register FormTypeGuesserChain as a service, pass it guessers as an IteratorArgument and deprecate both $guesserServices and $guesserServiceIds in favor of a FormTypeGuesserInterface $guesser here, even more lazy. Not sure how it would be hard to keep it BC though, maybe deprecating the whole class would be simpler.

Yes that's probably a good thing! I'll check this tomorrow with @nicolas-grekas.

Not sure we need FormTypeGuesserChain to be a service, but simply pass it the guessers iterator (so allow FormTypeGuesserChain to accept iterable).
IMHO the extension is in charge of building the whole thing and the use of the FormTypeGuesserChain is an implementation detail. There is no need to pass the guesser chain as a dependency.

There is an issue with FormTypeGuesserChain, though... its constructor. It iterates over the guessers and this would trigger the loading too soon anyway.

I'm not sure lazy loading here is a big deal though, as FormExtensionInterface::getTypeGuesser() is called in FormRegistryInterface::getTypeGuesser(), called in Formfactory::createBuilderForProperty() and is used directly to guess types by iterating over every guessers.

IMHO the extension is in charge of building the whole thing

Too many responsibilities to me.

and the use of the FormTypeGuesserChain is an implementation detail. There is no need to pass the guesser chain as a dependency.

The fact the guessers are multiple services is a detail IMO. At the end we have only one FormTypeGuesserInterface $guesser, the class could be usable with an iterable or a simple guesser, which looks better to me. Also it'd be consistent with other places in the core.

There is an issue with FormTypeGuesserChain, though... its constructor. It iterates over the guessers and this would trigger the loading too soon anyway.

Just validation/merge, can be moved at compile time I think.

I thought about deprecating the whole class and replacing it with a new one. But after discussing with @nicolas-grekas it requires too much efforts then to maintain both implementations. This is why we choose to adapt the class itself. One class to maintain and one tests suite. It's just a bit more difficult to read and understand the code at the moment because of the mixed two layers.

It's just a bit more difficult to read and understand the code at the moment because of the mixed two layers.

Indeed :)
I'm unable to say if the BC layer is correct right now, but the approach looks better!

Unit tests prove the BC layer is guaranteed ;)

ogizanagi · 2017-02-20T20:29:40Z

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

-            __CLASS__.'_Type1' => 'my.type1',
-            __CLASS__.'_Type2' => 'my.type2',
-        ), $extDefinition->getArgument(1));
+        $this->assertSame(


This change should be reverted I guess.

The code style and the assertSame change isn't related to this PR, right?

ogizanagi · 2017-02-20T20:29:50Z

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

-            'my.guesser1',
-            'my.guesser2',
-        ), $extDefinition->getArgument(3));
+        $this->assertSame(


ogizanagi · 2017-02-20T21:14:07Z

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

        }

+        $definition->replaceArgument(0, new ServiceLocatorArgument($servicesMap));


Why not 2 service locators, and one iterator?

So the DependencyInjectionExtension::__construct() method signature would be:

public function __construct(ContainerInterface $typesLocator, ContainerInterface $typeExtensionsLocator, iterator $guessers)

Might not be the easiest for BC though.

ogizanagi · 2017-02-20T21:17:39Z

src/Symfony/Component/Form/Extension/DependencyInjection/DependencyInjectionExtension.php

+                    $service = $this->typeContainer->get($serviceId);
+                }
+
+                $guessers[] = $service;


Not sure we need FormTypeGuesserChain to be a service, but simply pass it the guessers iterator (so allow FormTypeGuesserChain to accept iterable).
IMHO the extension is in charge of building the whole thing and the use of the FormTypeGuesserChain is an implementation detail. There is no need to pass the guesser chain as a dependency.

There is an issue with FormTypeGuesserChain, though... its constructor. It iterates over the guessers and this would trigger the loading too soon anyway.

I'm not sure lazy loading here is a big deal though, as FormExtensionInterface::getTypeGuesser() is called in FormRegistryInterface::getTypeGuesser(), called in Formfactory::createBuilderForProperty() and is used directly to guess types by iterating over every guessers.

ogizanagi · 2017-02-20T21:29:58Z

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

-            __CLASS__.'_Type1' => 'my.type1',
-            __CLASS__.'_Type2' => 'my.type2',
-        ), $extDefinition->getArgument(1));
+        $this->assertSame(


Should be reverted too I think.

ogizanagi · 2017-02-20T21:31:08Z

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

-            'my.guesser2',
-        ), $extDefinition->getArgument(3));
+        $this->assertSame(
+            array(


sstok · 2017-02-21T09:14:10Z

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

-        ), $extDefinition->getArgument(1));
+        $locator = $extDefinition->getArgument(0);
+
+        $this->assertInstanceOf(ServiceLocatorArgument::class, $locator);


No need for assertInstanceOf as assertEquals fails on an type-class mismatch.

Fixed. thanks.

stof · 2017-02-21T09:49:45Z

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

-            // Support type access by FQCN
-            $types[$serviceDefinition->getClass()] = $serviceId;
+            // Add form type service to the service locator
+            $servicesMap[$serviceId] = new Reference($serviceId);


this is wrong. Form types are accessible only by class, not by service id

hhamon · 2017-02-27T09:29:20Z

@nicolas-grekas @HeahDude comments were addressed. I'll squash after your new view.

HeahDude

👍

nicolas-grekas

👍

nicolas-grekas · 2017-02-27T09:49:26Z

src/Symfony/Component/Form/Extension/DependencyInjection/DependencyInjectionExtension.php

+     * Constructor.
+     *
+     * @param ContainerInterface $typeContainer
+     * @param iterable[]         $typeExtensionServices


iterable[] is more precise: array of iterables

nicolas-grekas · 2017-02-27T09:51:06Z

src/Symfony/Component/Form/composer.json

@@ -25,8 +25,9 @@
    },
    "require-dev": {
        "doctrine/collections": "~1.0",
+        "psr/container": "^1.0",


not required (as symfony/dependency-injection already requires it)

hhamon · 2017-02-27T14:03:04Z

@stof @webmozart can I have your review please?

nicolas-grekas · 2017-02-27T14:12:28Z

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

        </service>

        <!-- ValidatorTypeGuesser -->
-        <service id="form.type_guesser.validator" class="Symfony\Component\Form\Extension\Validator\ValidatorTypeGuesser">
+        <service id="form.type_guesser.validator" class="Symfony\Component\Form\Extension\Validator\ValidatorTypeGuesser" public="false">


see PR description for why this is not a BC break (same for all added public="false")

chalasr

👍 (needs rebase)

… so that form types can be made private at some point.

fabpot · 2017-02-28T20:26:35Z

Thank you @hhamon.

…type guessers to be private services (hhamon) This PR was merged into the 3.3-dev branch. Discussion ---------- [Form] allow form types + form type extensions + form type guessers to be private services | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ This pull request is about making internal form services (aka form types, form type extensions and form type guessers) private. They used to be public until Symfony 3.2 for one valid reason: lazyness. However, Symfony 3.3 now comes with built-in mechanism to support effective lazy loading of private services with service locators and proxies. This PR makes the `DependencyInjectionExtension` class of the `Form` component leverage these new DI component mechanisms. Form types, form type extensions and form type guessers can now be declared private as a best practice. We decided to make these services private as of Symfony 3.3 and of course it would break BC. But this PR introduces a BC layer using a Symfony trick to keep internal form services public. The service container currently has a known issue where private services are not really private if they're referenced by at least two other services in the container. We use this trick to maintain the legacy services public even though the new API relies on private ones. This trick is done thanks to the `deprecated.form.registry` and `deprecated.form.registry.csrf` fake services that will be removed in Symfony 4.0. Commits ------- 600e75c [Form] use new service locator in DependencyInjectionExtension class, so that form types can be made private at some point.

carsonbot added Status: Needs Review Form Feature labels Feb 20, 2017

hhamon force-pushed the feat-private-form-types branch 6 times, most recently from 2ca9847 to 5fc62de Compare February 20, 2017 18:34

chalasr reviewed Feb 20, 2017

View reviewed changes

chalasr mentioned this pull request Feb 20, 2017

Remove some container injections in favor of service locators #21625

Merged

chalasr reviewed Feb 20, 2017

View reviewed changes

ogizanagi reviewed Feb 20, 2017

View reviewed changes

hhamon force-pushed the feat-private-form-types branch 5 times, most recently from deb23a2 to 27e2a1a Compare February 21, 2017 08:35

sstok reviewed Feb 21, 2017

View reviewed changes

hhamon force-pushed the feat-private-form-types branch 4 times, most recently from ab23d37 to e4beebf Compare February 21, 2017 10:04

stof reviewed Feb 21, 2017

View reviewed changes

hhamon force-pushed the feat-private-form-types branch 4 times, most recently from 0702a5b to 5c0a576 Compare February 21, 2017 10:38

hhamon force-pushed the feat-private-form-types branch from 0753fcd to ec76718 Compare February 27, 2017 09:44

HeahDude approved these changes Feb 27, 2017

View reviewed changes

nicolas-grekas approved these changes Feb 27, 2017

View reviewed changes

hhamon force-pushed the feat-private-form-types branch 13 times, most recently from 16cab61 to a354cac Compare February 27, 2017 13:50

nicolas-grekas reviewed Feb 27, 2017

View reviewed changes

chalasr approved these changes Feb 28, 2017

View reviewed changes

[Form] use new service locator in DependencyInjectionExtension class,…

600e75c

… so that form types can be made private at some point.

hhamon force-pushed the feat-private-form-types branch from a354cac to 600e75c Compare February 28, 2017 09:16

nicolas-grekas added Status: Reviewed and removed Status: Needs Review labels Feb 28, 2017

fabpot merged commit 600e75c into symfony:master Feb 28, 2017

BPScott mentioned this pull request Mar 6, 2017

[do not merge] Change the default directory structure and use new features of 3.3 symfony/symfony-standard#1034

Closed

javiereguiluz mentioned this pull request Mar 26, 2017

[Form] Types, extensions and type guessers can now be private services symfony/symfony-docs#7687

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

		}

		$definition->replaceArgument(0, new ServiceLocatorArgument($servicesMap));

[Form] allow form types + form type extensions + form type guessers to be private services #21690

[Form] allow form types + form type extensions + form type guessers to be private services #21690

Conversation

hhamon commented Feb 20, 2017 • edited Loading

hhamon commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

chalasr Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhamon commented Feb 27, 2017

HeahDude left a comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhamon commented Feb 27, 2017

Choose a reason for hiding this comment

chalasr left a comment

Choose a reason for hiding this comment

fabpot commented Feb 28, 2017

hhamon commented Feb 20, 2017 •

edited

Loading

ogizanagi Feb 20, 2017 •

edited

Loading

chalasr Feb 20, 2017 •

edited

Loading

chalasr Feb 20, 2017 •

edited

Loading

chalasr Feb 20, 2017 •

edited

Loading

nicolas-grekas Feb 27, 2017 •

edited

Loading