[Form] Refactor DefaultChoiceListFactory

[vudaltsov]

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	n/a

In this PR I refactored a really messy DefaultChoiceListFactory code.
Ping @HeahDude, @xabbuh.

[vudaltsov]

Blackfire comparison for a form with 3 different choices.

So, it became slower a bit because of increased number of method calls basically. Is it too much for a DX improvement?

[vudaltsov]

Rebased

[stof]

So, it became slower a bit because of increased number of method calls basically. Is it too much for a DX improvement?

What kind of DX improvement is this PR doing ? Developers don't care about the internal implementation of the factory. They use the factory when developing their forms, they don't re-implement it.

[vudaltsov]

I meant Symfony developer's DX. I started working on #5136 when I came across the DefaultChoiceListFactory. It took me about 2 hours to get, what's going on there.

[vudaltsov]

Thank you @stof , I improved the phpdoc

[stof]

I meant Symfony developer's DX. I started working on #5136 when I came across the DefaultChoiceListFactory. It took me about 2 hours to get, what's going on there.

But does your refactoring change that at all ? The building of the list is still exactly the same, but moved in a different class AFAICT.

[vudaltsov]

@stof, I certainly didn't change the algorithm, but I claim that I improved the readability:

1. Stateful builder instead of stateless static factory methods. The builder incapsulates the logic of constructing a view object and delegates small tasks to private methods.
2. Less arguments. static DefaultChoiceListFactory::addChoiceView used to have 9 arguments which is almost impossible to keep in mind. New ChoiceListViewBuilder::addView has only 5 args which is still bad but almost two times easier to process in mind. That's because we still have to pass arrays of choices by reference due to the recursive nature of the building process.
3. Better method names. Old addChoiceViewsGroupedBy & addChoiceViewGroupedBy vs new addViewsGroupedByCallable & addViewsFromStructuredValues.
4. Label, index, attr, preferred choices are now resolved in separate methods which improves readability a lot because of early return statements instead of elseif structures.
5. Optimization: removing empty groups views used to happen for any $groupBy type (https://github.com/symfony/symfony/pull/29731/files#diff-ca1123eff9e0aa207aaec667aa16c8e8L99), now it happens only for callable $groupBy (https://github.com/symfony/symfony/pull/29731/files#diff-dbdec4be38173e7941f77b7b9292313eR111), because for structured choices we never create empty groups. I also moved it to a separate method to avoid code duplication.

[vudaltsov]

@nicolas-grekas , @xabbuh , @stof , @HeahDude , should I close this? Are my points above not convincing?

[xabbuh]

Please don't close it yet. I would like to have a look at the details first. I simply didn't have the time to do that yet. Sorry.

[fabpot]

I'm not convinced, and it makes merging older branches more difficult (not even talking about the performance here).

[HeahDude]

Thank you @vudaltsov, I'm sorry but I also tend to agree, that the benefit is too small so it is worth it.

👎 too on this one.

[vudaltsov]

Okay, no problem. I finally also agree with all you. My change does not really change anything :)