Ok so I dropped 8298b1d because the CachingFactoryDecorator (ref #16747) already do the job, then there is no gain in caching the array_flip even if it is justified for TimezoneType.

I still think this abstract class is good for DX and maintainability. For example introducing the choice_translation_domain option would have required one line to this abstract class instead of modifying 5 classes.
And extending ImmutableChoiceType to only implement loadChoices is better for DX than implementing ChoiceLoaderInterface.

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 to 8298b1d (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 a CallableChoiceLoader. To the constructor of that loader, we can pass a callable in each of these types that loads the choices, e.g.:

class CountryType extends AbstractType
{
    private $choiceLoader;

    public function __construct()
    {
        $this->choiceLoader = new CallableChoiceLoader(function () {
            return array_flip(Intl::getRegionBundle()->getCountryNames());
        });
    }

    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults([
            'choice_loader' => $this->choiceLoader,
        ]);
    }
}

For each type, we make sure to reuse the same loader instance, then the caching mechanisms in ChoiceType 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 using choice_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 or AbstractLazyChoiceType, it would be a more generic core version of DoctrineType.

• I feel TimezoneType is looking really good now but the four others could be more optimised soon (in regard of Simplify access to CLDR/ICU data #18368) to directly implement the ChoiceLoaderInterface as we can do for mappers or transformers (e.g [Form] Let TextType implement DataTransformerInterface #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 in LazyChoiceList:

    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. The DoctrineChoiceLoader is not registered as service, it is only instantiated by default in the DoctrineType 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 the ChoiceLoaderInterface 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