Skip to content

[Form] Fix same choice loader with different choice values #46098

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

Merged

Conversation

HeahDude
Copy link
Contributor

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets #44655
License MIT
Doc PR ~

It appears that deprecating the caching in the LazyChoiceList (cf #18359) was a mistake. The bug went under the radar because in practice every choice field has its own loader instance.
However, optimizations made in #30994 then revealed the flaw (cf #42206) as the loaders were actually shared across many fields.
While working on a fix I ended up implementing something similar to what's proposed in #44655.

I'll send a PR for 5.4 as well.

@HeahDude HeahDude requested review from xabbuh and yceruto as code owners April 18, 2022 22:03
@carsonbot carsonbot added this to the 4.4 milestone Apr 18, 2022
@HeahDude HeahDude changed the title [Form] Add test coverage for same choice loader with different choice values [Form] Fix same choice loader with different choice values Apr 18, 2022
@HeahDude HeahDude force-pushed the fix-choice-loader-different-choice-values branch 3 times, most recently from b41d306 to b2bd85d Compare April 18, 2022 22:34
@xabbuh
Copy link
Member

xabbuh commented Apr 19, 2022

When working on #46030 I stumbled over the tests for LazyChoiceList which did not make much sense to me compared to the application. I ended up with the solution in #46101 which is similar to yours here. But, I am not sure that this actually is a bugfix or if the test were not just misformulated.

For the other changes here I am not sure that this really is the way to go. If I am not missing anything we don't have anything that states that a ChoiceLoaderInterface implementation must return the same list on subsequent calls of loadChoiceList(), do we?

@HeahDude HeahDude force-pushed the fix-choice-loader-different-choice-values branch from 7785c35 to 07252a4 Compare May 14, 2022 07:51
@xabbuh xabbuh force-pushed the fix-choice-loader-different-choice-values branch from 07252a4 to 65cbf18 Compare May 20, 2022 09:08
@xabbuh
Copy link
Member

xabbuh commented May 20, 2022

Thank you Jules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants