Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[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.
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
49d04e5
tof1ab485
CompareAppveyor failures unrelated. This one is ready on my side. |
ef26f16
tod9379ad
Comparejaviereguiluz commentedNov 27, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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! |
Thank you for your comment@javiereguiluz, I've updated the description. |
ed731e1
to961582b
CompareUh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
b6cad00
to528f9f3
Compare528f9f3
to354816b
Comparesrc/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
d10b2d2
to0f4c3b0
Comparesrc/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Form/ChoiceList/Loader/AbstractChoiceLoader.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
…handle global optimizations
0f4c3b0
to1394df2
CompareThank you@HeahDude. |
…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 peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://symfony.com/roadmap): - Always add tests and ensure they pass. - Never break backward compatibility (seehttps://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
…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#13223Finally 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
Uh oh!
There was an error while loading.Please reload this page.
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 thechoice_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).