NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Form] Add FilterChoiceLoader + choice_filter option#28624

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

ro0NL wants to merge1 commit intosymfony:masterfromro0NL:choice-filter

Closed

[Form] Add FilterChoiceLoader + choice_filter option#28624

ro0NL wants to merge1 commit intosymfony:masterfromro0NL:choice-filter

Conversation

Copy link

Contributor

ro0NL commentedSep 27, 2018•
edited
Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#11847,#14072,#28607,#28542
License	MIT
Doc PR	symfony/symfony-docs#10413

I did it :) i understand form choice lists.. took me only 2 days 😂

But here it is, achoice_filter option for theChoiceType. Allowing to filter the list of choices independent from any choice loader / array.

This is no magic bullet. To truly optimize one should always prefer a filtered choice list (e.g. passing a query builder to the doctrine choice loader). But for built-in / pre-defined choice loaders, e.g. Intl, this is convenient.

Thanks to@sstok and@yceruto also.

agree on approach (per[Form] filter entity choicelist after hydration #14072 (comment))
consider delegating array choices toArrayChoiceList for sanity :)
update filtering array choices (see todo in code)
fix callable argument consistency (choice only)
add form integration/rendering tests

carsonbot added Status: Needs Review Form Feature labels

Sep 27, 2018

ro0NL mentioned this pull request

Sep 27, 2018

[Form][DX] Add choice_filter option to ChoiceType#28607

Closed

stof reviewed

Sep 27, 2018

View reviewed changes

src/Symfony/Component/Form/ChoiceList/Loader/FilterChoiceLoader.php OutdatedShow resolvedHide resolved

Copy link

Member

stof commentedSep 27, 2018

* fix callable argument consistency (`choice, key, value` ideally)

Looking at the places where you use the callable, you cannot do that, as you may only have the choice at that point.

ro0NL commented

Sep 27, 2018

View reviewed changes

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.phpShow resolvedHide resolved

This was referencedSep 29, 2018

[Form] Added choice_filter optionsymfony/symfony-docs#10413

Closed

[symfony/form] ChoiceType add possibility to changechoice_list while building view#28531

Closed

nicolas-grekas added this to thenext milestone

Sep 29, 2018

xabbuh reviewed

Sep 30, 2018

View reviewed changes

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.phpShow resolvedHide resolved

ro0NL commented

Oct 1, 2018

View reviewed changes

src/Symfony/Component/Form/Tests/Fixtures/Author.php OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

ro0NL commentedOct 1, 2018

Ready 👍

xabbuh mentioned this pull request

Oct 1, 2018

[WIP][Form] Allow to configure selectable locale and countries#28542

Closed

HeahDude reviewed

Oct 9, 2018

View reviewed changes

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php Outdated

		};

		$choiceFilterNormalizer =function (Options$options,$choiceFilter) {
		if (null !==$choiceFilter && !\is_callable($choiceFilter)) {

Copy link

Contributor

HeahDudeOct 9, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

So if we pass a valid set of two values to filter out the list but the array happens to be callable this may break. Since it would add complexity to handle this edge case I suggest to either remove the support of arrays or document the limitation.

Copy link

ContributorAuthor

ro0NLOct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

not sure i understand

$form =$this->createForm(ChoiceType::class,null, ['choices' => [1,2,3],'choice_filter' => [3],]);

works as expected...

Copy link

Contributor

HanmacOct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@ro0NL HashDude meant a choice_filter like this["ClassName", "methodName"] for this, is_callable might return true

Copy link

ContributorAuthor

ro0NLOct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

i see, that's edgy yes :) 👍 for removing array support from me

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

ro0NL commentedOct 11, 2018•
edited
Loading

@HeahDude i've optimized it a bit, now it only creates a new loader in case a filter is given. To me that's reasonable in terms of caching, the result would differ per filter anyway.. so not really cacheable to me.

ogizanagi reviewed

Oct 11, 2018

View reviewed changes

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

ro0NL commentedOct 11, 2018

to share cache between filtered/non-filtered choice lists we could add

ChoiceLoaderFactoryInterface::createLoaderFromChoices(iterable $choices): ChoiceLoaderInterface

then we can always use a cacheable loader (either given or created from choices) before decorating it and passing it tocreateListFromLoader. But im not sure it's worth it...

ro0NL mentioned this pull request

Oct 14, 2018

[Form] Deprecate TimezoneType regions option#28860

Merged

fabpot added a commit that referenced this pull request

Oct 25, 2018

feature#28860[Form] Deprecate TimezoneType regions option (ro0NL)

b01ea27

This PR was merged into the 4.2-dev branch.Discussion----------[Form] Deprecate TimezoneType regions option| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? | yes| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets |#28848| License       | MIT| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->I know i've added this option myself in 4.1, but given my recent development for#28624 i realized it's an opinionated feaure, which can/should be solved on user-side (`choice_filter/choice_loader` and/or `group_by`).- blocks translations as we dont have them (see#28831)- blocks possibility of switching to Intl zones which doesnt really have this filter feature (see#28836)~While at it, i solved a few issues with `OptionsResolver` that is able to deprecate options as of 4.2 also.~ Fixed in#28878- when resolved trigger the deprecation- allow to opt-out from triggering the deprecation- dont trigger deprecation for default values (only given ones)Commits-------5cb532d [Form] Deprecate TimezoneType regions option

Copy link

jo66 commentedFeb 4, 2019

@ro0NL choice_filter is nice option, will it be merged?

[Form] Add FilterChoiceLoader

b080d79

Copy link

ContributorAuthor

ro0NL commentedFeb 7, 2019

@jo66 rebased :) let's see if core wants to merge 👍

ro0NL mentioned this pull request

May 23, 2019

[Form] Recall for choice group_by ordering#31593

Closed

Copy link

Member

javiereguiluz commentedJul 3, 2019

Could we restart the work on this to finish it? It looks like a cool feature for Symfony 5.0!

ro0NL mentioned this pull request

Jul 22, 2019

Option to filter choices#32657

Closed

Copy link

ContributorAuthor

ro0NL commentedJul 22, 2019

I tend to close in favor of passing an optimizedchoice_loader (filtered and sorted as expected), that's the fastest way for core to move forward: no changes involved.

In#31593 i've also concluded this.

The only "issue" for core is the built-in Intl choice types, which manage the choice loader for us.

Im thinking of a feature specific to Intl to handle the sorting/ordering here. Much like concluded in#32657 (comment) as well.

Copy link

Member

stof commentedJul 22, 2019

@HeahDude didn't you start working on an alternative implementation at the choice factory level ? I cannot find a PR for it. You were supposed to open it as the result of the FOSSA Hackathon IIRC.