Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Sep 26, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #11847, #14072
License MIT
Doc PR TODO

Alternative of #18334 and #28542.

This is what we can do with this new option, before #28542 (comment), after #28542 (comment):

$builder->add('locale', LocaleType::class, [
    'choice_filter' => function ($choice) use ($supportedLocales) {
        return \in_array($choice, $supportedLocales, true);
    },
]);
// or simply
$builder->add('locale', LocaleType::class, [
    'choice_filter' => $supportedLocales, // which does exactly the same as above
]);

In addition, we can use it with EntityType, to filter choices thanks to dynamic properties or methods that could not be used with a custom query builder.

$builder->add('issues', EntityType::class, [
    'class' => Issue::class,
    'choice_filter' => function (Issue $issue) use ($user) {
        return $user->canHandle($issue);
    },
]);

This approach would filter the choices data just after loaded. This way, even filtered choice lists would be properly cached.

@carsonbot carsonbot added Status: Needs Review Form DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature labels Sep 26, 2018
This was referenced Sep 26, 2018
Closed
Closed
Closed
Closed
ro0NL
ro0NL reviewed Sep 26, 2018
View reviewed changes
Copy link
Contributor

@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

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

very nice 👌

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php Outdated
@@ -390,6 +404,10 @@ private function addSubForm(FormBuilderInterface $builder, string $name, ChoiceV
private function createChoiceList(array $options)
{
if (null !== $options['choice_loader']) {
if (null !== $options['choice_filter'] && $options['choice_loader'] instanceof ChoiceFilterInterface) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i think something should happen when the choice loader does not support ChoiceFilterInterface. E.g. wrap it in a decorating / callback loader, or throw.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I think throwing an exception is a good DX 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Decorating is not an option, because we lose the filtered list or the custom logic inside loadValuesForChoices and loadChoicesForValues in the decorated loader.

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php Outdated
@@ -399,6 +417,10 @@ private function createChoiceList(array $options)
// Harden against NULL values (like in EntityType and ModelType)
$choices = null !== $options['choices'] ? $options['choices'] : array();

if (null !== $options['choice_filter']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

&& $choices? Not sure it matters much... =/

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, should save some useless function calls 👍

ogizanagi
ogizanagi reviewed Sep 26, 2018
View reviewed changes
src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php
/**
* {@inheritdoc}
*/
public function setChoiceFilter(callable $choiceFilter)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wild thoughts, as I don't know all the implications right now: I don't know if a new interface with a setter is a good idea. Shouldn't we simply inject in the loaders' constructors the callable $choiceFilter instead?

Copy link
Contributor

@ro0NL ro0NL Sep 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

👍 makes sense, to keep compatibility with any choice_loader we could still decorate it if the choice filter wasnt injected initially.

Copy link
Member Author

Choose a reason for hiding this comment

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

I expected this question :) My goal is enable this feature for all (more and more used) CallbackChoiceLoader in current projects, automatically.

Otherwise, the feature will be enabled only for core types (e.g. Intl form types, EntityType) and will require more code and then more maintenance (injecting the $options['choice_filter'] everywhere).

This was the simplest solution I found to achieve a better DX, so callback loaders don't need to do something to enable this option.

Copy link
Member Author

@yceruto yceruto Sep 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Also, this would allow for custom choice loader to enable this feature without care about injecting $options['choice_filter'] everywhere the loader is used. We only care about the implementation as a plug-and-play interface.

Btw, there are many examples of interface with a setter into the core, I'm not sure it's a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

see #28624 for a different approach

@yceruto yceruto force-pushed the choice_filter branch 2 times, most recently from 543962b to fa0a5e3 Compare September 26, 2018 15:45
@ro0NL ro0NL mentioned this pull request Sep 27, 2018
Closed
5 tasks
@nicolas-grekas nicolas-grekas added this to the next milestone Sep 29, 2018
@xabbuh
Copy link
Member

xabbuh commented Oct 1, 2018

Should we close here? #28624 looks like the cleaner solution to me. Thank you @yceruto for your work on this! 👍

@yceruto
Copy link
Member Author

yceruto commented Oct 1, 2018

Sure :) I need some time to review #28624 deeply, but it looks good to me.

@yceruto yceruto closed this Oct 1, 2018
@yceruto yceruto deleted the choice_filter branch October 27, 2018 13:54
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ro0NL ro0NL ro0NL left review comments

@ogizanagi ogizanagi ogizanagi left review comments

Assignees
No one assigned
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Form Status: Needs Review
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
6 participants
@yceruto @xabbuh @ro0NL @ogizanagi @nicolas-grekas @carsonbot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.