sonata-project / SonataPageBundle

This bundle provides a Site and Page management through container and block services
https://docs.sonata-project.org/projects/SonataPageBundle
MIT License
219 stars 209 forks source link

[Doc][Bug] It's throwing an exception when we are decorating PageAdmin #1639

Closed eerison closed 1 year ago

eerison commented 2 years ago

in version 4.x was removed those parameters

https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Resources/config/admin.xml#L7-L25

I didn't test it yet, But I suspect the only way to overwrite the classes like PageAdmin is decorating right?

I don't know! I'll do some tests and add some doc/UPGRADE notes for this!

eerison commented 2 years ago

In this case if I want to overwrite the PageAdmin I need to do like this right?

<?php

namespace App\Admin;

use Knp\Menu\ItemInterface;
use Sonata\AdminBundle\Admin\AbstractAdmin;
use Sonata\AdminBundle\Admin\AdminInterface;
use Sonata\AdminBundle\Datagrid\DatagridMapper;
use Sonata\AdminBundle\Datagrid\ListMapper;
use Sonata\AdminBundle\Form\FormMapper;
use Sonata\AdminBundle\Route\RouteCollectionInterface;
use Sonata\AdminBundle\Show\ShowMapper;

class PageAdmin extends AbstractAdmin
{
    private AdminInterface $admin;

    public function __construct(AbstractAdmin $admin)
    {
        parent::__construct();

        $this->admin = $admin;
    }

    protected function configureRoutes(RouteCollectionInterface $collection): void
    {
        $this->admin->configureRoutes($collection);
    }

    protected function preUpdate(object $object): void
    {
        $this->admin->preUpdate($object);
    }

    protected function prePersist(object $object): void
    {
        $this->admin->prePersist($object);
    }

    protected function getAccessMapping(): array
    {
        return $this->admin->getAccessMapping();
    }

    protected function configureBatchActions(array $actions): array
    {
        return $this->admin->configureBatchActions($actions);
    }

    protected function alterNewInstance(object $object): void
    {
        $this->admin->alterNewInstance($object);
    }

    protected function configurePersistentParameters(): array
    {
        return $this->admin->configurePersistentParameters();
    }

    protected function configureShowFields(ShowMapper $show): void
    {
        $this->admin->configureShowFields($show);
    }

    protected function configureListFields(ListMapper $list): void
    {
        $this->admin->configureListFields($list);
    }

    protected function configureDatagridFilters(DatagridMapper $filter): void
    {
        $this->admin->configureDatagridFilters($filter);
    }

    protected function configureFormFields(FormMapper $form): void
    {
        $this->admin->configureFormFields($form);
    }

    protected function configureTabMenu(ItemInterface $menu, string $action, ?AdminInterface $childAdmin = null): void
    {
        $this->admin->configureTabMenu($menu, $action, $childAdmin);
    }
}
App\Admin\PageAdmin:
        decorates: sonata.page.admin.page
        arguments: [ '@App\Admin\PageAdmin.inner' ]
        tags:
            - {name: 'sonata.admin', model_class: 'App\Entity\SonataPagePage', controller: 'sonata.page.controller.admin.page', manager_type: 'orm', group: 'sonata_page', translation_domain: 'SonataPageBundle', label: 'page', label_translator_strategy: 'sonata.admin.label.strategy.underscore', icon: 'fa fa-sitemap' }
eerison commented 2 years ago

When I try to decorate the admin as I did above, I got this exception :'(

InvalidArgumentException:
Template named "tree" doesn't exist.

  at vendor/sonata-project/admin-bundle/src/Templating/AbstractTemplateRegistry.php:47
  at Sonata\AdminBundle\Templating\AbstractTemplateRegistry->getTemplate('tree')
     (vendor/sonata-project/page-bundle/src/Controller/PageAdminController.php:122)
  at Sonata\PageBundle\Controller\PageAdminController->treeAction(object(Request))
     (vendor/symfony/http-kernel/HttpKernel.php:153)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw(object(Request), 1)
     (vendor/symfony/http-kernel/HttpKernel.php:75)
  at Symfony\Component\HttpKernel\HttpKernel->handle(object(Request), 1, true)
     (vendor/symfony/http-kernel/Kernel.php:202)
  at Symfony\Component\HttpKernel\Kernel->handle(object(Request))
     (public/index.php:25) 
