Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
from

Conversation

@HeahDude
Copy link
Member

HeahDude commented Nov 23, 2019

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 Nov 23, 2019
@HeahDude HeahDude mentioned this pull request Nov 23, 2019
0 of 1 task complete
@HeahDude HeahDude force-pushed the HeahDude:form/abstract-choice-loader branch 2 times, most recently from 49d04e5 to f1ab485 Nov 24, 2019
@nicolas-grekas nicolas-grekas added this to the next milestone Nov 24, 2019
@HeahDude

This comment has been minimized.

Copy link
Member Author

HeahDude commented Nov 24, 2019

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

@HeahDude HeahDude force-pushed the HeahDude:form/abstract-choice-loader branch 2 times, most recently from ef26f16 to d9379ad Nov 24, 2019
@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Nov 27, 2019

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

This comment has been minimized.

Copy link
Member Author

HeahDude commented Nov 27, 2019

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

@HeahDude HeahDude force-pushed the HeahDude:form/abstract-choice-loader branch 2 times, most recently from ed731e1 to 961582b Nov 27, 2019
@@ -202,10 +209,10 @@ public function testLoadValuesForChoicesDoesNotLoadIfSingleIntId()
->with($this->obj2)
->willReturn('2');
$this->assertSame(['2'], $loader->loadValuesForChoices([$this->obj2]));
$this->assertSame(['2'], $loader->loadValuesForChoices([$this->obj2], [$this->idReader, 'getIdValue']));

This comment has been minimized.

Copy link
@HeahDude

HeahDude Dec 2, 2019

Author Member

This change was wrong, I reverted it

This comment has been minimized.

Copy link
@HeahDude

HeahDude Dec 7, 2019

Author Member

In fact this is more tricky, by extending the new AbstractChoiceLoader the doctrine one should not handle optimisation for getting the values anymore, since the callback is always defined when this is the case, that's why EntityType integration tests were still passing but not this one.

So I suggest to either modify this test as I did first, or add some deprecation as I did in another commit I pushed here before simplifying again the DoctrineChoiceLoader in 6.0.

WDYT? ping @xabbuh @stof

@HeahDude HeahDude force-pushed the HeahDude:form/abstract-choice-loader branch 4 times, most recently from b6cad00 to 528f9f3 Dec 2, 2019
HeahDude added 2 commits Apr 7, 2019
tmp
@HeahDude HeahDude force-pushed the HeahDude:form/abstract-choice-loader branch from 528f9f3 to 354816b Dec 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.