Introduce field description factory #6854

franmomu · 2021-02-14T17:50:15Z

Subject

For background: #6761 and #6701.

First of all, I had this half done, so I've "finished", I'm fine postponing this for 5.0 if we think this shouldn't be done now (and if this is fine ofc).

The idea of this is basically to move responsibility of creating a FieldDescription from ModelManagerInterface to FielDescriptionFactoryInterface.

And also there is a new Sonata\AdminBundle\FieldDescription\TypeGuesserInterface that will guess the FieldDescription type base on a FieldDescriptionInterface properties instead of rely on ModelManagerInterface.

I've created sonata-project/SonataDoctrineMongoDBAdminBundle#531 (unfinished) as example of implementation.

I am targeting this branch, because these changes are BC.

Closes #6761 and #6701.

Changelog

### Added
- Added `FieldDescriptionFactoryInterface` to create FieldDescription instances.
- Added `TypeGuesserInterface` to guess the proper FieldDescription type based on its properties.
### Deprecated
- Deprecated `ModelManagerInterface::getNewFieldDescriptionInstance`.

src/DependencyInjection/Admin/AbstractTaggedAdmin.php

OskarStark · 2021-02-14T19:43:07Z

src/DependencyInjection/Admin/TaggedAdminInterface.php

- * @method Pool                          getConfigurationPool()
- * @method void                          setRouteGenerator(RouteGeneratorInterface $routeGenerator)
- * @method RouteGeneratorInterface       getRouteGenerator()
+ * @method void                             initialize()


All of these method annotations look weird 🧐

Just an idea for all these methods...

Can we add a new NextMajorTaggedAdminInterface with all these methods and mark it as @internal.

We can then use @mixin NextMajorTaggedAdminInterface to include all these methods.

tests/App/FieldDescription/FieldDescriptionFactory.php

VincentLanglet

You can rebase 3.x to fix the phpstan failures

src/Admin/AbstractAdmin.php

src/DependencyInjection/Admin/AbstractTaggedAdmin.php

VincentLanglet · 2021-02-16T00:42:57Z

It could be great to have this in 4.0 since it will solved the following issue too if I understand correctly:
sonata-project/SonataDoctrineORMAdminBundle#1296

VincentLanglet · 2021-02-16T01:05:47Z

src/Admin/AbstractAdmin.php

@@ -3077,7 +3074,6 @@ protected function buildList()
                ]
            );

-            $fieldDescription->setAdmin($this);


You have to keep this, no ?

VincentLanglet · 2021-02-16T01:05:53Z

src/Admin/AbstractAdmin.php

@@ -3103,7 +3098,6 @@ protected function buildList()
                ]
            );

-            $fieldDescription->setAdmin($this);


VincentLanglet · 2021-02-16T01:09:01Z

src/FieldDescription/FieldDescriptionFactoryInterface.php

+    /**
+     * @phpstan-param class-string $class
+     */
+    public function create(string $class, string $name, array $options = []): FieldDescriptionInterface;


Can we use a more explicite name than $class ?

we could use modelClass maybe, but I guess this is "sync" with AdminInterface::getClass

Can we add some type hints for the $options?

VincentLanglet · 2021-02-16T01:09:55Z

src/FieldDescription/TypeGuesserInterface.php

+
+interface TypeGuesserInterface
+{
+    public function guess(FieldDescriptionInterface $fieldDescription): TypeGuess;


Should we create a TypeGuesserChain too ?

I wasn't sure, I guess so, I'll take a look.

franmomu · 2021-02-16T01:14:46Z

It could be great to have this in 4.0 since it will solved the following issue too if I understand correctly:
sonata-project/SonataDoctrineORMAdminBundle#1296

Yes, that is the idea, we could also remove all the fieldDescription->setAdmin calls and remove setting the doctrine mapping type to field description type. I'll try to continue tomorrow.

franmomu · 2021-02-19T00:48:01Z

I'll try to advance/finish it at the weekend, I have some doubts about how to enforce calling FieldDescriptionInterface::setAdmin() method always once FieldDescriptionInterface is created, because apart from AbstractAdmin, there are calls from the mappers: https://github.com/sonata-project/SonataAdminBundle/search?q=getNewFieldDescriptionInstance.

I thought about creating a FieldDescriptionCreator, something like:

final class FieldDescriptionCreator implements FieldDescriptionCreatorInterface
{
    public function create(AdminInterface $admin, string $propertyName, array $options = []): FieldDescriptionInterface
    {
        $fieldDescriptionFactory = $admin->getFieldDescriptionFactory();

        if (null === $fieldDescriptionFactory) {
            $fieldDescription = $admin->getModelManager()->getNewFieldDescriptionInstance(
                $this->getClass(),
                $propertyName,
                $options
            );
        } else {
            $fieldDescription = $fieldDescriptionFactory->create($admin->getClass(), $propertyName, $options);
        }

        $fieldDescription->setAdmin($admin);

        return $fieldDescription;
    }
}

This would work fine for Mappers, but maybe it's strange to inject this in Admins too. I'll give it some thought and check this current getNewFieldDescriptionInstance calls from AbstractAdmin.

VincentLanglet

What is needed to finish this @franmomu ?

franmomu · 2021-02-28T21:55:35Z

What is needed to finish this @franmomu ?

I think now some reviews to see if it's fine and then I'll create both PR for the persistence bundles.

VincentLanglet

after this PR we should move

FieldDescriptionCollection.php
FieldDescriptionInterface.php
FieldDescriptionRegistryInterface.php

To the new directory FieldDescription and deprecate the old classes

core23 · 2021-03-04T08:02:20Z

src/Admin/AbstractAdmin.php

@@ -2892,6 +2890,26 @@ final public function hasTemplateRegistry(): bool
        return null !== $this->templateRegistry;
    }