VincentLanglet commented 2 years ago

When I try to decorate the admin as I did above, I got this exception :'(

InvalidArgumentException:
Template named "tree" doesn't exist.

  at vendor/sonata-project/admin-bundle/src/Templating/AbstractTemplateRegistry.php:47
  at Sonata\AdminBundle\Templating\AbstractTemplateRegistry->getTemplate('tree')
     (vendor/sonata-project/page-bundle/src/Controller/PageAdminController.php:122)
  at Sonata\PageBundle\Controller\PageAdminController->treeAction(object(Request))
     (vendor/symfony/http-kernel/HttpKernel.php:153)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw(object(Request), 1)
     (vendor/symfony/http-kernel/HttpKernel.php:75)
  at Symfony\Component\HttpKernel\HttpKernel->handle(object(Request), 1, true)
     (vendor/symfony/http-kernel/Kernel.php:202)
  at Symfony\Component\HttpKernel\Kernel->handle(object(Request))
     (public/index.php:25) 

It's because the template is automatically injected for the admin https://github.com/sonata-project/SonataPageBundle/blob/4.x/src/DependencyInjection/SonataPageExtension.php#L225

would be interesting to see if it's possible to do the same for decorated services.

eerison commented 2 years ago

hmmmm ok, I'll check :D

eerison commented 2 years ago

Hey @VincentLanglet is there a other solution But I don't know if it's ok

App\Admin\PageAdmin:
        decorates: sonata.page.admin.page
        arguments:
            - '@App\Admin\PageAdmin.inner'
+        calls:
+            - setTemplate: ['tree', '@@SonataPage/PageAdmin/tree.html.twig']
        tags:
            - {name: 'sonata.admin', model_class: 'App\Entity\SonataPagePage', controller: 'sonata.page.controller.admin.page', manager_type: 'orm', group: 'sonata_page', translation_domain: 'SonataPageBundle', label: 'page', label_translator_strategy: 'sonata.admin.label.strategy.underscore', icon: 'fa fa-sitemap' }

I found others PR related with it, using a new tag

https://github.com/sonata-project/SonataAdminBundle/pull/6566 https://github.com/sonata-project/SonataAdminBundle/pull/6766

But it's not really clean how I can use it :/

I tested something like this

tags:
    - {name: sonata.template_registry, type: show, template:  '@@FooAdmin/CRUD/show.html.twig'}

But it doesn't work, do you have any idea how could I use it?

VincentLanglet commented 2 years ago

Hey @VincentLanglet is there a other solution But I don't know if it's ok

Sure it can be done with the configuration, but you shouldn't have to know every templates to add in the configuration. Especially one day if you want to change the tree template, you'll have to update the sonata page config and all of your admins.

I would expect something from the config here https://github.com/sonata-project/SonataPageBundle/blob/4.x/src/DependencyInjection/SonataPageExtension.php#L223-L230 to do the job.

$container->getDefinition('sonata.page.admin.page'); is done, I dunno if we can also get the definition of all the service which decorate this one in order to add the setTemplate call too.

eerison commented 2 years ago

but you shouldn't have to know every templates to add in the configuration.

That's true

eerison commented 2 years ago

I know it's a huge change to do in 4.x, But what do you think we split the admin in small piece of code?

for example, we extract the methods from the PageAdmin to small classes/interfaces and inject it in constructor like

https://github.com/sonata-project/SonataPageBundle/blob/4.x/src/Admin/PageAdmin.php

final class PageAdmin extends AbstractAdmin
{
    public function __construct(private AdminListFields $adminListFields) {}

    ....

    protected function configureListFields(ListMapper $list): void
    {
        $this->adminListFields-> configureListFields($list);
    }
}
interface AdminListFields
{
    public function configureListFields(ListMapper $list): void;
}

