[Form] Add support for sorting fields #40690

yceruto · 2021-04-03T04:27:34Z

Q	A
Branch?	5.x
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #5827
License	MIT
Doc PR	TODO

This PR adds a new priority option used later to sort all fields in order of importance instead of simply numerical order, i.e. fields with higher priority will be rendered first, fields with lower priority will be rendered last. The default priority would be "0" for all fields. Fields with equal priority will keep the original order (stable sorting).

History of previous proposals and discussions: #3452, #4254, #5827, #6111, #11241

This kind of feature has been abandoned in the past because complex proposals, and somehow rejected because ideally we should do the sorting at the view level (customizing form themes or layout templates) and it's true for most cases (the simple ones I think) but the fact is that it's often quite complex to accomplish that way, mainly for dynamic forms.

Let's focus the following analysis on explaining why and when the priority option could save us a lot of time, getting rid of cumbersome workarounds with templates to change the rendering order only.

A common example could be the building of a multi-steps form with a convenient type hierarchy and including fields dynamically based on the data. Let's take this sample:

class WorkflowType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        // get current enabled transitions from the workflow definition...

        foreach ($enabledTransitions as $transitionName) {
            $builder->add($transitionName, SubmitType::class, ['attr' => ['value' => $transitionName]]);
        }
    }
}

class PersonRegistrationType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('firstName')
            ->add('lastName');

        $builder->addEventListener(FormEvents::PRE_SET_DATA, function (FormEvent $event) {
            $person = $event->getData();

            if ($person->isLegalType()) {
                $event->getForm()->add('company');
            }
        });
    }

    public function getParent()
    {
        return WorkflowType::class;
    }
}

These classes model the required form. However, according to the form building process and taken into account that the underlaying person data was set with "legal" type (from the previous step), this will be the rendering result:

<button type="submit" name="form[register]" value="register">Register</button> {# wrong place #}
<input name="form[first_name]">
<input name="form[last_name]">
<input name="form[company]">  {# wrong place #}

Now, taking the view customization path to fix the order, you likely will face some problems regarding to:

How do I render the dynamic submit buttons at the bottom?
How can the specific form fields be rendered in the correct order if they vary from step to step? (being it a generic template)
if you solve the previous questions, you will also need to check if the company field is defined or not before rendering it in the right order.

There could be more and different problems depending on the context and you can find workarounds using the block prefixes system to customize each step block theme, but to me it's enough to think about other alternatives.

On the other side, using the priority option, the solution is quite easy. Setting the right priority value will guarantee the right rendering order:

// ...
$builder->add($transitionName, SubmitType::class, ['priority' => -1, // ...]);

// ...
$event->getForm()->add('company', null, ['priority' => 1]);

That's ! There is no need to customize templates at all when you only need to change the fields order.

This way, these fields are being pre-defined in the order they will be rendered at view level. This sorting process will take place on $form->createView() method, just after buildView() and before finishView() calls.

Could we reconsider the usefulness of this feature? please.

Cheers!

yceruto · 2021-04-03T04:34:58Z

src/Symfony/Component/Form/Form.php

+        }
+
+        uksort($children, static function ($a, $b) use ($c): int {
+            return $c[$a]['w'] === $c[$b]['w'] ? $c[$a]['n'] <=> $c[$b]['n'] : $c[$a]['w'] <=> $c[$b]['w'];


For PHP < 8.0 *sort() functions don't apply a stable sorting to keep equals elements in the original order https://3v4l.org/NQtul. That's why I needed to take care about it here.

this can be written as return [$c[$a]['w'], $c[$a]['n']] <=> [$c[$b]['w'], $c[$b]['n']]; Arrays are sorted by comparing their values one by one

Nicer 😍 ! - Updated.

Nyholm · 2021-04-03T08:37:52Z

I like that this diff is only (+53 -2). I agree that this could be a simple way to control the rendering of the form. It will allow me to render more forms as {{ form(form) }}.

On most other places we use the word priority where a high priority means that it is handled first. Does it make sense to reuse that terminology?

OskarStark · 2021-04-03T09:15:40Z

Nice proposal Yonel 😃🎉

I thought about the priority too but the explanation with "weight" and "sink down" is super power. I find myself asking the question quite often: negative or positive priority, but if I think of a weight it makes things much easier and more clear.

I like the wording, plus it's a setting for something "visual", which is not the case for priority 🙂

yceruto · 2021-04-03T15:32:45Z

Wow! thanks for your fast review :)

I find "the field weight" pretty clear, but I'm open to change it if we all agree with another name.

On most other places we use the word priority where a high priority means that it is handled first. Does it make sense to reuse that terminology?

Yes, "priority" could be another name and it should be interpreted as "the field rendering priority". Let's way for other opinions if it's really clearer than "weight" or for terminology consistency.

nicolas-grekas · 2021-04-13T08:04:57Z

We use "priority" more commonly, I think it would be more consistent to use it.

yceruto · 2021-04-13T17:41:39Z

PR updated with priority option instead (for consistency).

src/Symfony/Component/Form/CHANGELOG.md

src/Symfony/Component/Form/Form.php

stof · 2021-04-13T17:45:59Z

src/Symfony/Component/Form/Form.php

@@ -1044,11 +1044,29 @@ public function createView(FormView $parent = null)
            $view->children[$name] = $child->createView($view);
        }

+        $this->sort($view->children);


should this be done before or after finishView ?

From my point of view, this should be done before finishView().

This is the final step of the view creation, thus I don't expect any view change after that.

In case you want to change the current order to anything else (custom conditions) you could do it in finishView().

Indeed, if you have a type extension doing this currently (sorting fields on finishView by a custom criteria), we will break that chance if we sort after finishView.

makes sense

src/Symfony/Component/Form/Form.php

yceruto · 2021-04-13T18:12:13Z

Comments addressed, thanks!

apfelbox · 2021-04-14T09:04:51Z

This is a really great proposal and a non-invasive implementation. Great work! 🎉

I had this functionality in an external bundle in use for several years now and the most common use case for me was a before field X, after field Y (and maybe first and last).
This covered basically 99% of use cases and shifted the work to the sorter instead of the developer to need to manually come up with proper priority values.

Is that a feature that is possible to add / planned?

Nyholm

<3

Nyholm · 2021-04-14T09:08:30Z

Is that a feature that is possible to add / planned?

That feature is not currently in the pipe and it will not be covered in this PR.
I do see some value in it. I suggest to create a new issue for that.

keichinger · 2021-04-14T09:09:22Z

The bundle @apfelbox is talking about is https://github.com/becklyn/OrderedFormBundle. Yes, I'm biased in this regards since this is our company's bundle, but I still vastly prefer a more explicit and relative API (the one from our bundle) over a priority number option.

We wouldn't mind if our bundle (or rather the functionality of it) would become part of Symfony's core :)

Nyholm · 2021-04-14T09:14:30Z

Thank you. @keichinger, that is a great bundle. Let's consider this PR baby steps and if we want to see the bundle's feature in Symfony, then I suggest to open a new issue. =)

This PR was merged into the 5.3-dev branch. Discussion ---------- [Form] Documenting new priority option PR: symfony/symfony#40690 Fix: symfony#15239 Commits ------- 454087d Documenting form field priority option

curry684 · 2022-11-22T15:17:19Z

@yceruto why is the sorting done in createView instead of for example handleRequest which seems more logical to me?

I discovered the priority feature today when I ran into a precedence issue - I have a form field whose setter depends on the content of one of the other fields. This crashed because in the form type I was defining them in the wrong way, and reordering the form type actually fixes the problem, but that makes for fragile code. So I was happy to discover that I can set priority, only to then discover it doesn't do anything at all as I'm using explicit per-field rendering in Twig, and the priority is only used as drawing order.

Referring back to this comment by @Nyholm:

On most other places we use the word priority where a high priority means that it is handled first. Does it make sense to reuse that terminology?

That makes the feature currently broken, because indeed I was expecting a high priority to mean it would be HANDLED first, as everywhere else in Symfony. Instead it is only rendered first, in some situations, and completely ignored while handling. Currently it functions as drawOrder or position and the priority name is misleading and highly confusing. #40858 by @keichinger has a point in requesting the rename.

Further confusing is that the draw order and handle priority would in my use case not even be identical, the dependent element is drawn first for UX reasons. So I think the whole concept of priority needs to be separated from the default drawing order.

stof · 2022-11-22T16:35:46Z

Well, the feature request was always about the rendering, not about the handling. That's why it was only implemented as part of the form view creation.

yceruto requested a review from xabbuh as a code owner April 3, 2021 04:27

carsonbot added Status: Needs Review DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Form labels Apr 3, 2021

carsonbot changed the title ~~[Form][DX] Add support for sorting fields (weighted sort)~~ [Form] Add support for sorting fields (weighted sort) Apr 3, 2021

yceruto added this to the 5.x milestone Apr 3, 2021

yceruto force-pushed the form_field_weight_option branch 3 times, most recently from 025aba9 to ce8398f Compare April 3, 2021 04:30

yceruto commented Apr 3, 2021

View reviewed changes

yceruto changed the title ~~[Form] Add support for sorting fields (weighted sort)~~ [Form][DX] Add support for sorting fields (weighted sort) Apr 3, 2021

yceruto force-pushed the form_field_weight_option branch from ce8398f to be75330 Compare April 13, 2021 17:35

yceruto changed the title ~~[Form][DX] Add support for sorting fields (weighted sort)~~ [Form][DX] Add support for sorting fields Apr 13, 2021

yceruto force-pushed the form_field_weight_option branch from be75330 to ee71072 Compare April 13, 2021 17:40

nicolas-grekas approved these changes Apr 13, 2021

View reviewed changes

src/Symfony/Component/Form/CHANGELOG.md Outdated Show resolved Hide resolved

src/Symfony/Component/Form/Form.php Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 13, 2021

carsonbot changed the title ~~[Form][DX] Add support for sorting fields~~ [Form] Add support for sorting fields Apr 13, 2021

yceruto force-pushed the form_field_weight_option branch from ee71072 to 40e1766 Compare April 13, 2021 17:49

stof reviewed Apr 13, 2021

View reviewed changes

yceruto force-pushed the form_field_weight_option branch from 40e1766 to 3c61c6e Compare April 13, 2021 18:10

yceruto added the Ready label Apr 13, 2021

Nyholm approved these changes Apr 14, 2021

View reviewed changes

[Form] Add support for sorting fields

62650bb

Nyholm force-pushed the form_field_weight_option branch from 8d4ea0a to 62650bb Compare April 14, 2021 09:13

Nyholm merged commit b2f269e into symfony:5.x Apr 14, 2021

Nyholm mentioned this pull request Apr 14, 2021

[Form] Add support for sorting fields symfony/symfony-docs#15239

Closed

yceruto deleted the form_field_weight_option branch April 14, 2021 13:40

fabpot mentioned this pull request Apr 18, 2021

Release v5.3.0-BETA1 #40853

Merged

keichinger mentioned this pull request Apr 18, 2021

[Form] Rename priority to position and allow relative sorting of form elements #40858

Closed

yceruto mentioned this pull request May 10, 2021

[Form] Documenting new priority option symfony/symfony-docs#15324

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] Add support for sorting fields #40690

[Form] Add support for sorting fields #40690

yceruto commented Apr 3, 2021 •

edited

Loading

yceruto Apr 3, 2021 •

edited

Loading

stof Apr 13, 2021

yceruto Apr 13, 2021

Nyholm commented Apr 3, 2021 •

edited by OskarStark

Loading

OskarStark commented Apr 3, 2021

yceruto commented Apr 3, 2021

nicolas-grekas commented Apr 13, 2021

yceruto commented Apr 13, 2021

stof Apr 13, 2021

yceruto Apr 13, 2021

yceruto Apr 13, 2021

stof Apr 14, 2021

yceruto commented Apr 13, 2021

apfelbox commented Apr 14, 2021

Nyholm left a comment

Nyholm commented Apr 14, 2021

keichinger commented Apr 14, 2021 •

edited

Loading

Nyholm commented Apr 14, 2021

curry684 commented Nov 22, 2022

stof commented Nov 22, 2022

[Form] Add support for sorting fields #40690

[Form] Add support for sorting fields #40690

Conversation

yceruto commented Apr 3, 2021 • edited Loading

yceruto Apr 3, 2021 • edited Loading

Choose a reason for hiding this comment

stof Apr 13, 2021

Choose a reason for hiding this comment

yceruto Apr 13, 2021

Choose a reason for hiding this comment

Nyholm commented Apr 3, 2021 • edited by OskarStark Loading

OskarStark commented Apr 3, 2021

yceruto commented Apr 3, 2021

nicolas-grekas commented Apr 13, 2021

yceruto commented Apr 13, 2021

stof Apr 13, 2021

Choose a reason for hiding this comment

yceruto Apr 13, 2021

Choose a reason for hiding this comment

yceruto Apr 13, 2021

Choose a reason for hiding this comment

stof Apr 14, 2021

Choose a reason for hiding this comment

yceruto commented Apr 13, 2021

apfelbox commented Apr 14, 2021

Nyholm left a comment

Choose a reason for hiding this comment

Nyholm commented Apr 14, 2021

keichinger commented Apr 14, 2021 • edited Loading

Nyholm commented Apr 14, 2021

curry684 commented Nov 22, 2022

stof commented Nov 22, 2022

yceruto commented Apr 3, 2021 •

edited

Loading

yceruto Apr 3, 2021 •

edited

Loading

Nyholm commented Apr 3, 2021 •

edited by OskarStark

Loading

keichinger commented Apr 14, 2021 •

edited

Loading