[Form] Added an AbstractChoiceLoader to simplify implementations and handle global optimizations #34550
+174
−121
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 task
49d04e5 to
f1ab485
Compare
Contributor
Author
|
Appveyor failures unrelated. This one is ready on my side. |
ef26f16 to
d9379ad
Compare
Member
|
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! |
Contributor
Author
|
Thank you for your comment @javiereguiluz, I've updated the description. |
ed731e1 to
961582b
Compare
src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php
Outdated
Show resolved
Hide resolved
HeahDude
commented
Dec 2, 2019
src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php
Outdated
Show resolved
Hide resolved
b6cad00 to
528f9f3
Compare
528f9f3 to
354816b
Compare
stof
reviewed
Dec 18, 2019
src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php
Outdated
Show resolved
Hide resolved
d10b2d2 to
0f4c3b0
Compare
src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Form/ChoiceList/Loader/AbstractChoiceLoader.php
Outdated
Show resolved
Hide resolved
…handle global optimizations
0f4c3b0 to
1394df2
Compare
nicolas-grekas
approved these changes
Feb 12, 2020
fabpot
approved these changes
Feb 12, 2020
Member
|
8000
Thank you @HeahDude. |
fabpot
added a commit
that referenced
this pull request
Feb 12, 2020
…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
added a commit
that referenced
this pull request
Mar 16, 2020
…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
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Taking over #33218 (taking over #30983)
The
ChoiceLoaderInterfaceis 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
$valuecallback (which refers to thechoice_valueoption 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): ChoiceListInterfaceloadChoicesForValues(array $values, ?callable $value): arrayloadValuesForChoices(array $choices, ?callable $value): arrayand handle optimization, in each.
Now we only need to implement:
loadChoices(): iterableand optionnally:
doLoadChoicesForValues(array $values, ?callable $value): ar 8000 rayif 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).