final class PageAdminListFields implements AdminListFields
{
    public function configureListFields(ListMapper $list): void
    {
        $list
            ->add('hybrid', null, ['template' => '@SonataPage/PageAdmin/field_hybrid.html.twig'])
            ->addIdentifier('name')
            ->add('type')
            ->add('pageAlias')
            ->add('site', null, [
                'sortable' => 'site.name',
            ])
            ->add('decorate', null, ['editable' => true])
            ->add('enabled', null, ['editable' => true])
            ->add('edited', null, ['editable' => true])
            ->add(ListMapper::NAME_ACTIONS, ListMapper::TYPE_ACTIONS, [
                'translation_domain' => 'SonataAdminBundle',
                'actions' => [
                    'edit' => [],
                ],
            ]);
    }
}

This way you just need to decorate/customize what is really necessary, and you don't need to implements too many methods that you don't want to touch like in the current solution: https://github.com/sonata-project/SonataPageBundle/issues/1639#issuecomment-1299889832

in this case I will decorate like this


#[AsDecorator(decorates: PageAdminListFields::class)]
class YourCustomPageAdminListFields implements AdminListFields
{
    public function __construct(#[MapDecorated] private AdminListFields $currentPageAdminListFields) {}

    public function configureListFields(ListMapper $list): void
    {
        $this->currentPageAdminListFields-> configureListFields($list);
        $list->add('YourCustomField');
    }
}

https://symfony.com/doc/current/service_container/service_decoration.html

@VincentLanglet @jordisala1991

eerison commented 2 years ago

I know it's a huge change to do in 4.x, But what do you think we split the admin in small piece of code?

for example, we extract the methods from the PageAdmin to small classes/interfaces and inject it in constructor like

https://github.com/sonata-project/SonataPageBundle/blob/4.x/src/Admin/PageAdmin.php

final class PageAdmin extends AbstractAdmin
{
    public function __construct(private AdminListFields $adminListFields) {}

    ....

    protected function configureListFields(ListMapper $list): void
    {
        $this->adminListFields-> configureListFields($list);
    }
}
interface AdminListFields
{
    public function configureListFields(ListMapper $list): void;
}

final class PageAdminListFields implements AdminListFields
{
    public function configureListFields(ListMapper $list): void
    {
        $list
            ->add('hybrid', null, ['template' => '@SonataPage/PageAdmin/field_hybrid.html.twig'])
            ->addIdentifier('name')
            ->add('type')
            ->add('pageAlias')
            ->add('site', null, [
                'sortable' => 'site.name',
            ])
            ->add('decorate', null, ['editable' => true])
            ->add('enabled', null, ['editable' => true])
            ->add('edited', null, ['editable' => true])
            ->add(ListMapper::NAME_ACTIONS, ListMapper::TYPE_ACTIONS, [
                'translation_domain' => 'SonataAdminBundle',
                'actions' => [
                    'edit' => [],
                ],
            ]);
    }
}

This way you just need to decorate/customize what is really necessary, and you don't need to implements too many methods that you don't want to touch like in the current solution: #1639 (comment)

in this case I will decorate like this

#[AsDecorator(decorates: PageAdminListFields::class)]
class YourCustomPageAdminListFields implements AdminListFields
{
    public function __construct(#[MapDecorated] private AdminListFields $currentPageAdminListFields) {}

    public function configureListFields(ListMapper $list): void
    {
        $this->currentPageAdminListFields-> configureListFields($list);
        $list->add('YourCustomField');
    }
}

https://symfony.com/doc/current/service_container/service_decoration.html

@VincentLanglet @jordisala1991

Extension works better then my suggestion :D

I'll test that :)

eerison commented 2 years ago

Using extension works!

<?php

namespace App\Admin\Extension;

use Sonata\AdminBundle\Admin\AbstractAdminExtension;
use Sonata\AdminBundle\Datagrid\ListMapper;

class PageAdminExtension extends AbstractAdminExtension
{
    public function configureListFields(ListMapper $list): void
    {
        $list->add('id');
    }
}

But it added the fields in the end of the list like this

Screenshot 2022-11-07 at 09 10 29

are there anyway to put the field in the begin of table

VincentLanglet commented 2 years ago

are there anyway to put the field in the begin of table

You can play with the reorder method

eerison commented 2 years ago

I didn't find anything in sonata docs about reorder method 👀

VincentLanglet commented 2 years ago

https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Datagrid/ListMapper.php#L193

Might be interesting to add docs explaining how to use it then