Sonataadminbundle: Sonata and Symfony 3.3+ new service system

[soullivaneuh]

Symfony 3.3 comes with several changes on the DI system in order to simplify the configuration.

This is great, but absolutely not compatible with Sonata ATM.

I think we should take profit to simplify our admin definition too. I don't know yet if this will be BC or not.

I'll try to explain each issue, step by step.

Current statement

Take this services as example:

services:
    admin.user:
        class: AppBundle\Admin\UserAdmin
        tags:
            - { name: sonata.admin, manager_type: orm, group: User, label: User, show_mosaic_button: true }
        arguments: [ ~, AppBundle\Entity\User, 'AppBundle:Admin/User' ]
        calls:
            - [setUserManager, ['@fos_user.user_manager']]
            - [setMailer, ['@mailer.legacy']]
            - [setTokenGenerator, ['@fos_user.util.token_generator']]

    admin.ssh_key:
        class: AppBundle\Admin\SshKeyAdmin
        tags:
            - { name: sonata.admin, manager_type: orm, group: User, label: SSH Key }
        arguments: [ ~, AppBundle\Entity\SshKey, ~ ]

Most of the tag options and services arguments are generally left untouched, except for group and label.

This may be simplified like this (and make it as default on the bundle):

services:
    _defaults:
        autowire: true
        autoconfigure: true
        public: false

    _instanceof:
        Sonata\AdminBundle\Admin\AdminInterface:
            tags: [ sonata.admin ]

But many things are missing:

• The manager_type needs a default value
• The group, the label and any another tag options are not provided anymore
• The required entity argument is not provided at all.

So I start to build a compiler pass on my project to simulate autoconfiguring:

use Doctrine\Common\Inflector\Inflector;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\HttpKernel\Bundle\Bundle;

final class AdminCompilerPass implements CompilerPassInterface
{
    public function process(ContainerBuilder $container)
    {
        foreach ($container->findTaggedServiceIds('sonata.admin') as $id => $attributes) {
            $definition = $container->getDefinition($id);

            $adminClassTab = \explode('\\', $definition->getClass());
            $entityName = \preg_replace('/Admin$/', '', \end($adminClassTab));
            $definition
                ->clearTag('sonata.admin')
                ->addTag('sonata.admin', [
                    'manager_type' => 'orm',
                    'label' => Inflector::ucwords(\str_replace('_', ' ', Inflector::tableize($entityName))),
                ])
                ->setArguments([
                    null,
                    "AppBundle\\Entity\\{$entityName}",
                    null,
                ])
            ;
        }
    }
}

final class AppBundle extends Bundle
{
    public function build(ContainerBuilder $container)
    {
        parent::build($container);

        $container
            ->addCompilerPass(new AdminCompilerPass(), PassConfig::TYPE_BEFORE_OPTIMIZATION, 10)
        ;
    }
}

It works for my case even if it does not manage all the options (like the mosaic view for example), but this ran to another issue: The generated sonata role are now different:

This is "normal" because the admin code is not provided and take the service name which is now the class name.

So I tried to replace the null code argument by 'admin.'.Inflector::tableize($entityName) in order to get the old admin code and roles.

And then, this error:

Possible improvements

To simplify the service definition, we must:

• Have admin tags options defined on admin classes only. With properties and/or getter.
• Have some options/argument like the manager type, managed entity, Controllers or label guessed, based from the admin class name.

Obviously, some arguments may be still be overridden on the service definition.

Concerning the code, for now I don't get why it try to get the admin service with this.

I never used a custom admin code from now. What is the purpose of this comparing to the service name? Can this be be removed/simplified? Or maybe leave it as is and change how the sonata role are generated?

This is some ideas I got, feel free to add yours.

[greg0ire]

The purpose of the admin_code is IIRC to be able to have several admins for a same entity, and be able to specify it. I think we should simply drop it in favor of the class name. Even if FQCNs are a bit unwieldy, they are far less confusing.

