Improve "sonata.admin.template_registry" tag #6566

wbloszyk · 2020-11-03T17:22:02Z

Subject

After move templates from AbsractAdmin to TemplateRegistry some methods were deprecated (like setTemplate and setTemplates). To allow configuration templates in easy way like it was before some important change are require.

Additional attributes on tag

Usages

with `TemplateRegistryInterface`

You MUST set templates by argument but you can override it by tag.

with `MutableTemplateRegistryInterface`

You CAN set templates by argument, override it by call setTemplate or setTemplates method and than override it again by tag.

with `TemplateRegistryAwareInterface`

You CAN NOT use tag when setTemplateRegistry is called. You should override template directly on this template registry then.
You CAN use tag when setTemplateRegistry is not called. In this case TemplateRegistry will be auto generate with id equal to tagged_service_id + .template_registry.

with `MutableTemplateRegistryAwareInterface`

You CAN use tag when setTemplateRegistry is called but it is not recommended.
You CAN use tag when setTemplateRegistry is not called. In this case MutableTemplateRegistry will be auto generate with id equal to tagged_service_id + .template_registry.

Examples

Sonata\AdminBundle\Tests\App\Admin\FooAdmin:
    arguments: [~, Sonata\AdminBundle\Tests\App\Model\Foo, ~]
    tags:
        - {name: sonata.admin, manager_type: test, label: Foo}
        - {name: sonata.admin.template_registry, template_name:'edit', template_path: '@FooAdmin/CRUD/edit.html.twig'}
        - {name: sonata.admin.template_registry, template_name:'list', template_path: '@FooAdmin/CRUD/list.html.twig'}

I am targeting this branch, because this change respect BC and should be done in 3.x.

Changelog

### Added
- Added `Sonata\AdminBundle\DependencyInjection\Compiler\TemplateRegistryCompilerPass`

To do

Update the tests;
Update the documentation;
Update the documentation - new tags description;
Fix problem with order template override

tests/App/config/services.yml

VincentLanglet · 2020-11-04T22:45:01Z

Sonata\AdminBundle\Tests\App\Admin\FooAdmin:
    arguments: [~, Sonata\AdminBundle\Tests\App\Model\Foo, ~]
    tags:
        - {name: sonata.admin, manager_type: test, label: Foo}
        - {name: sonata.template_registry, virtual_templates_method: 'my_custom_templates' }
        - {name: sonata.template_registry, virtual_templates_method: 'other_custom_templates' }
    calls:
        - my_custom_templates: [[
                show: '@@FooAdmin/CRUD/show.html.twig',
                edit: '@@FooAdmin/CRUD/show.html.edit'
            ]]
        - other_custom_template: [[edit: '@@FooAdmin/CRUD/other_show.html.edit']]

Since array are not allowed inside tags, WDYT about this solution ?

Sonata\AdminBundle\Tests\App\Admin\FooAdmin:
    arguments: [~, Sonata\AdminBundle\Tests\App\Model\Foo, ~]
    tags:
        - {name: sonata.admin, manager_type: test, label: Foo}
        - {name: sonata.template_registry, type: show, template:  '@@FooAdmin/CRUD/show.html.twig'}
        - {name: sonata.template_registry, type: edit, template:  '@@FooAdmin/CRUD/edit.html.twig'}

If this is confusing with

admin.foo.template_registry:
   class: Sonata\AdminBundle\Templating\MutableTemplatingRegistry
   tags: 
       - [name: "sonata.template_registry"]
   calls:
       - [setTemplate, ['show', 'admin/foo/show.html.twig']]
       - [setTemplate, ['edit', 'admin/foo/edit.html.twig']]

We maybe could have two tag, like sonata.template_registry and sonata.admin_template_registry

wbloszyk · 2020-11-05T08:16:22Z

I think about type => template too.

IMO we should add sonata.template_registry_aware which will be create template registry not only for admin but for each aware interface implementing - also this by people. It is good solutions becouse we do not need this functionality in sonata.template_registry tag.

VincentLanglet · 2020-11-05T08:39:24Z

I think about type => template too.

IMO we should add sonata.template_registry_aware which will be create template registry not only for admin but for each aware interface implementing - also this by people. It is good solutions becouse we do not need this functionality in sonata.template_registry tag.

Nice
Can you make a try with type/template and sonata.template_registry_aware ? 👍

src/DependencyInjection/Compiler/TemplateRegistryCompilerPass.php

phansys · 2020-11-06T09:15:44Z

I just have a consideration about the proposed service sonata.template_registry_aware, since this name does not accomplish with the standard.
IMO, it should be sonata_admin.template_registry_aware.
For the tags, I'd change sonata.template_registry with sonata_admin.template_registry.

