I would not rename$container to reduce the diff

According to the current deprecation the fourth argument should be removed

Yep good catch!

I'm wondering if mixing types/guessers/extensions in the same locator is a good thing here, as we have still 3 constructor arguments that's a bit confusing to me.
Instead, couldn'tguesserServices be anIteratorArgument of guessers? It seems we only iterate over them to build a chain one, no need for having them identified right?

Yes It's on the way! This is what I'm currently doing :)

Why not 2 service locators, and one iterator?

So theDependencyInjectionExtension::__construct() method signature would be:

publicfunction __construct(ContainerInterface$typesLocator,ContainerInterface$typeExtensionsLocator,iterator$guessers)

Might not be the easiest for BC though.

Oh, actually$typeExtensionServiceIds cannot easily be a locator, as it is a map of multiple type extension services indexed by type. Symfony'sServiceLocator does not handle collection of services as values.
But couldn't it be a collection of iterables indexed by type?

Edit: actually in order to keep things simple, I think the current implementation is probably safer.

iterable[] make me understand that the value should be an array of iterables.
iterable|FormTypeGuesserInterface[] instead?

Yes it can be an array or an iterator.

In fact, guessers do not need to be instantiated at this point.
I think we should registerFormTypeGuesserChain as a service, pass it guessers as an IteratorArgument and deprecate both$guesserServices and$guesserServiceIds in favor of aFormTypeGuesserInterface $guesser here, even more lazy. Not sure how it would be hard to keep it BC though, maybe deprecating the whole class would be simpler.

Yes that's probably a good thing! I'll check this tomorrow with@nicolas-grekas.

Not sure we needFormTypeGuesserChain to be a service, but simply pass it the guessers iterator (so allowFormTypeGuesserChain to accept iterable).
IMHO the extension is in charge of building the whole thing and the use of theFormTypeGuesserChain is an implementation detail. There is no need to pass the guesser chain as a dependency.

There is an issue withFormTypeGuesserChain, though... its constructor. It iterates over the guessers and this would trigger the loading too soon anyway.

I'm not sure lazy loading here is a big deal though, asFormExtensionInterface::getTypeGuesser() is called inFormRegistryInterface::getTypeGuesser(), called inFormfactory::createBuilderForProperty() and is used directly to guess types by iterating over every guessers.

IMHO the extension is in charge of building the whole thing

Too many responsibilities to me.

and the use of the FormTypeGuesserChain is an implementation detail. There is no need to pass the guesser chain as a dependency.

The fact the guessers are multiple services is a detail IMO. At the end we have only oneFormTypeGuesserInterface $guesser, the class could be usable with an iterable or a simple guesser, which looks better to me. Also it'd be consistent with other places in the core.

There is an issue with FormTypeGuesserChain, though... its constructor. It iterates over the guessers and this would trigger the loading too soon anyway.

Just validation/merge, can be moved at compile time I think.

I thought about deprecating the whole class and replacing it with a new one. But after discussing with@nicolas-grekas it requires too much efforts then to maintain both implementations. This is why we choose to adapt the class itself. One class to maintain and one tests suite. It's just a bit more difficult to read and understand the code at the moment because of the mixed two layers.

It's just a bit more difficult to read and understand the code at the moment because of the mixed two layers.

Indeed :)
I'm unable to say if the BC layer is correct right now, but the approach looks better!

Unit tests prove the BC layer is guaranteed ;)

This change should be reverted I guess.

Why?

The code style and theassertSame change isn't related to this PR, right?

Same here.

Why not 2 service locators, and one iterator?

So theDependencyInjectionExtension::__construct() method signature would be:

publicfunction __construct(ContainerInterface$typesLocator,ContainerInterface$typeExtensionsLocator,iterator$guessers)

Might not be the easiest for BC though.

Not sure we needFormTypeGuesserChain to be a service, but simply pass it the guessers iterator (so allowFormTypeGuesserChain to accept iterable).
IMHO the extension is in charge of building the whole thing and the use of theFormTypeGuesserChain is an implementation detail. There is no need to pass the guesser chain as a dependency.

There is an issue withFormTypeGuesserChain, though... its constructor. It iterates over the guessers and this would trigger the loading too soon anyway.

I'm not sure lazy loading here is a big deal though, asFormExtensionInterface::getTypeGuesser() is called inFormRegistryInterface::getTypeGuesser(), called inFormfactory::createBuilderForProperty() and is used directly to guess types by iterating over every guessers.

Should be reverted too I think.

same

No need forassertInstanceOf asassertEquals fails on an type-class mismatch.

Fixed. thanks.

this is wrong. Form types are accessible only by class, not by service id

Ok thanks!

Fixed.

iterable[] is more precise: array of iterables

not required (as symfony/dependency-injection already requires it)

see PR description for why this is not a BC break (same for all addedpublic="false")