sonata-project / SonataAdminBundle

The missing Symfony Admin Generator
https://docs.sonata-project.org/projects/SonataAdminBundle
MIT License
2.11k stars 1.26k forks source link

Split TemplateRegistryInterface constants #6749

Closed franmomu closed 3 years ago

franmomu commented 3 years ago

Feature Request

Right now we have all these constants

https://github.com/sonata-project/SonataAdminBundle/blob/6b43d89be78db1b34b05ffa9b554b9c5e56debb3/src/Templating/TemplateRegistryInterface.php#L23-L67

To be honest, I've never used them, so I'm not sure what are they for. At first I thought they were meant to be used in FieldDescriptionInterface::getType, but looks like there are others usages too like types for list and show actions.

IMO we should split constants by concept, so for example move/copy the related ones to FieldDescriptionInterface and other ones to where they should belong.

Talking about FieldDescriptionInterface type property, I think we should create a "mapper" in the persistence bundles from the persistence specific field mapping type to our types, because for example ORM implementation:

https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/92e7964a8abc81efead6217626f9aba0dd757a31/src/Admin/FieldDescription.php#L34

IMO this is wrong since they could change any value and, in this case, association mapping in ORM is an int value and our getType will only admit string in 4.x.

VincentLanglet commented 3 years ago

I do use them a lot personally.

This way: $showMapper->add('foo', TemplateRegistry::TYPE_ARRAY)

$listMapper->add('bar', TemplateRegistry::TYPE_HTML)

If you don't provide a Type, the TypeGuesser is guessing the constant to use for you, and then the right template is selected.

So I only see one context here ; I dont understand why you see multiple usage/context.

franmomu commented 3 years ago

It confuses me the fact that they are in TemplateRegistryInterface, so if I have understood correctly they are always FieldDescriptionInterface types, so maybe it is better to move them to FieldDescriptionInterface?

VincentLanglet commented 3 years ago

Indeed, the builder is doing

fieldDescription->setType($type);

So it seems right to be in the FieldDescriptionInterface ; do you want to provide the PR ? :)

wbloszyk commented 3 years ago

I working on move Templating directory from AdminBundle to twig-extensions becouse template registry should be standalone feature. In new case these constants will be even more cofusing.

IMO better wil be move it to FieldTypesInterface (name can be diffrence) and implements it in FieldDescriptionInterface, ListMapper and ShowMapper to get result like these:

protected function configureListFields(ListMapper $list)
{
    $listMapper->add('bar', ListMapper::TYPE_HTML);  
}

WDYT?

VincentLanglet commented 3 years ago
protected function configureListFields(ListMapper $list)
{
    $listMapper->add('bar', ListMapper::TYPE_HTML);  
}

I don't like the fact we'll make believe that ListMapper::TYPE_HTML !== ShowMapper::TYPE_HTML, it could produce a weird behavior if someone write:

switch ($type) {
    case ListMapper::TYPE_HTML:
         // ...
         break;
    case ShowMapper::TYPE_HTML:
         // ...
         break;
    ...
}

We will always use the same name for show and list template ; so I think it's better to have these constant in only one files. FieldDescriptionInterface seems enough to me.