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

[Form] Keep preferred choices order in ChoiceType #30985

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@vudaltsov
Copy link
Contributor

vudaltsov commented Apr 7, 2019

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

Added a boolean keep_preferred_choices_order option.

#EUFOSSA

@vudaltsov vudaltsov force-pushed the vudaltsov:keep-preferred-choices-order branch 2 times, most recently from b1a9a9e to 1516862 Apr 7, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 7, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 7, 2019

Do we need an option for that? Why not make it the only and default behavior?

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

vudaltsov commented Apr 8, 2019

@nicolas-grekas , if that's okay from the BC prospective, I would do that.

@HeahDude

This comment has been minimized.

Copy link
Member

HeahDude commented Apr 8, 2019

We have to be sure that changing the order does not break BC in some way. But I would favor this behavior if possible indeed.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 8, 2019

That's only a "display" order right? and the order is "random", since we don't preserve it,?
If confirmed, I don't see how this would be a BC break/

@HeahDude

This comment has been minimized.

Copy link
Member

HeahDude commented Apr 8, 2019

The order is not random, it is based on the order of the original choices, ignoring the one set in the preferred choices array.

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

vudaltsov commented Apr 9, 2019

So yeah, the behavior will change, but in a better, more controllable way. I am sure that usually people specify preferred choices in the order they expect or don't care about the order at all.

I am going to do this as a default. In case we get any reasonable complaints, the BC layer would be easy. Otherwise one can just change the order of the preferred choices in the desired way and the issue is gone.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 9, 2019

Ok great. Note that adding an argument on a signature is a BC break. I'd suggest not making this optional and just adjust the behavior.

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Apr 9, 2019

Does this have to be configurable per form or would it be enough to pass an argument to the DefaultChoiceListFactory constructor?

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

vudaltsov commented Apr 9, 2019

@xabbuh , we currently discuss to make it the only behavior. So no opt-in and no opt-out. Just change it.

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Apr 9, 2019

I am sure there will be complaints that some JavaScript or test breaks when the order changes.

@vudaltsov vudaltsov force-pushed the vudaltsov:keep-preferred-choices-order branch from 1516862 to b15c028 Apr 12, 2019

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

vudaltsov commented Apr 12, 2019

@xabbuh , we'll see :)
We don't have any tests broken by the changed order. Neither do other developers, I guess.

@HeahDude , @nicolas-grekas , ready for review!

@vudaltsov vudaltsov marked this pull request as ready for review Apr 12, 2019

@vudaltsov vudaltsov force-pushed the vudaltsov:keep-preferred-choices-order branch from b15c028 to 88753ee Apr 12, 2019

$otherViews
);
}
uksort($preferredViews, static function ($a, $b) use ($preferredViewsOrder): int {
return isset($preferredViewsOrder[$a], $preferredViewsOrder[$b])

This comment has been minimized.

Copy link
@vudaltsov

vudaltsov Apr 12, 2019

Author Contributor

This check is needed when we have groups in preferred views.
It happens in tests and might happen in user land.

@vudaltsov vudaltsov force-pushed the vudaltsov:keep-preferred-choices-order branch from 88753ee to 340a2fb Apr 12, 2019

@nicolas-grekas nicolas-grekas changed the title [Form] Allow to keep preferred choices order in ChoiceType [Form] Keep preferred choices order in ChoiceType Apr 15, 2019

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