[WIP][Form] Implement field order #6111
Conversation
|
Just as a reminder, it needs to be defined what happens when option A has option B in |
|
@Tobion Thanks for your feedback! It can be easily archive but I can't find an appropriate exception message... Any idea? |
|
Just say that the form ordering cannot be resolved because they have conflicting options. Also mention which forms are concerned and which config excatly is impossible. |
| } | ||
| } | ||
|
|
||
| if (null !== $before) { |
There was a problem hiding this comment.
The FormConfigInterface::getBefore says, it returns a string only. So null is not expected here.
There was a problem hiding this comment.
@Tobion I have updated the PHPDoc but it seems there is other PHPDoc which have the same issue.
|
Does the invalid field order detection work with bigger cycles? A is before B, B is before C and C is before A? |
|
@laszlokorte Currently nope... The issue is the NB: I have updated the tests to show you better use cases. |
| @@ -242,6 +242,24 @@ public function setData($data); | |||
| public function setDataLocked($locked); | |||
|
|
|||
| /** | |||
| * Sets the field name which is just before the field. | |||
There was a problem hiding this comment.
Is this correct? Doesn't "before" receive the name of the field just after this field? (i.e., this field (A) is before the other field (B))
There was a problem hiding this comment.
I agree too :) I will fix it tonight
|
I think the algorithm here needs to be smarter. Currently it relies heavily on |
|
I'm sure there is room for improvements, but unfortunately SplDoublyLinkList does not implement before and after insertion as it should. I implemented the same kind of logic here this week, closest I could get to a decent solution was using those same array functions. Unfortunately. While questioning in internals when this could be fixed, i got pointed to this userland solution: https://github.com/morrisonlevi/PHP-Datastructures/blob/master/src/Spl/LinkedList.php, maybe worth taking a look at how @morrisonlevi solved it? |
|
Watch out, the code @rdohms linked doesn't contain any license, so it's not really "open source"... |
|
@mvrhov Seems like there "is" something about license: https://github.com/morrisonlevi/PHP-Datastructures/blob/master/composer.json#L7 |
|
Well the norm is to put at least a part or the information of where to find it into each file + license file with the license in the root of the project. It never crossed my mind that the author hide it into a composer.json file |
|
@mvrhov i myself also end up skipping the docblock in every file, especially in my small libraries, its a hassle to get done. |
|
On Fri, Dec 7, 2012 at 7:18 AM, Miha Vrhovnik notifications@github.comwrote:
As the author of the library in question, I can affirmatively say that it |
It's honestly not a difficult structure to implement. I would change |
|
Ping, any update on the progress? |
|
@rdohms Unfortunately nope... I need to change my approach to avoid using expensive |
|
@bschussek i have looked at this from a few different angles, most solutions i see end up with the use of the same array_ functions above. The only other solution i see is to use a true doubly linked list implementation, that would force us to introduce a new object to support it since the native SPL structure does not support before/after behavior. Do you think introducing that list would be the best way out? or do you see another one? |
|
On Tue, Jan 8, 2013 at 3:03 PM, Rafael Dohms notifications@github.comwrote:
If the answers are yes then no, what is holding you back? |
|
@rdohms @bschussek I just push an other proposal which avoid usage of |
|
It does seem a bit faster from running the tests. Personally I would move all the ordering stuff out of that method, either to a separate object or at least a couple protected methods since it has multiple levels of indentation, nested ifs and foreaches, etc. |
| if (null !== $before) { | ||
|
|
||
| // If there is a before option, we cache it & assume the current child is the after option of | ||
| // the before child. |
There was a problem hiding this comment.
This assumption is wrong.
A:
B: before A and after C
C: before A
The resulting order should be C B A
There was a problem hiding this comment.
@stof I'm not sure what you're pointing here. The documentation or the implementation? Your use case seems handled (see this test).
Anyway you're right, the internal documentation of the method is bad & need to be rewritten. But before that, I would like to get feedback about the implementation as the previous one relies heavily on array_search, array_slice and array_merge which are expensive operations as pointed by @bschussek
|
Any news guys? Seems like everyone thinks there is a less expensive way of doing it but no one actually finds it...maybe its time to go with the current implementation and improve the internals in future releases? |
|
I don't know what's happening lately, but everything in Symfony in the last months are taking a long time until happen. Currently, there are more than 600 issues and 34 pull requests opened. IMHO, Symfony's team is not managing to keep up with their community/popularity growth. It's not a criticism, but I think it's something to be evaluated once that demand of new features, bug fixes, etc will grow in the same proportion of community growth. |
|
I will come back on my PR to introduce |
|
If we agree on the interface to the user, we can always swap out the internals if we find a better solution. |
|
@bschussek I have rebased the PR & so, I have added the ability to define the button position. At the same time, I have moved the position validation in |
|
How much longer are we going to drag this? This will not even be in 2.3 anymore i guess... we have people willing to help and get it done, but its just hitting a wall. @bschussek can we push this ahead? |
|
👎 for this. It is something for the rendering not the component it self. |
|
+1 for this ! it would really help to manage dynamic forms if you want to manage this in the rendering part of your application nothing prevents you to do that and not use this functionnality if you want the rendering to be able to manage this automatically then you have to make the component aware of this. Rendering of the form is still a (small) part of the component (see FormRenderer or AbstractRendererEngine) |
|
@henrikbjorn there is a form_widget, so there must be a way to add some control. I have forms that change constantly, being able to only deal with them in an object is a major productivity bonus for me. |
|
Seems we are still standing still. /cc @bschussek @fabpot |
|
@rdohms the form_widget is only a helper to select the correct template to render (or block in twig). I cant see how that makes the field order signifient in the type definitions. I can see it "maybe" for prototyping but anywhere else it would be overhead to sort and so on. |
|
I'm personally against this change for a simple reason: the That said, this is only my opinion. What do you think @bschussek? |
|
I somewhat agree with @fabpot. If you use these helpers you probably have a simple form where you dont need this anyway. If you have something more complex, you should not use these helpers. And if you need such feature for example a CMS like Drupal where you can add and configure the forms using a UI, the handling of the order of form fields should be part of that system and not symfony. |
|
@Tobion @fabpot Actually there is another use case. Dynamic forms, especially the ones that depend on fields that are added on the fly based on the Data, as @bschussek himself has described in his talks. If my Form object is already reacting with the data to include or change fields, its overhead for me to have to also do that in the view. Personally if you view follows a simple form layout, i see no need to render each field individually, hence our strong use of the form widget. |
|
I agree with @rdohms. Dynamic forms requires a lot of |
|
As everybody seems not really agree on this feature, I'm waiting @bschussek feedback before restart my work on this PR. |
|
To give my opinion, I really think this feature should be added to the core (especially for the case rdohms pointed out). Having to define all fields one by one in a template just because we can't order it on the form side it really a pain (espacally with event listener). |
|
I think I remember seeing a bundle that provided a weight type extension and a wrapper for |
|
@kbond A link would be awesome! Maybe it can do the trick need to read the code to see how it works :) Anyway, If someone have a trick to implement this feature outside the form component, it will be very appreciated. Before making this PR, I have try to make this logic outside the form component through the form extension but it seems a form extension can only update fields & not the global form behavior (orders). |
|
As fabpot mentioned earlier, the Anyway for those who are interested by this feature, I'm currently implementing a bundle which will provide it: https://github.com/egeloen/IvoryOrderedFormBundle It is in WIP but I hope I will be able to finish in coming weeks. |
|
"designed for prototyping" is a poor excuse for this, it also ignores the usage in a large part of the projects out there. There was never a rebuttal or further discussion of the matter, i would like to see this get another look over before we abandon it. |
|
A native solution in symfony with weight as @kbond mentioned will be appreciate... |
|
Thanks @egeloen. Fact remains that if a quick and native weight solution can be considered by @bschussek, it can be appreciate. |
|
If everybody agrees on the weight solution, I can reopen my PR & make it working pretty easily. |
|
not in the master ? need ordered form by position natively :) |
|
A bundle is the right place for this. If it is in core it would add additional overhead, even if it was optional. Having a bundle makes it easy to plug it in for your application. There is no need to reopen this. @egeloen Good job on the bundle! |
This PR was squashed before being merged into the 5.3-dev branch. Discussion ---------- [Form] Add support for sorting fields | 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](https://symfony.com/doc/current/form/form_customization.html) 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](https://symfony.com/doc/current/form/dynamic_form_modification.html). 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:  ```php 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: ```html <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: ```php // ... $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! Commits ------- 62650bb [Form] Add support for sorting fields
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #5827 #4254 #3452
Todo: -
License of the code: MIT
Documentation PR: ~
This PR adds the ability to set fields order via two new options:
before&after.These options represent respectively the field name just before the field & the field name just after the field.
If none of the two options is given, the current behavior is maintained (fields order is determined by the way you added your fields on your builder). If you partially order your fields, the default behavior will be overridden by your explicit order.