wbloszyk · 2020-11-06T09:40:52Z

I just have a consideration about the proposed service sonata.template_registry_aware, since this name does not accomplish with the standard.
IMO, it should be sonata_admin.template_registry_aware.
For the tags, I'd change sonata.template_registry with sonata_admin.template_registry.

Current we have tags like: sonata.admin, sonata.block, sonata.page. IMO we should add sonata.template_registry_aware and consider change all tags in sonata 4.

WDYT? @sonata-project/contributors

VincentLanglet · 2020-11-06T09:57:50Z

I just have a consideration about the proposed service sonata.template_registry_aware, since this name does not accomplish with the standard.
IMO, it should be sonata_admin.template_registry_aware.
For the tags, I'd change sonata.template_registry with sonata_admin.template_registry.

Current we have tags like: sonata.admin, sonata.block, sonata.page. IMO we should add sonata.template_registry_aware and consider change all tags in sonata 4.

WDYT? @sonata-project/contributors

Indeed. Consistency is the key.

phansys · 2020-11-06T10:19:39Z

I think the tags prefixed with sonata.admin. are intended to be interpreted by SonataAdminBundle, the tags prefixed with sonata.block. are intended to be interpreted by SonataBlockBundle, and so.
I see no problem in using sonata.admin.template_registry as tag name for this case.
The main concern I think must be considered is about the service name.

IMO, it should be sonata_admin.template_registry_aware.

VincentLanglet · 2020-11-06T11:55:34Z

I think the tags prefixed with sonata.admin. are intended to be interpreted by SonataAdminBundle, the tags prefixed with sonata.block. are intended to be interpreted by SonataBlockBundle, and so.
I see no problem in using sonata.admin.template_registry as tag name for this case.
The main concern I think must be considered is about the service name.

IMO, it should be sonata_admin.template_registry_aware.

Why not using sonata.admin.template_registry and sonata.admin.template_registry_aware then ?

phansys · 2020-11-06T14:05:11Z

I'm sorry. Didn't noticed before that our services are not following the standard.
I agree that currently, our main goal must be the consistency.

tests/App/config/services.yml

VincentLanglet

Upgrade note / Doc updates will be required. But this seems a great solution.

VincentLanglet · 2020-11-07T21:16:15Z

tests/App/AppKernel.php