+    public function createFieldDescription(string $propertyName, array $options = []): FieldDescriptionInterface


core23 · 2021-03-04T08:03:58Z

src/FieldDescription/FieldDescriptionFactoryInterface.php

+    /**
+     * @phpstan-param class-string $class
+     */
+    public function create(string $class, string $name, array $options = []): FieldDescriptionInterface;


Can we add some type hints for the $options?

core23 · 2021-03-04T08:04:13Z

src/FieldDescription/TypeGuesserChain.php

+     */
+    private $guessers = [];
+
+    public function __construct(array $guessers)


Type hint missing

core23 · 2021-03-06T12:22:55Z

Can we use the factory to solve this issue too? sonata-project/SonataBlockBundle#244

VincentLanglet · 2021-03-07T14:18:39Z

Friendly ping @franmomu ; do you have some time to finish this ? :)

franmomu · 2021-03-07T18:45:17Z

Can we use the factory to solve this issue too? sonata-project/SonataBlockBundle#244

hmmmm I guess that this code:

// simulate an association ...
$fieldDescription = $this->getGalleryAdmin()->getModelManager()->getNewFieldDescriptionInstance($this->getGalleryAdmin()->getClass(), 'media', [
   'translation_domain' => 'SonataMediaBundle',
]);
$fieldDescription->setAssociationAdmin($this->getGalleryAdmin());
$fieldDescription->setAdmin($formMapper->getAdmin());
$fieldDescription->setOption('edit', 'list');
$fieldDescription->setAssociationMapping(['fieldName' => 'gallery', 'type' => ClassMetadataInfo::MANY_TO_ONE]);

Will become:

$fieldDescription = $this->getGalleryAdmin()->createFieldDescription('media', [
   'translation_domain' => 'SonataMediaBundle',
    'edit' => 'list',
]);
// Not sure about this one, I think is not necessary
$fieldDescription->setAssociationAdmin($this->getGalleryAdmin());

But to do this: sonata-project/SonataBlockBundle#244 (comment)

I guess we could create something like:

SonataAdminBundle/src/Form/Extension/Field/Type/FormTypeFieldExtension.php

Lines 75 to 87 in 4c97daf

    
           if ($options['sonata_field_description'] instanceof FieldDescriptionInterface) { 
        
               $fieldDescription = $options['sonata_field_description']; 
        
               $sonataAdmin['admin'] = $fieldDescription->getAdmin(); 
        
               $sonataAdmin['field_description'] = $fieldDescription; 
        
               $sonataAdmin['name'] = $fieldDescription->getName(); 
        
               $sonataAdmin['edit'] = $fieldDescription->getOption('edit', 'standard'); 
        
               $sonataAdmin['inline'] = $fieldDescription->getOption('inline', 'natural'); 
        
               $sonataAdmin['block_name'] = $fieldDescription->getOption('block_name', false); 
        
               $sonataAdmin['class'] = $this->getClass($builder); 
        
               $builder->setAttribute('sonata_admin_enabled', true); 
        
           }

And I guess we could call $options['admin']->createFieldDescription(), the name I supposed that can be fetched from the builder (all of this theoretically).

I'll address the suggestions later today.

This interface will be responsible of creating FieldDescriptionInterface instances.

This will guess the field description type from its properties.

This method will use the field description factory and also sets the current admin to the field description.

Deprecated in favor of Sonata\AdminBundle\FieldDescription\TypeGuesserInterface.

core23 · 2021-03-09T19:49:59Z

src/Admin/AbstractAdmin.php

@@ -2892,6 +2890,26 @@ final public function hasTemplateRegistry(): bool
        return null !== $this->templateRegistry;
    }

+    final public function createFieldDescription(string $propertyName, array $options = []): FieldDescriptionInterface


Can you provide type hints for the $options?

core23 · 2021-03-09T19:56:20Z

src/DependencyInjection/Admin/TaggedAdminInterface.php

- * @method Pool                          getConfigurationPool()
- * @method void                          setRouteGenerator(RouteGeneratorInterface $routeGenerator)
- * @method RouteGeneratorInterface       getRouteGenerator()
+ * @method void                             initialize()


Just an idea for all these methods...

Can we add a new NextMajorTaggedAdminInterface with all these methods and mark it as @internal.

We can then use @mixin NextMajorTaggedAdminInterface to include all these methods.

franmomu added the minor label Feb 14, 2021

