Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP][Form] Implement field order #6111

Closed
wants to merge 4 commits into from

Conversation

@egeloen
Copy link
Contributor

commented Nov 24, 2012

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.

@Tobion

This comment has been minimized.

Copy link
Member

commented Nov 24, 2012

Just as a reminder, it needs to be defined what happens when option A has option B in before and B has A in before.
Probably raise an exception.

@egeloen

This comment has been minimized.

Copy link
Contributor Author

commented Nov 24, 2012

@Tobion Thanks for your feedback! It can be easily archive but I can't find an appropriate exception message... Any idea?

@Tobion

This comment has been minimized.

Copy link
Member

commented Nov 24, 2012

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.

@Tobion

View changes

src/Symfony/Component/Form/ResolvedFormType.php Outdated
}
}

if (null !== $before) {

This comment has been minimized.

Copy link
@Tobion

Tobion Nov 24, 2012

Member

The FormConfigInterface::getBefore says, it returns a string only. So null is not expected here.

This comment has been minimized.

Copy link
@egeloen

egeloen Nov 25, 2012

Author Contributor

@Tobion I have updated the PHPDoc but it seems there is other PHPDoc which have the same issue.

@laszlokorte

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2012

Does the invalid field order detection work with bigger cycles?

A is before B, B is before C and C is before A?

@egeloen

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2012

@laszlokorte Currently nope... The issue is the before & after options should follow a specific behavior explain by @bschussek here. It works a little bit like the form builder (it follows the way you added your fields on the builder). So, detecting circular before/after options is a little bit difficult. I will continue my investigations.

NB: I have updated the tests to show you better use cases.

@webmozart

View changes

src/Symfony/Component/Form/FormConfigBuilderInterface.php Outdated
@@ -242,6 +242,24 @@ public function setData($data);
public function setDataLocked($locked);

/**
* Sets the field name which is just before the field.

This comment has been minimized.

Copy link
@webmozart

webmozart Dec 7, 2012

Contributor

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

This comment has been minimized.

Copy link
@rdohms

rdohms Dec 7, 2012

Contributor

i agree with @bschussek

This comment has been minimized.

Copy link
@egeloen

egeloen Dec 7, 2012

Author Contributor

I agree too :) I will fix it tonight

@webmozart

View changes

src/Symfony/Component/Form/FormConfigBuilderInterface.php Outdated
public function setBefore($before);

/**
* Sets the field name which is just after the field.

This comment has been minimized.

Copy link
@webmozart

webmozart Dec 7, 2012

Contributor

Same here

@webmozart

View changes

src/Symfony/Component/Form/FormConfigInterface.php Outdated
@@ -217,4 +217,18 @@ public function hasOption($name);
* @return mixed The option value.
*/
public function getOption($name, $default = null);

/**
* Gets the field name which is just before the field.

This comment has been minimized.

Copy link
@webmozart

webmozart Dec 7, 2012

Contributor

Same here

@webmozart

View changes

src/Symfony/Component/Form/FormConfigInterface.php Outdated
public function getBefore();

/**
* Gets the field name which is just after the field.

This comment has been minimized.

Copy link
@webmozart

webmozart Dec 7, 2012

Contributor

Same here

@webmozart

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2012

I think the algorithm here needs to be smarter. Currently it relies heavily on array_search, array_slice and array_merge, which are expensive operations.

@rdohms

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2012

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?

@mvrhov

This comment has been minimized.

Copy link

commented Dec 7, 2012

Watch out, the code @rdohms linked doesn't contain any license, so it's not really "open source"...

@stloyd

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2012

@mvrhov

This comment has been minimized.

Copy link

commented Dec 7, 2012

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

@rdohms

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2012

@mvrhov i myself also end up skipping the docblock in every file, especially in my small libraries, its a hassle to get done.

@morrisonlevi

This comment has been minimized.

Copy link

commented Dec 7, 2012

On Fri, Dec 7, 2012 at 7:18 AM, Miha Vrhovnik notifications@github.comwrote:

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


Reply to this email directly or view it on GitHubhttps://github.com//pull/6111#issuecomment-11131294.

As the author of the library in question, I can affirmatively say that it
uses an MIT license. I do need to make it more clear. I should at least at
a separate license file instead of it being only in the composer.json
file.

@morrisonlevi

This comment has been minimized.

Copy link

commented Dec 7, 2012

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?

It's honestly not a difficult structure to implement. I would change
the SPL but I am beginning to believe that there are too many BC
concerns to properly fix the extension. So I created this new library.
Whether it will ever make it to PECL, I'm not sure, but during the
design phase it's much simpler to keep this as a user-land library.

@rdohms

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2013

Ping, any update on the progress?

@egeloen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2013

@rdohms Unfortunately nope... I need to change my approach to avoid using expensive array_* functions but I can't find something which works well & at the same time, is efficient.

@rdohms

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2013

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

@morrisonlevi

This comment has been minimized.

Copy link

commented Jan 8, 2013

On Tue, Jan 8, 2013 at 3:03 PM, Rafael Dohms notifications@github.comwrote:

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?

Implementing a DLL is really not that hard. The key things to ask yourself
are:

- Do you need the operations to be quick?
- Is the increased complexity of using a DLL a problem?

If the answers are yes then no, what is holding you back?

@egeloen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2013

@rdohms @bschussek I just push an other proposal which avoid usage of array_* functions & use max & asort instead. Your opinion?

@rdohms

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2013

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.

@stof

View changes

src/Symfony/Component/Form/ResolvedFormType.php Outdated
if (null !== $before) {

// If there is a before option, we cache it & assume the current child is the after option of
// the before child.

This comment has been minimized.

Copy link
@stof

stof Jan 9, 2013

Member

This assumption is wrong.

A:
B: before A and after C
C: before A

The resulting order should be C B A

This comment has been minimized.

Copy link
@egeloen

egeloen Jan 9, 2013

Author Contributor

@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

@rdohms

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2013

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?

@marcospassos

This comment has been minimized.

Copy link

commented Mar 30, 2013

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.

@egeloen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2013

I will come back on my PR to introduce position as proposed by @bschussek IMO, it is the most flexible & appropriate proposition. Anyway, I don't think I will be able to more optimize the current algo... Build a weight on each field according to the position option & then, sort all fields accordingly is IMO the better approach I can propose.

@rdohms

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2013

If we agree on the interface to the user, we can always swap out the internals if we find a better solution.

@webmozart

View changes

src/Symfony/Component/Form/ResolvedFormType.php Outdated
));
}

if ($positions[$item]['before'] !== null) {

This comment has been minimized.

Copy link
@webmozart

webmozart Apr 18, 2013

Contributor

This if clause is useless due to the isset() check before.

@egeloen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2013

@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 setPosition & remove useless code. What's your opinion about the PR?

@rdohms

This comment has been minimized.

Copy link
Contributor

commented May 30, 2013

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?

@henrikbjorn

This comment has been minimized.

Copy link
Contributor

commented May 30, 2013

👎 for this. It is something for the rendering not the component it self.

@tkleinhakisa

This comment has been minimized.

Copy link

commented May 30, 2013

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

@rdohms

This comment has been minimized.

Copy link
Contributor

commented May 30, 2013

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

@rdohms

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2013

Seems we are still standing still. /cc @bschussek @fabpot

@henrikbjorn

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2013

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

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 12, 2013

I'm personally against this change for a simple reason: the form() and form_widget() helpers were only created for prototyping... and I made it clear from the beginning (I was against their introduction in the first place). If we want to use these helpers to solve all possible issues that might come up, we are going to complexify the form system a lot (we went down this road in symfony1).

That said, this is only my opinion. What do you think @bschussek?

@Tobion

This comment has been minimized.

Copy link
Member

commented Jun 12, 2013

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.

@rdohms

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2013

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

@marcospassos

This comment has been minimized.

Copy link

commented Jun 12, 2013

I agree with @rdohms. Dynamic forms requires a lot of if and else rendering field by field. The possibility of controlling field's position improves a lot the productivity. A big 👍 for introducing this feature.

@egeloen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2013

As everybody seems not really agree on this feature, I'm waiting @bschussek feedback before restart my work on this PR.

@egeloen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2013

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

@kbond

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2013

I think I remember seeing a bundle that provided a weight type extension and a wrapper for FormView. I can't seem to find it now - would something like that work?

@egeloen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2013

@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).

@egeloen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2013

As fabpot mentioned earlier, the form_widget(form) has been introduced for prototyping... Then, I will close this issue for this reason.

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.

@egeloen egeloen closed this Aug 23, 2013

@rdohms

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2013

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

@webda2l

This comment has been minimized.

Copy link

commented Aug 26, 2013

A native solution in symfony with weight as @kbond mentioned will be appreciate...

@egeloen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2013

@rdohms @webda2l You can use this bundle, just need to register it & you can use position in your form.

@webda2l

This comment has been minimized.

Copy link

commented Aug 26, 2013

Thanks @egeloen. Fact remains that if a quick and native weight solution can be considered by @bschussek, it can be appreciate.

@egeloen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2013

If everybody agrees on the weight solution, I can reopen my PR & make it working pretty easily.

@ibasaw

This comment has been minimized.

Copy link

commented Dec 2, 2013

not in the master ? need ordered form by position natively :)

@henrikbjorn

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2013

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!

@egeloen egeloen deleted the egeloen:field-order branch Dec 2, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.