[FrameworkBundle] add support for prioritizing form type extension tags #19790

dmaicher · 2016-08-30T19:54:20Z

Q	A
Branch?	"master"
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#19735
License	MIT
Doc PR	symfony/symfony-docs#6958

This PR proposes to add support for priority on form.type_extension dependecyinjection tags to enable sorting/prioritizing form type extensions.

Issue was mentioned here: #19735

HeahDude · 2016-08-30T19:59:35Z

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

-        $definition->replaceArgument(2, $typeExtensions);
+        $sortedTypeExtensions = [];
+        foreach ($typeExtensions as $extendedType => $extensionsWithPriority) {
+            usort($extensionsWithPriority, function (array $a, array $b) {


You can use the https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php, it will do the job for you :)

Ah good to know 😊

But that would globally sort all form extensions. We only want to sort per extended type though? 😉

dmaicher · 2016-08-30T20:25:21Z

Actually I just realized that the usort function is not stable :(

http://www.php.net/manual/en/function.usort.php

"If two members compare as equal, their relative order in the sorted array is undefined."

This means we would have to sort differently to be 100% sure there is no BC break?

ogizanagi · 2016-08-30T22:18:05Z

What about using the ~~PriorityTaggedServiceTrait ?~~ (or at least a \SplPriorityQueue like other passes like the AddSecurityVotersPass or DecoratorServicePass ones ?)

dmaicher · 2016-08-31T07:15:58Z

It seems that also \SplPriorityQueue will not preserve the initial order of elements if the priority is equal (meaning its not a "stable sort"):

http://php.net/manual/de/class.splpriorityqueue.php#117067

HeahDude · 2016-08-31T07:30:36Z

@dmaicher There is no stable sorting by default either as it depends on the order of the services registration (which is not predictable unless using a custom pass, not sure it is worth it in that case though), and this is partially responsible for the issue you're trying to solve.

~~I don't see why you shouldn't use the trait.~~

Currently form type extensions cannot rely on other extensions, at least handling priorities will solve this.

dmaicher · 2016-08-31T07:41:28Z

@HeahDude But as you said currently without priorities it depends on the order of service registration. If we introduce a non-stable sorting then it might be a BC break though as the order of service registration is not 100% guaranteed to be used anymore for the form extensions (when all extensions have equal priority 0)?

HeahDude · 2016-08-31T07:45:25Z

currently without priorities it depends on the order of service registration

What I'm saying is that this order is not reliable already, so it is an acceptable BC break IMO because it does not change anything unless some extensions rely on each other, which is buggy right now unless using your new feature :)

dmaicher · 2016-08-31T07:47:37Z

@HeahDude Ok now I see what you mean 😊 If this "BC break" is acceptable then I can surely change it 😄

Other opinions on that?

nicolas-grekas · 2016-08-31T07:53:22Z

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

+        $sortedTypeExtensions = array();
+        foreach ($typeExtensions as $extendedType => $extensionsWithPriority) {
+            usort($extensionsWithPriority, function (array $a, array $b) {
+                return $a[1] > $b[1] ? -1 : 1;


this never returns 0, strange, why that?

Changed in favour of \SplPriorityQueue now 😉

linaori · 2016-08-31T10:22:25Z

You can still use the trait as inspiration and maybe play around with the existing function.

dmaicher · 2016-08-31T17:05:15Z

I changed it to use \SplPriorityQueue now 😉

If all agree this feature is useful as is I could prepare a PR for the docs?

fabpot · 2016-08-31T19:11:34Z

Relying on the order is it really something we want to support? Do we have enough real-world use cases? Nobody ever had this issue in the last 5 years, so I'm wondering if we really need to support this.

HeahDude · 2016-08-31T22:53:42Z

Form type extensions are meant to decouple specific configuration, thus may add options to resolve. So on one hand I agree, this is maybe covering edge cases where one need to handle that kind of option globally.
But on the other hand, the logical solution is to use an extension and in such cases having a full control of the order has a significant importance, otherwise it's just unpredictable.

Actually resolving the form configuration is why we need this composite pattern, and the process works well because it is mainly based on inheritance which do keep an order, so if we can support it for extensions too, I guess we should.

Good things:

DX: user is in full control (workaround from @javiereguiluz is good but not enough, and neither is mine, the option may be undefined and throw an exception or implies to define it first which is definitely not user friendly),
Perf: it is handled in the compiler pass so no runtime overhead,
Design: this is consistent with the composite pattern: options, form and view are resolved from the higher parent to lowest priority extension,

Bad things:

This is not a bug fix, so master only :troll face:

Anyway thank you @dmaicher for proposing this feature.

dmaicher · 2016-09-01T07:49:11Z

I would also agree with @HeahDude. I don't see any disadvantages of having this really small (+10, -3 LOC excluding tests) feature. Sure maybe not so many people will use it but currently the behaviour is a bit unpredictable for the order of form type extensions which is quite bad imho 😢

HeahDude · 2016-09-03T14:13:37Z

@dmaicher After re-reading this, I still think you should use the trait and change only this line, you can then keep the same logic as before to handle the indexation by extended type but you'll create the priority job only once which will also use only one instance one SplPriorityQueue since anyway by default no extension will have a priority set (keeping the same old unpredictable behavior ;).

I think that is is a good thing too because in the future if there is some issue about extensions overriding each other we will be able to solve it thanks to this feature.

dmaicher · 2016-09-04T08:01:25Z

@HeahDude

with this

foreach ($this->findAndSortTaggedServices('form.type_extension', $container) as $reference) {
    ...
}

I just have a sorted list of service Reference objects. How do I get the extended_type info then from the tags?

But I think you are right about sorting all extensions globally though 😊 Might make more sense.

ogizanagi · 2016-09-04T09:44:21Z

@dmaicher : The Reference instance converted to string gives you the service id. Just get the definition back from it, and access the tags :)

HeahDude · 2016-09-04T10:20:27Z

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

            $serviceDefinition = $container->getDefinition($serviceId);
            if (!$serviceDefinition->isPublic()) {
                throw new \InvalidArgumentException(sprintf('The service "%s" must be public as form type extensions are lazy-loaded.', $serviceId));
            }

+            $tag = $serviceDefinition->getTag('form.type_extension');
+


this empty line is not needed :)

HeahDude · 2016-09-04T10:22:33Z

LGTM Thanks @dmaicher :)

So what the conclusion here? ping @symfony/deciders

@dmaicher If this is accepted you'll have to update the framework bundle changelog file as well.

nicolas-grekas · 2016-09-06T09:31:47Z

@dmaicher can you please update the CHANGELOG file of the component?

dmaicher · 2016-09-08T16:10:57Z

@nicolas-grekas done 😉 Let me know if I need to change anything else

HeahDude · 2016-09-08T20:23:19Z

@dmaicher Can you please update the description too (tests are passing now :) and submit a PR in the docs? Thanks again!

Doc PR for symfony/symfony#19790

dmaicher · 2016-09-09T16:49:19Z

@HeahDude done 😉

HeahDude · 2016-09-09T16:52:32Z

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

+     * @dataProvider addTaggedTypeExtensionsDataProvider
+     *
+     * @param array $extensions
+     * @param array $expectedRegisteredExtensions


this phpdoc for params is needless

Ok removing it 👍

HeahDude · 2016-09-09T16:53:35Z

👍

Status: Reviewed

fabpot · 2016-09-14T21:07:43Z

Thank you @dmaicher.

…pe extension tags (dmaicher) This PR was merged into the 3.2-dev branch. Discussion ---------- [FrameworkBundle] add support for prioritizing form type extension tags | Q | A | ------------- | --- | Branch? | "master" | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19735 | License | MIT | Doc PR | symfony/symfony-docs#6958 This PR proposes to add support for `priority` on `form.type_extension` dependecyinjection tags to enable sorting/prioritizing form type extensions. Issue was mentioned here: #19735 Commits ------- a3db5f0 [FrameworkBundle] add support for prioritizing form type extension tags

…e extension tags (dmaicher) This PR was squashed before being merged into the master branch (closes #6958). Discussion ---------- [FrameworkBundle] add support for prioritizing form type extension tags Doc PR for symfony/symfony#19790 Commits ------- 0e0bdc3 [FrameworkBundle] add support for prioritizing form type extension tags

chrisguitarguy · 2017-01-04T21:33:00Z

A bit late to the party here, but I absolutely think this was a BC break. See dunglas/DunglasAngularCsrfBundle#39

The entire bundle's csrf protection form extension is broken because it now loads before the core csrf extension -- it disables csrf on a form in favor of its own system only to have it re-enabled by the core extension which now loads later in 3.2.

dmaicher · 2017-01-04T21:39:55Z

@chrisguitarguy this should have been fixed with #20995

chrisguitarguy · 2017-01-04T22:43:22Z

Great! Will have to downgrade to Symfony 3.1 until the next bugfix release of 3.2, though.

carsonbot added Status: Needs Review Feature labels Aug 30, 2016

HeahDude reviewed Aug 30, 2016
View reviewed changes

dmaicher force-pushed the form-extension-priority branch 3 times, most recently from 79d2af6 to 7d7c957 Compare August 30, 2016 20:02

dmaicher changed the title ~~add support for prioritizing form type extension tags~~ [Form] add support for prioritizing form type extension tags Aug 30, 2016

dmaicher changed the title ~~[Form] add support for prioritizing form type extension tags~~ [FrameworkBundle] add support for prioritizing form type extension tags Aug 30, 2016

dmaicher force-pushed the form-extension-priority branch from 7d7c957 to 3d0bda3 Compare August 30, 2016 20:32

nicolas-grekas reviewed Aug 31, 2016
View reviewed changes

dmaicher force-pushed the form-extension-priority branch 2 times, most recently from c982f84 to f9f8b57 Compare August 31, 2016 17:03

dmaicher force-pushed the form-extension-priority branch 2 times, most recently from bb0e184 to 7570bbb Compare August 31, 2016 17:08

dmaicher force-pushed the form-extension-priority branch from 7570bbb to ab302dc Compare September 4, 2016 10:09

dmaicher force-pushed the form-extension-priority branch from ab302dc to e3aa701 Compare September 4, 2016 10:17

HeahDude reviewed Sep 4, 2016
View reviewed changes

dmaicher force-pushed the form-extension-priority branch from e3aa701 to aac8a3f Compare September 4, 2016 10:21

dmaicher force-pushed the form-extension-priority branch from aac8a3f to ce051af Compare September 8, 2016 16:09

dmaicher added a commit to dmaicher/symfony-docs that referenced this pull request Sep 9, 2016

[FrameworkBundle] add support for prioritizing form type extension tags

9b195f9

Doc PR for symfony/symfony#19790

dmaicher mentioned this pull request Sep 9, 2016

[FrameworkBundle] add support for prioritizing form type extension tags symfony/symfony-docs#6958

Closed

HeahDude reviewed Sep 9, 2016
View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 9, 2016

[FrameworkBundle] add support for prioritizing form type extension tags

a3db5f0

dmaicher force-pushed the form-extension-priority branch from ce051af to a3db5f0 Compare September 9, 2016 17:24

fabpot merged commit a3db5f0 into symfony:master Sep 14, 2016

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

daFish mentioned this pull request Nov 7, 2016

Change priority of DisableCSRFExtension to make it compatible with Symfony 3.2 FriendsOfSymfony/FOSRestBundle#1602

Closed

HeahDude mentioned this pull request Dec 19, 2016

[FrameworkBundle] Fixed form type extensions priority ordering #20993

Closed

dmaicher mentioned this pull request Dec 20, 2016

[DependencyInjection] Fix the priority order of compiler pass trait #20995

Merged

dmaicher deleted the form-extension-priority branch December 20, 2016 18:08

chrisguitarguy mentioned this pull request Jan 4, 2017

Form CSRF Token Disiabling No Longer Works in Symfony 3.2 dunglas/DunglasAngularCsrfBundle#39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle] add support for prioritizing form type extension tags #19790

[FrameworkBundle] add support for prioritizing form type extension tags #19790

dmaicher commented Aug 30, 2016 •

edited

Loading

HeahDude Aug 30, 2016 •

edited

Loading

dmaicher Aug 30, 2016

dmaicher commented Aug 30, 2016

ogizanagi commented Aug 30, 2016 •

edited

Loading

dmaicher commented Aug 31, 2016

HeahDude commented Aug 31, 2016 •

edited

Loading

dmaicher commented Aug 31, 2016

HeahDude commented Aug 31, 2016

dmaicher commented Aug 31, 2016

nicolas-grekas Aug 31, 2016

dmaicher Aug 31, 2016

linaori commented Aug 31, 2016 •

edited

Loading

dmaicher commented Aug 31, 2016

fabpot commented Aug 31, 2016

HeahDude commented Aug 31, 2016

dmaicher commented Sep 1, 2016

HeahDude commented Sep 3, 2016 •

edited

Loading

dmaicher commented Sep 4, 2016

ogizanagi commented Sep 4, 2016

HeahDude Sep 4, 2016

dmaicher Sep 4, 2016

HeahDude commented Sep 4, 2016

nicolas-grekas commented Sep 6, 2016

dmaicher commented Sep 8, 2016

HeahDude commented Sep 8, 2016

dmaicher commented Sep 9, 2016

HeahDude Sep 9, 2016

dmaicher Sep 9, 2016

HeahDude commented Sep 9, 2016

fabpot commented Sep 14, 2016

chrisguitarguy commented Jan 4, 2017 •

edited

Loading

dmaicher commented Jan 4, 2017

chrisguitarguy commented Jan 4, 2017

[FrameworkBundle] add support for prioritizing form type extension tags #19790

[FrameworkBundle] add support for prioritizing form type extension tags #19790

Conversation

dmaicher commented Aug 30, 2016 • edited Loading

HeahDude Aug 30, 2016 • edited Loading

Choose a reason for hiding this comment

dmaicher Aug 30, 2016

Choose a reason for hiding this comment

dmaicher commented Aug 30, 2016

ogizanagi commented Aug 30, 2016 • edited Loading

dmaicher commented Aug 31, 2016

HeahDude commented Aug 31, 2016 • edited Loading

dmaicher commented Aug 31, 2016

HeahDude commented Aug 31, 2016

dmaicher commented Aug 31, 2016

nicolas-grekas Aug 31, 2016

Choose a reason for hiding this comment

dmaicher Aug 31, 2016

Choose a reason for hiding this comment

linaori commented Aug 31, 2016 • edited Loading

dmaicher commented Aug 31, 2016

fabpot commented Aug 31, 2016

HeahDude commented Aug 31, 2016

dmaicher commented Sep 1, 2016

HeahDude commented Sep 3, 2016 • edited Loading

dmaicher commented Sep 4, 2016

ogizanagi commented Sep 4, 2016

HeahDude Sep 4, 2016

Choose a reason for hiding this comment

dmaicher Sep 4, 2016

Choose a reason for hiding this comment

HeahDude commented Sep 4, 2016

nicolas-grekas commented Sep 6, 2016

dmaicher commented Sep 8, 2016

HeahDude commented Sep 8, 2016

dmaicher commented Sep 9, 2016

HeahDude Sep 9, 2016

Choose a reason for hiding this comment

dmaicher Sep 9, 2016

Choose a reason for hiding this comment

HeahDude commented Sep 9, 2016

fabpot commented Sep 14, 2016

chrisguitarguy commented Jan 4, 2017 • edited Loading

dmaicher commented Jan 4, 2017

chrisguitarguy commented Jan 4, 2017

dmaicher commented Aug 30, 2016 •

edited

Loading

HeahDude Aug 30, 2016 •

edited

Loading

ogizanagi commented Aug 30, 2016 •

edited

Loading

HeahDude commented Aug 31, 2016 •

edited

Loading

linaori commented Aug 31, 2016 •

edited

Loading

HeahDude commented Sep 3, 2016 •

edited

Loading

chrisguitarguy commented Jan 4, 2017 •

edited

Loading