[POC] Add guessTypeForFieldDescription to TypeGuesserInterface

[franmomu]

Subject

For a long description: #6701

Basically, in persistence bundles, to guess the type we have a call to a nonexistent ModelManagerInterface::getParentMetadataForProperty() which fetches the mapping for a property, the field description should have all the mapping information when is created, so it should be enough for guessing the right type.

I am targeting this branch, because this is BC.

Closes #6701.

Changelog

### Added
- Added `TypeGuesserInterface::guessTypeForFieldDescription()` method.
- Added `TypeGuesserChain::guessTypeForFieldDescription()` method.
### Deprecated
- Deprecate `TypeGuesserInterface::guessType()` method in favor of `TypeGuesserInterface::guessTypeForFieldDescription()`.
- Deprecate `TypeGuesserChain::guessType()` method in favor of `TypeGuesserInterface::guessTypeForFieldDescription()`.

To be honest I don't like the name, but I think there is no way to reuse guessType without breaking BC, any other idea is welcome.

[VincentLanglet]

GuessType($classOrFieldDescription, $property = null, $modelManager = null)

Couldnt this be a way to change from a signature to another with BC ?

[franmomu]

GuessType($classOrFieldDescription, $property = null, $modelManager = null)

Couldnt this be a way to change from a signature to another with BC ?

Nope, It is not BC: https://3v4l.org/KZBXZ

Method parameter types are contravariant (only allow a more general argument).

[VincentLanglet]

WDYT of GuessFieldDescriptionType ?

Or we can keep guessType if we make the Pr directly on master and document the BC break...

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[wbloszyk]

@VincentLanglet on line 43, Is it wrong - $guesser->guessers ?

SonataAdminBundle/src/Guesser/TypeGuesserChain.php

Lines 35 to 43 in f80fca5

	public function __construct(array $guessers)
	{
	foreach ($guessers as $guesser) {
	if (!$guesser instanceof TypeGuesserInterface) {
	throw new UnexpectedTypeException($guesser, TypeGuesserInterface::class);
	}
	if ($guesser instanceof self) {
	$this->guessers = array_merge($this->guessers, $guesser->guessers);

[VincentLanglet]

@VincentLanglet on line 43, Is it wrong - $guesser->guessers ?

SonataAdminBundle/src/Guesser/TypeGuesserChain.php

Lines 35 to 43 in f80fca5

	public function __construct(array $guessers)
	{
	foreach ($guessers as $guesser) {
	if (!$guesser instanceof TypeGuesserInterface) {
	throw new UnexpectedTypeException($guesser, TypeGuesserInterface::class);
	}
	if ($guesser instanceof self) {
	$this->guessers = array_merge($this->guessers, $guesser->guessers);

If I understand correctly the guessers property is protected so it shouldn't work.
We should add a getGuessers() method instead. But it's not related to this PR.

[wbloszyk]

If we wanna guess type only by one method then we should keep guessType() naming. Having guessTypeForFieldDescription() when you havent guessType() is not respecting KISS.
So can you do something like this:

public function guessType($fieldDescriptionOrDeprecaedClass, $deprecatedProperty, ModelManagerInterface $deprecatedModelManager)

[VincentLanglet]

So can you do something like this:

public function guessType($fieldDescriptionOrDeprecaedClass, $deprecatedProperty, ModelManagerInterface $deprecatedModelManager)

It will still lead to BC break in 4.0.
User will have to change from guessType($fieldDescription, '', $modelManager) to guessType($fieldDescription).

If we want to avoid BC break we have to use a new method like guessFieldDescriptionType and deprecate the other.
If not, I think we could do this directly on master.

[wbloszyk]

So can you do something like this:

public function guessType($fieldDescriptionOrDeprecaedClass, $deprecatedProperty, ModelManagerInterface $deprecatedModelManager)

It will still lead to BC break in 4.0.
User will have to change from guessType($fieldDescription, '', $modelManager) to guessType($fieldDescription).

If we want to avoid BC break we have to use a new method like guessFieldDescriptionType and deprecate the other.
If not, I think we could do this directly on master.

What is diffrence beetween?:

public function guessType($fieldDescriptionOrDeprecaedClass, $deprecatedProperty, ModelManagerInterface 
        $deprecatedModelManager) 
{
    if ($fieldDescriptionOrDeprecaedClass instaneof FieldDescription) {
        // call guessTypeForFieldDescription() which will be merge in master or write code diretly here 
        return $this->guessTypeForFieldDescription($fieldDescriptionOrDeprecaedClass);
    } // old code + trigger_errors
}

protected function guessTypeForFieldDescription(FieldDescription $fieldDescription) {}

and

public function guessType($class, $property, ModelManagerInterface $ModelManager) {}
public function guessTypeForFieldDescription(FieldDescription $fieldDescription) {}

[VincentLanglet]

It's not about our implementation, it's about the developer who implement the interface and the developer who use the method.

When we're introducing a deprecation, the developer can update his code in order to fix the deprecation AND avoiding a BC break in 4.0.
Here you don't avoid the BC break.

If I need to use guessType, I'll do

guessType($fieldDescription, $deprecatedProperty, $deprecatedModelManager)

But in next major, I'll have to change my code to

guessType($fieldDescription)

And I cant do the change when using the 3.x branch.
Same if you implement the guessType method.

If the new signature was

guessType($classOrFieldDescription, $property = null, $modelManager = null)

I could do

guessType($fieldDescription)

in 3.x directly. But as explained by @franmomu this is a BC break.

[VincentLanglet]

Do you have some time to rebase @franmomu ?

And Wdyt of guessFieldDescriptionType instead ?

Maybe one day we will guess another Type, like a guessExportFieldType

[franmomu]

Hey, sorry, I've been busy these days, I think I can continue this weekend.

And Wdyt of guessFieldDescriptionType instead ?

For #6761 I had something done and created a FieldDescription folder where all the FieldDescription* could eventually end up there, so creating something new could be a solution.

[VincentLanglet]

For #6761 I had something done and created a FieldDescription folder where all the FieldDescription* could eventually end up there, so creating something new could be a solution.

I like this solution. And a field description folder is a nice idea.

[VincentLanglet]

Closing in favor of #6854