[greg0ire]

Also IIRC, the admin code is used for children admin classes.

[soullivaneuh]

The purpose of the admin_code is IIRC to be able to have several admins for a same entity, and be able to specify it.

Are you sure about that?

services:
    admin.ticket:
        class: AppBundle\Admin\TicketAdmin
        arguments: [ ~, AppBundle\Entity\Ticket, 'AppBundle:Admin/Ticket' ]
        tags:
            - { name: sonata.admin, manager_type: orm, group: Support, label: Ticket }

    admin.ticket_feedback:
        class: AppBundle\Admin\TicketFeedbackAdmin
        arguments: [ ~, AppBundle\Entity\Ticket, ~ ]
        tags:
            - { name: sonata.admin, manager_type: orm, group: Support, label: Ticket Feedback }

Two different admins, same targeted entity and no provided code.

Or maybe we are not talking about the same thing.

[greg0ire]

I said: "IIRC"

[jlamur]

I never used a custom admin code from now. What is the purpose of this comparing to the service name?

There is no purpose to set an admin_code different from the service name.
It actually does not work because the pool tries to get the admin service from the container with... its admin_code.

https://github.com/sonata-project/SonataAdminBundle/blob/f3530bfd6929f3ba54e2f1396707605122ce4a73/Admin/Pool.php#L247-L254

[greg0ire]

with... its admin_code.

Did you mean "its service id"?

[jlamur]

I don't know what I meant, I'm confused. Let me try to explain again:

When you call Pool::getAdminByAdminCode($adminCode), it internally calls $this->getInstance($adminCode) which in turn calls $this->container->get($adminCode).

However Pool::getAdminByAdminCode() is called with an admin code everywhere in the admin-bundle, never with a service id.
And of course, the container won't work if you pass anything else than a service id to the get method.

That's why we can assume, IMO, that no-one using this bundle has ever overriden the admin code, or can do it.

Hope it's clearer.

[soullivaneuh]

It actually does not work because the pool tries to get the admin service from the container with... its admin_code.

Yes, this is definitely the issue. The question is: why? :smile: :thinking:

[OskarStark]

what about using our "old" service name like app.admin.foo as an alias?
does this fix the problem?

[soullivaneuh]

@OskarStark Yes, but not in static service definition.

I ended up with those two compiler passes:

final class AdminCompilerPass implements CompilerPassInterface
{
    /**
     * This can't be done on services.yml because this compiler pass is called before it.
     */
    private const ENTITY_MATCHES = [
        'TicketFeedback' => 'Ticket',
    ];

    private const CODE_MATCHES = [
        'admin.o_auth_client' => 'admin.oauth_client',
        'admin.power_d_n_s_domain' => 'admin.power_dns_domain',
        'admin.cloud_flare_zone' => 'admin.cloudflare_zone',
    ];

    /**
     * {@inheritdoc}
     */
    public function process(ContainerBuilder $container)
    {
        foreach ($container->findTaggedServiceIds('sonata.admin') as $id => $attributes) {
            $definition = $container->getDefinition($id);

            $adminClassTab = \explode('\\', $definition->getClass());
            $entityName = \preg_replace('/Admin$/', '', \end($adminClassTab));
            $label = Inflector::ucwords(\str_replace('_', ' ', Inflector::tableize($entityName)));
            $code = 'admin.'.Inflector::tableize($entityName);
            if (\array_key_exists($code, static::CODE_MATCHES)) {
                $code = static::CODE_MATCHES[$code];
            }
            if (\array_key_exists($entityName, static::ENTITY_MATCHES)) {
                $entityName = static::ENTITY_MATCHES[$entityName];
            }
            $entityFQCN = \in_array($entityName, ['PowerDNSDomain', 'PowerDNSRecord'])
                ? "PowerDNSBundle\\Entity\\{$entityName}"
                : "AppBundle\\Entity\\{$entityName}";
            $dedicatedCRUDController = "AppBundle\\Controller\\Admin\\{$entityName}Controller";
            $definition
                ->clearTag('sonata.admin')
                ->addTag('sonata.admin', [
                    'manager_type' => 'orm',
                    'label' => $label,
                ])
                ->setArguments([
                    $code,
                    $entityFQCN,
                    \class_exists($dedicatedCRUDController) ? "AppBundle:Admin/{$entityName}" : null,
                ])
            ;
        }
    }
}

