Ok so I dropped8298b1d because theCachingFactoryDecorator (ref#16747) already do the job, then there is no gain in caching thearray_flip even if it is justified forTimezoneType.

I still think this abstract class is good for DX and maintainability. For example introducing thechoice_translation_domain option would have required one line to this abstract class instead of modifying 5 classes.
And extendingImmutableChoiceType to only implementloadChoices is better for DX than implementingChoiceLoaderInterface.

This may be trivial but there is no harm in performance, I've tried this code in SE:

$builder =$this->createFormBuilder();for ($i =0;$i <10; ++$i) {$builder->add('country'.$i,FormType\CountryType::class);$builder->add('currency'.$i,FormType\CurrencyType::class);$builder->add('language'.$i,FormType\LanguageType::class);$builder->add('locale'.$i,FormType\LocaleType::class);$builder->add('timezone'.$i,FormType\TimezoneType::class);}$form =$builder->getForm();

From base to8298b1d (try to improve cache with static properties:

And from base to refactoring (current PR):

ping@webmozart, does it fit with#18368 ?

A simpler and more flexible solution would be to add aCallableChoiceLoader. To the constructor of that loader, we can pass a callable in each of these types that loads the choices, e.g.:

class CountryTypeextends AbstractType{private$choiceLoader;publicfunction__construct()    {$this->choiceLoader =newCallableChoiceLoader(function () {returnarray_flip(Intl::getRegionBundle()->getCountryNames());        });    }publicfunctionconfigureOptions(OptionsResolver$resolver)    {$resolver->setDefaults(['choice_loader' =>$this->choiceLoader,        ]);    }}

For each type, we make sure to reuse the same loader instance, then the caching mechanisms inChoiceType will automatically take effect.

I thought about it too since it is very easy to do, I'm glad you brought that up, I might be done in time :)

Should I go on with it ?

I prefer the solution suggested by@webmozart

Rebased.

I first thought of usingchoice_loader as any callable when dealing with#18334, but I was not sure that it would be good to add an interface for that, although that's looks very great for those sub types!

I think this callable choice loader will really improve performances, I will try to profile something today :)

Also, I still think of some things:

• it would be good to decouple the need to inherit from choice type basis from the need to load choices in a permanent way to ease maintaining them.

  This was originally sent as PoC, but it would be good to find a better name for such an abstract class:
  LoadedChoiceType orAbstractLazyChoiceType, it would be a more generic core version ofDoctrineType.

• I feelTimezoneType is looking really good now but the four others could be more optimised soon (in regard ofSimplify access to CLDR/ICU data #18368) to directly implement theChoiceLoaderInterface as we can do for mappers or transformers (e.g[Form] LetTextType implementDataTransformerInterface #18357).

Concerning choice loaders in general, if this PR is merged, this would be the first one implemented in the form core and in regard of the work I'm trying to do in#18359, I think there is two much caching when using loaders here's a schema:

Needs choice list (configuration | loading form types)|=> factory   |   |__ from choices ?   |   |   |   |__ decorated with cache ?   |   |   |         |   |   |__ => return ArrayChoiceList   |   |   |   |   |   |__ decorates   |   |   |   |__ => creates   |       |   |       |__ => cache => return ArrayChoiceList    |   |   |__ from loader ?       |       |__ decorated with cache ?           |             |__ => return ArrayChoiceList           |           |           |__ decorates               |               |__ => cache (*1) and return **LazyChoiceList**Then later or not uses choice list (actually build form or view)|=> not lazy ?    |    |__ => return classic (good)    |    |__ is loaded ?        |        |__ => return classic        |        |__ call loader            |            |__ is loaded ?            |   |            |   |__ => return classic            |   |            |   |__ call factory            |   |   |            |   |   |__ decorated with cache            |   |   |    |            |   |   |    |__ wtf ?            |   |   |            |   |   |__ decorated with property path ?            |   |   |    |            |   |   |    |__ wtf ?            |   |   |            |   |   |__ factory creates, cache (*2) and return ArrayChoiceList            |   |            |   |__ loader cache (*3)            |            |__ lazy choice list cache (*4)

The(*n) marker indicates when the caching implies the number of copy of the same choice list.

So I suggest to use the following code inLazyChoiceList:

    public function getChoices()    {        // BC        if (!$this->isLoaded) {            $this->loadedList = $this->loader->loadChoiceList($this->value);            $this->isLoaded = true;        } else {            if ($this->loadedList !== $loadedList = $this->loader->loadChoiceList($this->value) {                // Trigger deprecation notice for caching in lazy choice list                // not caching in choice loader unsupported in 4.0                return $this->loadedList->getChoices();            }        }        // End bc        return $this->loader->loadChoiceList($this->value)->getChoices();    }

And together with the change of#18359 we can get in 4.0:

Uses choice list (actually build form or view)|=> not lazy ?    |    |__ => return classic (good)    |    |    |__ call loader        |        |__ is loaded ?            |            |__ => return classic            |            |__ loader creates, cache (*2) and returns ChoiceListInterface

I think the loader should not know about factories. TheDoctrineChoiceLoader is not registered as service, it is only instantiated by default in theDoctrineType which passes it the factory so it's not really an extension point.

What do you think ?

Thanks.

When exactly will 3.1 be feature frozen ?

@stof All comments addressed here, plus the optimization by implementing theChoiceLoaderInterface directly!

This one still has a chance. I'm just waiting for the appveyor tests to be green :)

Green :)

I will try to profile again but there's no doubt that they'll be better than above :)

Alright, last comments addressed! Failures are unrelated, I think I'm done here :)

👍

Rebased. Tests still green.

Last comment here is@fabpot approval. Is this still planned for 3.1? If so, it should be merged as soon as possible to be tested during beta.

ping@symfony/deciders

Thanks.

Re-re-rebased :)

Comments addressed and changelog updated to target 3.2.

@HeahDude Can you submit a PR for the docs? Thanks.

Thank you@HeahDude.

Will do, thank you@fabpot