Narrow Form and Mapper API

[jordisala1991]

Subject

I am targeting this branch, because this is a BC break.

Changelog

### Changed
- Narrow the API for classes declared under the `Sonata\AdminBundle\Form\` namespace.
- Narrow the API for classes declared under the `Sonata\AdminBundle\Mapper\` namespace.
- Changed the order of `ModelChoiceLoder` construct arguments

I have applied PHPStan level 8 and Psalm level 2 when doing it and all of the errors related to the typehints are done.

[phansys]

IMO, changelog messages should be focused in the namespaces instead of the directories where they are declared.
By instance:

Narrow the API for classes declared under the Sonata\AdminBundle\Form\ namespace.

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[jordisala1991]

IMO, changelog messages should be focused in the namespaces instead of the directories where they are declared.
By instance:

Narrow the API for classes declared under the Sonata\AdminBundle\Form\ namespace.

Done on all PRs

[VincentLanglet]

IMO, changelog messages should be focused in the namespaces instead of the directories where they are declared.
By instance:

Narrow the API for classes declared under the Sonata\AdminBundle\Form\ namespace.

Done on all PRs

By the way, I discovered this changelog file on master https://github.com/sonata-project/SonataAdminBundle/blob/master/CHANGELOG-4.0.md

Should we do something about it ?

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[jordisala1991]

By the way, I discovered this changelog file on master https://github.com/sonata-project/SonataAdminBundle/blob/master/CHANGELOG-4.0.md

Should we do something about it ?

IMO the initial release of SonataAdmin 4.0 (or alpha version) will need to redo that file and add the missing changelog lines.

[jordisala1991]

This PRs needs to wait to the #6261 merge first. Then I will have to check if everything works, and update it.

[jordisala1991]

This one needs the #6261 first

[jordisala1991]

Just found this: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Builder/FormContractor.php#L104-L131

In theory that field should never be null really. It is a bit strange how everything around this modelType was implemented, but I prefer to fix the test rather than introduce a new else case on a code that shouldn't be reachable. Wdyt?

[VincentLanglet]

Upgrade note is needed

[jordisala1991]

Do you think this case is different from the rest of the code we changed? TBH using this modelType inside sonata can't lead to getting null here as class

[VincentLanglet]

The construct arguments changed.

Before new ModelChoiceLoader($modelManager, $class) was ok.
Now it won't work.

[VincentLanglet]

Just found this: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Builder/FormContractor.php#L104-L131

In theory that field should never be null really. It is a bit strange how everything around this modelType was implemented, but I prefer to fix the test rather than introduce a new else case on a code that shouldn't be reachable. Wdyt?

Im sorry, I dont understand.
Is it an answer to something ?

[VincentLanglet]

I guessed that ModelType, ModelChoiceLoader and ModelToIdTransformer were meant to work together. So I supposed that if one of them requires it, the other one will also always receive the same argument.

I understand that point.

But seems it's not fully needed for ModelChoiceLoader, we could support null for the ModelChoiceLoader "for free".

But you can still open a PR with the failing test on ModelType, we should fix this.

If we are changing that to nullable, I guess at least we should check before using it:

$entities = $this->modelManager->findBy($this->class);

otherwise a TypeError will be thrown.

From

if ($this->query) {

    $entities = $this->modelManager->executeQuery($this->query);

} elseif (\is_array($this->choices) && \count($this->choices) > 0) {

    $entities = $this->choices;

} else {

    $entities = $this->modelManager->findBy($this->class);

}

To

if ($this->query) {

    $entities = $this->modelManager->executeQuery($this->query);

} elseif (\is_array($this->choices) && \count($this->choices) > 0) {

    $entities = $this->choices;

} elseif (null !== $this->class) {

    $entities = $this->modelManager->findBy($this->class);

} else {

    throw some exception "Either query, choices or class should be provided".

}

seems a good idea.

@jordisala1991 WDYT ?

And wdyt about this ?

[jordisala1991]

Im sorry, I dont understand.
Is it an answer to something ?

Yes, sorry I think my previous answer was not really complete.

And wdyt about this ?

Yes, It could work, but the thing is:

ModelType does 3 new with the $options['class'] parameter:

https://github.com/sonata-project/SonataAdminBundle/blob/master/src/Form/Type/ModelType.php#L54
https://github.com/sonata-project/SonataAdminBundle/blob/master/src/Form/Type/ModelType.php#L63
https://github.com/sonata-project/SonataAdminBundle/blob/master/src/Form/Type/ModelType.php#L85-L92

Those classes have the following __construct on master:

https://github.com/sonata-project/SonataAdminBundle/blob/master/src/Form/DataTransformer/ModelsToArrayTransformer.php#L47 (this one is already typed: #5873)
https://github.com/sonata-project/SonataAdminBundle/blob/master/src/Form/DataTransformer/ModelToIdTransformer.php#L43 (this one is changed on this PR to accept only string)
https://github.com/sonata-project/SonataAdminBundle/blob/master/src/Form/ChoiceList/ModelChoiceLoader.php#L82-L89 (this one is changed on this PR too, to accept only string)

The last one is where we having this discussion about accepting or not null. Initially I thought that $options['class'] could be null, and it showed on tests that if I make ModelChoiceLoader construct only accept string, the test suite failed.

What really happens is if we want to be consistent with the null thing I should make the same changes to the three classes.

For now, one could argue that those 3 classes could accept null because it can happen when creating that form field without giving the class option, but reading docs and code I found:

https://symfony.com/doc/master/bundles/SonataAdminBundle/reference/form_types.html#sonata-adminbundle-form-type-modeltype

reading below that...

model_manager
defaults to null, but is actually calculated from the linked admin class. You usually should not need to set this manually.

class
The entity class managed by this field. Defaults to null, but is actually calculated from the linked admin class. You usually should not need to set this manually.

And if we look at code on SonataDoctrineORMAdminBundle, we can find:

https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Builder/FormContractor.php#L104-L131

So, If one uses the ModelType on Sonata, with the persistence bundle (the only viable choice for this form type atm), that code will get executed and the class and modelManager will get added to the default options.

Bonus points, if we consider that class might be null, we should also consider that modelManager could also be null too.

With all the information, my opinion is that:

• We should be consistent between those 3 classes
• class and modelManager should be consistent too
• class and modelManager never are null when using the ModelType on Sonata AbstractAdmin because of the FormContractor on the persistence bundle.

[franmomu]

That's what I meant from the beginning, but well explained 🤣

[VincentLanglet]

Since you're changing the ModelChoiceLoader required arguments (and the order) I think you need a Changelog. Everything else seems ok to me.

Please review @core23

[VincentLanglet]

Please review @core23

Or can you unblock the PR @greg0ire since all the requested changes are made ? :)

Change requested was added more then one month ago. Also it is made now.

[wbloszyk]

@jordisala1991 IMO this is RTM but will be nice to add changelog (like @VincentLanglet mentioned).

[VincentLanglet]

Why can you dismiss the core23 review @wbloszyk ? Or how do you do ? 😅

[jordisala1991]

Changelog is already added, can we have this one merged?

[wbloszyk]

Why can you dismiss the core23 review @wbloszyk ? Or how do you do ?

@rande Add me to @sonata-project/core 🤔

[wbloszyk]

Changelog is already added, can we have this one merged?

In uprade is write about it. ModelChoiceLoader is final and should not be overriden. IMO it is enought.

[wbloszyk]

Thank you @jordisala1991

[VincentLanglet]

Why can you dismiss the core23 review @wbloszyk ? Or how do you do ?

@rande Add me to @sonata-project/core 🤔

That's certainly the reason indeed. I can't ping @sonata-project/core ; only @sonata-project/contributors