OskarStark reviewed Feb 14, 2021

View reviewed changes

VincentLanglet requested changes Feb 15, 2021

View reviewed changes

src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved

src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved

src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved

src/DependencyInjection/Admin/AbstractTaggedAdmin.php Outdated Show resolved Hide resolved

franmomu force-pushed the introduce_field_description_factory branch 4 times, most recently from f52897b to 8f3dfb1 Compare February 16, 2021 00:55

VincentLanglet reviewed Feb 16, 2021

View reviewed changes

franmomu marked this pull request as draft February 16, 2021 08:37

franmomu force-pushed the introduce_field_description_factory branch from 8f3dfb1 to 11fa21f Compare February 21, 2021 00:51

VincentLanglet reviewed Feb 26, 2021

View reviewed changes

franmomu force-pushed the introduce_field_description_factory branch from 11fa21f to 75e03fa Compare February 28, 2021 21:53

VincentLanglet previously approved these changes Feb 28, 2021

View reviewed changes

VincentLanglet requested a review from a team February 28, 2021 22:45

VincentLanglet added this to the 4.0 milestone Feb 28, 2021

VincentLanglet mentioned this pull request Feb 28, 2021

[POC] Add guessTypeForFieldDescription to TypeGuesserInterface #6839

Closed

VincentLanglet requested a review from OskarStark March 3, 2021 10:21

VincentLanglet marked this pull request as ready for review March 3, 2021 10:21

VincentLanglet requested review from jordisala1991, phansys, wbloszyk, greg0ire, core23 and dmaicher March 3, 2021 10:21

core23 requested changes Mar 4, 2021

View reviewed changes

franmomu added 2 commits March 7, 2021 23:06

Introduce FieldFactoryInterface

e60ebfe

This interface will be responsible of creating FieldDescriptionInterface instances.

Introduce TypeGuesserInterface

218e433

This will guess the field description type from its properties.

franmomu dismissed VincentLanglet’s stale review via c1db7b6 March 7, 2021 22:56

franmomu force-pushed the introduce_field_description_factory branch from 75e03fa to c1db7b6 Compare March 7, 2021 22:56

franmomu added 4 commits March 8, 2021 00:12

Add AdminInterface::createFieldDescription method

5274f8f

This method will use the field description factory and also sets the current admin to the field description.

Add TypeGuesserChain for FieldDescription

b0af0a7

Deprecate TypeGuesserInterface from Guesser

cb38c97

Deprecated in favor of Sonata\AdminBundle\FieldDescription\TypeGuesserInterface.

Add FieldDescriptionOptions psalm type

e477c50

franmomu force-pushed the introduce_field_description_factory branch from c1db7b6 to e477c50 Compare March 7, 2021 23:12

VincentLanglet approved these changes Mar 7, 2021

View reviewed changes

VincentLanglet closed this Mar 8, 2021

VincentLanglet reopened this Mar 8, 2021

dmaicher requested a review from core23 March 9, 2021 19:20

core23 approved these changes Mar 9, 2021

View reviewed changes

VincentLanglet merged commit d74c43f into sonata-project:3.x Mar 10, 2021

dmaicher mentioned this pull request Mar 13, 2021

Add FieldDescriptionFactory + resolve TypeGuesser deprecations sonata-project/SonataDoctrineORMAdminBundle#1348

Closed

franmomu deleted the introduce_field_description_factory branch March 17, 2021 22:12

franmomu mentioned this pull request Mar 17, 2021

Add missing phpdoc #6947

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce field description factory #6854

Introduce field description factory #6854

franmomu commented Feb 14, 2021

OskarStark Feb 14, 2021

VincentLanglet Feb 15, 2021

core23 Mar 9, 2021

VincentLanglet left a comment

VincentLanglet commented Feb 16, 2021

VincentLanglet Feb 16, 2021

VincentLanglet Feb 16, 2021

VincentLanglet Feb 16, 2021

franmomu Feb 19, 2021

core23 Mar 4, 2021

VincentLanglet Feb 16, 2021

franmomu Feb 19, 2021

franmomu commented Feb 16, 2021

franmomu commented Feb 19, 2021

VincentLanglet left a comment

franmomu commented Feb 28, 2021

VincentLanglet left a comment

core23 Mar 4, 2021

core23 Mar 4, 2021

core23 Mar 4, 2021

core23 commented Mar 6, 2021

VincentLanglet commented Mar 7, 2021

franmomu commented Mar 7, 2021

core23 Mar 9, 2021

core23 Mar 9, 2021

Introduce field description factory #6854

Introduce field description factory #6854

Conversation

franmomu commented Feb 14, 2021

Subject

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentLanglet left a comment

Choose a reason for hiding this comment

VincentLanglet commented Feb 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franmomu commented Feb 16, 2021

franmomu commented Feb 19, 2021

VincentLanglet left a comment

Choose a reason for hiding this comment

franmomu commented Feb 28, 2021

VincentLanglet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

core23 commented Mar 6, 2021

VincentLanglet commented Mar 7, 2021

franmomu commented Mar 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment