8000 [Form] Added an AbstractChoiceLoader to simplify implementations and handle global optimizations by HeahDude · Pull Request #34550 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] Added an AbstractChoiceLoader to simplify implementations and handle global optimizations #34550

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
Merged
merged 1 commit into from
Feb 12, 2020
Merged

[Form] Added an AbstractChoiceLoader to simplify implementations and handle global optimizations #34550

merged 1 commit into from
Feb 12, 2020

Conversation

HeahDude
Copy link
Contributor
@HeahDude HeahDude commented Nov 23, 2019 •
edited
Loading
Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets ~
License MIT
Doc PR ~

Taking over #33218 (taking over #30983)

The ChoiceLoaderInterface is not easy to understand/implement, while its goal is simple, lazy load an array of choices.
What may seem complicated is the how/what to optimize loading, we need to deal with a $value callback (which refers to the choice_value option allowing to transform a model choice to a unique string value that can be displayed/submitted).

We have now enough implementations in core to justify the need of an abstraction and provide a better DX in the process.

Before this PR we needed to implement 3 methods to create a loader:

  • loadChoiceList(?callable $value): ChoiceListInterface
  • loadChoicesForValues(array $values, ?callable $value): array
  • loadValuesForChoices(array $choices, ?callable $value): array
    and handle optimization, in each.

Now we only need to implement:

  • loadChoices(): iterable

and optionnally:

  • doLoadChoicesForValues(array $values, ?callable $value): array
    if more optimization is needed to only load submitted values, (i.e the core intl loader prevents loading values that are the same as choices, and the doctrine one performs a WHERE id IN ($ids) query).

@HeahDude HeahDude requested a review from xabbuh as a code owner November 23, 2019 14:23
@HeahDude HeahDude mentioned this pull request Nov 23, 2019
Closed
1 task
@HeahDude HeahDude force-pushed the form/abstract-choice-loader branch 2 times, most recently from 49d04e5 to f1ab485 Compare November 24, 2019 17:05
@nicolas-grekas nicolas-grekas added this to the next milestone Nov 24, 2019
@HeahDude
Copy link
Contributor Author

Appveyor failures unrelated. This one is ready on my side.

@HeahDude HeahDude force-pushed the form/abstract-choice-loader branch 2 times, most recently from ef26f16 to d9379ad Compare November 25, 2019 18:57
@javiereguiluz
Copy link
Member
javiereguiluz commented Nov 27, 2019 •
edited
Loading

Hi Jules! This looks "complicated" because different people have attempted to solve this in multiple different pull requests. So, could you please update your pull request description with a link to the issue/PR which explains how this feature will behave for end users?

If not such a link exists, please update the PR description to briefly explain what does this do, how does it work, show a simple example, etc.

Thanks!

@HeahDude
Copy link
Contributor Author

Thank you for your comment @javiereguiluz, I've updated the description.

@HeahDude HeahDude force-pushed the form/abstract-choice-loader branch 2 times, most recently from ed731e1 to 961582b Compare December 2, 2019 11:57
@HeahDude HeahDude force-pushed the form/abstract-choice-loader branch 4 times, most recently from b6cad00 to 528f9f3 Compare December 8, 2019 13:30
@HeahDude HeahDude force-pushed the form/abstract-choice-loader branch from 528f9f3 to 354816b Compare December 14, 2019 17:17
@HeahDude HeahDude force-pushed the form/abstract-choice-loader branch 2 times, most recently from d10b2d2 to 0f4c3b0 Compare February 9, 2020 13:43
@HeahDude HeahDude force-pushed the form/abstract-choice-loader branch from 0f4c3b0 to 1394df2 Compare February 9, 2020 23:49
@fabpot
Copy link
Member
fabpot commented Feb 12, 2020

Thank you @HeahDude.

@fabpot fabpot mentioned this pull request Feb 12, 2020
Closed
fabpot added a commit that referenced this pull request Feb 12, 2020
@fabpot
feature #34550 [Form] Added an AbstractChoiceLoader to simplify imple…
31fb219 
…mentations and handle global optimizations (HeahDude)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[Form] Added an AbstractChoiceLoader to simplify implementations and handle global optimizations

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | ~
| License       | MIT
| Doc PR        | ~
<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Always add tests and ensure they pass.
 - Never break backward compatibility (see https://symfony.com/bc).
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too.)
 - Features and deprecations must be submitted against branch master.
-->
Taking over #33218 (taking over #30983)

The `ChoiceLoaderInterface` is not easy to understand/implement, while its goal is simple, lazy load an array of choices.
What may seem complicated is the how/what to optimize loading, we need to deal with a `$value` callback (which refers to the `choice_value` option allowing to transform a model choice to a unique string value that can be displayed/submitted).

We have now enough implementations in core to justify the need of an abstraction and provide a better DX in the process.

Before this PR we needed to implement 3 methods to create a loader:
 - `loadChoiceList(?callable $value): ChoiceListInterface`
 - `loadChoicesForValues(array $values, ?callable $value): array`
 - `loadValuesForChoices(array $choices, ?callable $value): array`
and handle optimization, in each.

Now we only need to implement:
 - `loadChoices():  iterable`

and optionnally:
 - `doLoadChoicesForValues(array $values, ?callable $value): array`
if more optimization is needed to only load submitted values, (i.e the core intl loader prevents loading values that are the same as choices, and the doctrine one performs a `WHERE id IN ($ids)` query).

Commits
-------

1394df2 [Form] Added an AbstractChoiceLoader to simplify implementations and handle global optimizations
@fabpot fabpot merged commit 1394df2 into symfony:master Feb 12, 2020
@HeahDude HeahDude deleted the form/abstract-choice-loader branch February 12, 2020 21:06
@HeahDude HeahDude mentioned this pull request Feb 15, 2020
Merged
fabpot added a commit that referenced this pull request Mar 16, 2020
@fabpot
feature #35733 [Form] Added a "choice_filter" option to ChoiceType (H…
e0bddee 
…eahDude)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[Form] Added a "choice_filter" option to ChoiceType

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       | Fix #32657
| License       | MIT
| Doc PR        | symfony/symfony-docs#13223

Finally opening this PR for a very old branch, based on both #34550 (merged) and #30994 (merged).

~Until #30994 is merged, this PR should better be reviewed by commits. Thanks!~

Commits
-------

ed2c312 [Form] Added a "choice_filter" option to ChoiceType
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
Merged
@kbond kbond mentioned this pull request Apr 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Feature Form Status: Reviewed
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.
6 participants
@HeahDude @javiereguiluz @fabpot @nicolas-grekas @stof @carsonbot
0