@@ -102,6 +102,8 @@ protected function configureContainer(ContainerBuilder $containerBuilder, Loader
            'strict_variables' => '%kernel.debug%',
            'exception_controller' => null,
            'form_themes' => ['@SonataAdmin/Form/form_admin_fields.html.twig'],
+            'paths' => ['%kernel.project_dir%/Resources/views'],
+


You can remove this extra line

WIP. I will probably need it in functional tests. Set templates must exists.

I was talking about the blank line

src/DependencyInjection/Compiler/TemplateRegistryCompilerPass.php

core23 · 2020-11-08T13:09:39Z

src/DependencyInjection/Compiler/TemplateRegistryCompilerPass.php

+{
+    const TEMPLATE_REGISTRY_TAG = 'sonata.admin.template_registry';
+    const MUTABLE_TEMPLATE_REGISTRY_TAG = 'sonata.admin.template_registry';
+    const TEMPLATE_REGISTRY_AWARE_TAG = 'sonata.admin.template_registry_aware';


Both tags sound very technical / related to the concrete implementation. Maybe just sonata.admin.template?

They look like related to the concreate implementation but thye have diffrence functionality too.
"Nonmutable" tags sets template by constructor argument.
"Mutable" tags set template by setTemplate method.
"Nonaware" tags are for mark template registry service.
"Aware" tags will auto generate template registry.

I will think about it.

VincentLanglet · 2020-11-14T20:03:23Z

Hi @wbloszyk, what's missing to finish the PR ?
This is really a need currently since we've introduced deprecation for setTemplate method.

CC @sonata-project/contributors, any review is good to take.

phansys · 2020-11-14T21:29:10Z

src/DependencyInjection/Compiler/TemplateRegistryCompilerPass.php

+            $templates = [];
+
+            foreach ($tags as $attributes) {
+                if (isset($attributes['type'], $attributes['template'])) {


IMO, the name "type" for this option is too restrictive. Shouldn't this be "template_name"?
My concern is about scenarios where the passed template could be considered something more general than the "type" concept.

template_name seems better indeed.

sonata.admin.template_registry.template tag with name and value arguments.

Isn't name reserved ?

I mean, you currently used

- {name: sonata.admin.mutable_template_registry_aware, type:'edit', template: '@@FooAdmin/CRUD/edit.html.twig'}

So how could we use name instead of type since name is already used ?

My bad. template_name and template_value to be consider. I will finish it in friday evening or in saturday.

I'd use other name than template_value. Maybe template_path or just template.

template_path will be the best.

src/DependencyInjection/Compiler/TemplateRegistryCompilerPass.php

phansys · 2020-11-14T21:44:06Z

src/DependencyInjection/Compiler/TemplateRegistryCompilerPass.php

+                $templateRegistryDefinition = $container->getDefinition($reference->__toString());
+
+                if (!is_a($templateRegistryDefinition->getClass(), $templateRegistryInterface, true)) {
+                    throw new \Exception(sprintf(


IMO, this exception could use a less generic type (something like \InvalidArgumentException, \BadMethodCallException or similar.
I don't know if we have domain specific exceptions available under the Symfony\Component\DependencyInjection\ NS for cases like this.

tests/DependencyInjection/Compiler/TemplateRegistryCompilerPassTest.php

kirya-dev · 2020-11-25T20:33:41Z

What do you think about make simplest template configuration?

SonataCI · 2020-12-23T18:13:15Z

Could you please rebase your PR and fix merge conflicts?

VincentLanglet · 2020-12-23T20:01:47Z

@wbloszyk the changelog should be updated

SonataCI · 2020-12-24T15:33:16Z

Could you please rebase your PR and fix merge conflicts?

VincentLanglet · 2020-12-31T12:25:39Z

Is it ok for you now @phansys @core23 ? :)

SonataCI · 2021-01-02T15:43:13Z

Could you please rebase your PR and fix merge conflicts?

VincentLanglet · 2021-01-03T16:59:20Z

docs/reference/tags.rst

+Tags
+======
+
+sonata.admin.template_registry


I don't fully understand this doc.

In the first case you defined a app.admin.post.template_registry but you never use it.
If it's not well documented, it will never be used.

Instead of a technical documentation about the tag, it seems better to have a functional documentation about the feature related: how to override an admin template.

First case, directly in the configuration: thanks to the sonata.admin.template_registry tag.

Second case, with a re-usable template registry (and how to do this).

In the first case you do not have to use template registry with admin. This service can be standalone registry. Service name is confusing, I will change it.

IMO template registry should be move to sonata-project/twig-extensions with this tags.rst file and documentation dedicated for override admin template should be add.

WDYT? @sonata-project/contributors

I still don't understand how to use a template registry, so I cannot say if it's better in sonataAdmin or twigExtensions.

But currently, it's the only blocking point for Sonata 4.0.
So if we can't solve the situation, we should just un-deprecate setTemplate, remove templateRegistry and delay this feature to the next major.

core23 · 2021-01-05T18:04:22Z

UPGRADE-3.x.md

+        arguments: [~, Sonata\AdminBundle\Tests\App\Model\Foo, ~]
+        tags:
+            - {name: sonata.admin, manager_type: test, label: Foo}
+            - {name: sonata.admin.template_registry, template_name:'edit', template_path: '@FooAdmin/CRUD/edit.html.twig'}


Suggested change

- {name: sonata.admin.template_registry, template_name:'edit', template_path: '@FooAdmin/CRUD/edit.html.twig'}

- {name: sonata.admin.template_registry, template_name: 'edit', template_path: '@FooAdmin/CRUD/edit.html.twig'}

core23 · 2021-01-05T18:04:33Z

UPGRADE-3.x.md

+        tags:
+            - {name: sonata.admin, manager_type: test, label: Foo}
+            - {name: sonata.admin.template_registry, template_name:'edit', template_path: '@FooAdmin/CRUD/edit.html.twig'}
+            - {name: sonata.admin.template_registry, template_name:'list', template_path: '@FooAdmin/CRUD/list.html.twig'}


Suggested change

- {name: sonata.admin.template_registry, template_name:'list', template_path: '@FooAdmin/CRUD/list.html.twig'}

- {name: sonata.admin.template_registry, template_name: 'list', template_path: '@FooAdmin/CRUD/list.html.twig'}

In yaml need use double @

wbloszyk · 2021-01-11T21:08:36Z

Close in favor of #6766

VincentLanglet added docs Needs: Documentation and removed docs labels Nov 3, 2020

wbloszyk force-pushed the template_registry_tag_improve branch from eba3dfd to 05b8c2c Compare November 4, 2020 14:03

wbloszyk mentioned this pull request Nov 4, 2020

Update docs in order to avoid calling deprecated methods setTemplate() and setTemplates() in AbstractAdmin #6538

Closed

FlintCIBot reviewed Nov 4, 2020

View reviewed changes

tests/App/config/services.yml Outdated Show resolved Hide resolved

FlintCIBot reviewed Nov 4, 2020

View reviewed changes

tests/App/config/services.yml Outdated Show resolved Hide resolved

core23 requested changes Nov 5, 2020

View reviewed changes

src/DependencyInjection/Compiler/TemplateRegistryCompilerPass.php Show resolved Hide resolved

FlintCIBot reviewed Nov 7, 2020

View reviewed changes

tests/App/config/services.yml Outdated Show resolved Hide resolved

FlintCIBot reviewed Nov 7, 2020

View reviewed changes

tests/App/config/services.yml Outdated Show resolved Hide resolved

FlintCIBot reviewed Nov 7, 2020

View reviewed changes

tests/App/config/services.yml Outdated Show resolved Hide resolved

wbloszyk changed the title ~~Improve sonata.template_registry tag~~ Template registry tags Nov 7, 2020

VincentLanglet reviewed Nov 7, 2020

View reviewed changes

core23 reviewed Nov 8, 2020

View reviewed changes

VincentLanglet added the minor label Nov 14, 2020

VincentLanglet added this to the 4.0 milestone Nov 14, 2020

phansys reviewed Nov 14, 2020

View reviewed changes

phansys requested changes Nov 14, 2020

View reviewed changes

wbloszyk force-pushed the template_registry_tag_improve branch from fc8cda9 to d4324c2 Compare November 21, 2020 17:52

SonataCI added the pending author label Dec 23, 2020

wbloszyk force-pushed the template_registry_tag_improve branch from c2cc237 to 959d7c2 Compare December 23, 2020 18:32

SonataCI removed the pending author label Dec 23, 2020

VincentLanglet removed the Needs: Documentation label Dec 23, 2020

wbloszyk force-pushed the template_registry_tag_improve branch from 351717e to cff51fb Compare December 23, 2020 20:11

SonataCI added the pending author label Dec 24, 2020

This was referenced Dec 24, 2020

Allow create admin template registry by user #6561

Closed

Add trigger_error and fix some comments #6393

Closed

Add TemplateRegistryCompilerPass

de31fc6

wbloszyk force-pushed the template_registry_tag_improve branch from cff51fb to de31fc6 Compare December 29, 2020 08:50

SonataCI removed the pending author label Dec 29, 2020

VincentLanglet approved these changes Dec 29, 2020

View reviewed changes

VincentLanglet requested review from a team, franmomu and jordisala1991 December 29, 2020 10:11

wbloszyk mentioned this pull request Dec 31, 2020

Changing TemplateRegistry service does not change TemplateRegistry in Admin classes #5522

Closed

SonataCI added the pending author label Jan 2, 2021

VincentLanglet requested changes Jan 3, 2021

View reviewed changes

VincentLanglet mentioned this pull request Jan 3, 2021

Admin supports ServiceSubscriberInterface and configure admin directely in file #6551

Closed

SonataCI removed the pending author label Jan 5, 2021

core23 reviewed Jan 5, 2021

View reviewed changes

wbloszyk closed this Jan 11, 2021

eerison mentioned this pull request Nov 2, 2022

[Doc][Bug] It's throwing an exception when we are decorating PageAdmin sonata-project/SonataPageBundle#1639

Closed

	- {name: sonata.admin.template_registry, template_name:'edit', template_path: '@FooAdmin/CRUD/edit.html.twig'}
	- {name: sonata.admin.template_registry, template_name: 'edit', template_path: '@FooAdmin/CRUD/edit.html.twig'}

Improve "sonata.admin.template_registry" tag #6566

Improve "sonata.admin.template_registry" tag #6566

Conversation

wbloszyk commented Nov 3, 2020 • edited Loading

Subject

Usages

with TemplateRegistryInterface

with MutableTemplateRegistryInterface

with TemplateRegistryAwareInterface

with MutableTemplateRegistryAwareInterface

Examples

Changelog

To do

VincentLanglet commented Nov 4, 2020

wbloszyk commented Nov 5, 2020

VincentLanglet commented Nov 5, 2020

phansys commented Nov 6, 2020

wbloszyk commented Nov 6, 2020

VincentLanglet commented Nov 6, 2020

phansys commented Nov 6, 2020

VincentLanglet commented Nov 6, 2020

phansys commented Nov 6, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentLanglet commented Nov 14, 2020

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

Choose a reason for hiding this comment

kirya-dev commented Nov 25, 2020

SonataCI commented Dec 23, 2020

VincentLanglet commented Dec 23, 2020

SonataCI commented Dec 24, 2020

VincentLanglet commented Dec 31, 2020

SonataCI commented Jan 2, 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

wbloszyk commented Jan 11, 2021

wbloszyk commented Nov 3, 2020 •

edited

Loading

with `TemplateRegistryInterface`

with `MutableTemplateRegistryInterface`

with `TemplateRegistryAwareInterface`

with `MutableTemplateRegistryAwareInterface`