final class AdminPoolCompilerPass implements CompilerPassInterface
{
    /**
     * {@inheritdoc}
     */
    public function process(ContainerBuilder $container)
    {
        $adminServicesIds = [];
        foreach ($container->findTaggedServiceIds('sonata.admin') as $id => $attributes) {
            $definition = $container->getDefinition($id);
            $code = $definition->getArgument(0);
            // Must keep an alias with service code for proper role generation.
            $container->setAlias($code, $id);
            \array_push($adminServicesIds, $id);
            \array_push($adminServicesIds, $code);
        }
        $container->getDefinition('sonata.admin.pool')
            ->removeMethodCall('setAdminServiceIds')
            ->addMethodCall('setAdminServiceIds', [$adminServicesIds])
        ;
    }
}

class AppBundle extends Bundle
{
    /**
     * {@inheritdoc}
     */
    public function build(ContainerBuilder $container)
    {
        parent::build($container);

        $container
            ->addCompilerPass(new AdminCompilerPass(), PassConfig::TYPE_BEFORE_OPTIMIZATION, 10)
            ->addCompilerPass(new AdminPoolCompilerPass())
        ;
    }
}

It's usable for my project, but I can't ensure it's compatible with all.

In any case, admin service configuration should be simplified IMHO.

[binhle410]

I suggest putting a new INterface called AutoconfiguredAdminInterface and define CONST for default and opinionated settings which could further be overridden by the actual Admin Class for customisation.

[soullivaneuh]

Please note since #4980, you will need to add this line on the foreach of AdminPoolCompilerPass::process:

$container->setAlias("{$code}.template_registry", "{$id}.template_registry");

[deguif]

I'm just seeing this discussion:

The purpose of the admin_code is IIRC to be able to have several admins for a same entity, and be able to specify it.

Yes, we can have this case when using parent/child admins services.

For example, see this service definitions sample:

 <service id="admin.parent" class="MyAdminClass" abstract="true" public="false">
    <argument />
    <argument>Entity</argument>
    <argument></argument>
</service>

<service id="admin.child_one" class="MyAdminClass" parent="admin.parent" public="true">
    <argument>xxxx</argument>
    <tag name="sonata.admin" manager_type="orm" label="XXXX" />
</service>

<service id="admin.child_two" class="MyAdminClass" parent="admin.parent" public="true">
    <argument>yyyy</argument>
    <tag name="sonata.admin" manager_type="orm" label="YYYY" />
</service>

For this scenario, the admin code is very important, as the different admin services use same class and same entity.
An extra parameter is injected to the constructor which is a discriminator, that is used to build a custom query inside the createQuery() method of the admin class.

[OskarStark]

But couldn't you use alias=admin.child_two ?

[binhle410]

IMHO, Having different definitions of the same AdminClass and same Entity is really weird and kinda dirty. A good design should not allow this kinda a 'hacking' 😀

[yann-eugone]

Hi everyone !

I was not aware that an issue was opened about this.
I've implemented something very similar on a project of mine. I extracted the relevant code in a gist : https://gist.github.com/yann-eugone/8b7414e02aedd0dc75e9e22481d68e63

Does it help ? Do you need someone to get started with that feature ?

[kunicmarko20]

I just created https://github.com/kunicmarko20/SonataAutoConfigureBundle, anyone willing to give it a try? ty @Soullivaneuh for compiler pass example, I modified it a bit.

[stale[bot]]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.