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

Problem with an admin class without entity and extension set up in `sonata_admin.yaml` #6189

Closed rfaivre closed 4 years ago

rfaivre commented 4 years ago

Environment

Symfony 4.4.8 with php 7.2

Sonata packages

SonataAdminBundle: 3.66

$ composer show --latest 'sonata-project/*'
sonata-project/admin-bundle              3.66.0 3.71.1 The missing Symfony Admin Generator
sonata-project/block-bundle              3.18.4 3.20.0 Symfony SonataBlockBundle
sonata-project/cache                     2.0.1  2.0.1  Cache library
sonata-project/core-bundle               3.18.0 3.20.0 Symfony SonataCoreBundle (abandoned)
sonata-project/datagrid-bundle           2.5.0  3.2.0  Symfony SonataDatagridBundle
sonata-project/doctrine-extensions       1.6.0  1.7.0  Doctrine2 behavioral extensions
sonata-project/doctrine-orm-admin-bundle 3.17.1 3.20.0 Symfony Sonata / Integrate Doctrine ORM into th...
sonata-project/easy-extends-bundle       2.5.0  2.5.0  Symfony SonataEasyExtendsBundle
sonata-project/exporter                  2.2.0  2.2.0  Lightweight Exporter library
sonata-project/formatter-bundle          4.1.3  4.2.0  Symfony SonataFormatterBundle
sonata-project/intl-bundle               2.7.0  2.7.0  Symfony SonataIntlBundle
sonata-project/translation-bundle        2.5.0  2.5.0  SonataTranslationBundle
sonata-project/user-bundle               4.5.2  4.6.0  Symfony SonataUserBundle

Symfony packages

$ composer show --latest 'symfony/*'
symfony/apache-pack                v1.0.1  v1.0.1  A pack for Apache support in Symfony
symfony/asset                      v4.4.8  v4.4.10 Symfony Asset Component
symfony/browser-kit                v4.4.8  v4.4.10 Symfony BrowserKit Component
symfony/cache                      v4.4.8  v4.4.10 Symfony Cache component with PSR-6, PSR-16, and tags
symfony/cache-contracts            v2.0.1  v2.1.3  Generic abstractions related to caching
symfony/config                     v4.4.8  v4.4.10 Symfony Config Component
symfony/console                    v4.4.8  v4.4.10 Symfony Console Component
symfony/css-selector               v4.4.8  v4.4.10 Symfony CssSelector Component
symfony/debug                      v4.4.8  v4.4.10 Symfony Debug Component
symfony/dependency-injection       v4.4.8  v4.4.10 Symfony DependencyInjection Component
symfony/doctrine-bridge            v4.4.8  v4.4.10 Symfony Doctrine Bridge
symfony/dom-crawler                v4.4.8  v4.4.10 Symfony DomCrawler Component
symfony/dotenv                     v4.4.8  v4.4.10 Registers environment variables from a .env file
symfony/error-handler              v4.4.8  v4.4.10 Symfony ErrorHandler Component
symfony/event-dispatcher           v4.4.9  v4.4.10 Symfony EventDispatcher Component
symfony/event-dispatcher-contracts v1.1.7  v2.1.3  Generic abstractions related to dispatching event
symfony/expression-language        v4.4.8  v4.4.10 Symfony ExpressionLanguage Component
symfony/filesystem                 v4.4.8  v4.4.10 Symfony Filesystem Component
symfony/finder                     v4.4.8  v4.4.10 Symfony Finder Component
symfony/flex                       v1.6.2  v1.8.4  Composer plugin for Symfony
symfony/form                       v4.4.8  v4.4.10 Symfony Form Component

PHP version

$ php -v
PHP 7.2.31-1+0~20200514.41+debian9~1.gbpe2a56b (cli) (built: May 14 2020 08:33:26) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies
    with Zend OPcache v7.2.31-1+0~20200514.41+debian9~1.gbpe2a56b, Copyright (c) 1999-2018, by Zend Technologies
    with Xdebug v2.9.6, Copyright (c) 2002-2020, by Derick Rethans

Subject

I created many admin classes with and without entity in my project. Until then, no worries, it works well. I want to create an extension and use the configuration in sonata_admin.yaml with, for example, implements as mentioned in the documentation Like that, my extension will be added to every admin with the entity implementing this interface filled out in the configuration.

But in the extensionCompilerPass, when it loops on every admin class with tags sonata.admin, and check if there are some extensions for admin in getExtensionsForAdmin functions, getManagedClass return null for my admin without an entity (it's correct), but passing null to class_exists provokes an error

The proposed fixed below is straightforward: Add a check if the return is not null in addition to check if the class exists.

$class = $this->getManagedClass($admin, $container);
if (!$class || !class_exists($class)) {
  continue;
}

Let me know if I'm right or if I forgot a part in documentation ;)

Then I will provide a pull request if the bug is confirmed ;)

VincentLanglet commented 4 years ago

What's your use case for admin class without entity ?

The documentation ask for a model: https://sonata-project.org/bundles/admin/master/doc/getting_started/creating_an_admin.html

And we're currently throwing a deprecation if you pass a null class.

if (!\is_string($class)) {
    @trigger_error(sprintf(
        'Passing other type than string as argument 2 for method %s() is deprecated since'
        .' sonata-project/admin-bundle 3.65. It will accept only string in version 4.0.',
            __METHOD__
      ), E_USER_DEPRECATED);
}

Plus, the AdminInterface::getClass() have to return a string. I'm surprised everything works for you cause this function is used almost 100 times in the project, and every methods are using the result as a string.

If you want to support admin without class you'll need

rfaivre commented 4 years ago

Read this part of the documentation: https://sonata-project.org/bundles/admin/master/doc/cookbook/recipe_custom_view.html As mentioned in the documentation, providing an entity class is not mandatory for an admin.

There is a lot of use cases. For example, create a specific view to display stats, amongst others, and add it to the sidebar with the rest of my admin pages This page is not related to a specific entity and the logic is specific and I never had a problem with that

Hum, ok. Then how will be managed admin without entity in version 4.0? If providing an entity is mandatory. BTW, @trigger_error does not throw an exception, but only a deprecation message.

But my problem is not here. As explained above, I created an extension and I want this to be added to all my entities implementing a specific class. In the ExtensionCompilerPass file, it loops on every admin and according to the configuration defined in sonata_admin.yaml (https://symfony.com/doc/master/bundles/SonataAdminBundle/reference/extensions.html) add the extension to the entity if it matches.

My problem is only related to add an extension to a specific entity and I have some admin without entity.

Of course, I have a workaround (but I find it's not very reusable), if my PR is rejected by adding my extension like that:

app.example.extension:
    class: App\Admin\Extension\ExampleExtension
    tags:
        - { name: sonata.admin.extension, target: admin.product }
        - { name: sonata.admin.extension, target: admin.question }

But with this fix, you can use your configuration centralized in the sonata_admin.yaml

sonata_admin:
    extensions:
        app.example.extension:
            implements:
                - App\Interface\ExampleInterface

With my entities implementing App\Interface\ExampleInterface and that's all.

It's more reusable, and if I want to use the extension for another entity, I just have to implement this interface and everything is done.

The current development works well if you don't have a specific custom view without entity.

Look at my PR maybe it's more clear ;)

VincentLanglet commented 4 years ago

BTW, @trigger_error does not throw an exception, but only a deprecation message.

I know, we're adding deprecation to be BC. But it means, it will throw an exception in next major.

Hum, ok. Then how will be managed admin without entity in version 4.0? If providing an entity is mandatory.

Looks like admin without entity won't be supported.

Even if there is a documentation, the phpdoc was asking for an entity: https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Admin/AbstractAdmin.php#L594 Then, every methods using AdminInterface::getClass() as argument were expecting a @param string $class. And exception are like "something about instance $class". This either doesn't works, either make no sens for null/''.

I also found some comment

/* NEXT_MAJOR: remove cast to string, null is not supposed to be supported but was documented as such */

And for example, how do you want this code to work with a null entity ?

public function getNewInstance()
{
   $object = $this->getModelManager()->getModelInstance($this->getClass());
   $this->appendParentObject($object);

    foreach ($this->getExtensions() as $extension) {
       $extension->alterNewInstance($this, $object);
    }

    return $object;
 }

IMHO, even with the documentation, I still have trouble to understand the goal of an admin without entity. An admin is made to list entities, show an entity, edit an entity or create one. If you want a custom view, I will recommend to create a custom route and extends the admin_layout.

Maybe it worked for you, since you were using only a few of Sonata features, but you're already discovering that it doesn't work with everything (Like your bug with admin extension). Your PR is theorically correct, if we want to support admin without entity. But the phpdoc of the Sonata code is not really promoting this... And they're will be a lot of bug with this feature since the developer won't care about the possibly null value, since it's not documented in the code.

If we want to support admin without entity, we'll need to start to update all the phpdoc of the project from string to string|null and then to update a lot of code too. But currently I'm really sorry, but I don't see much interest in this.

@sonata-project/contributors If you agree with me, I think this documentation should be removed.

@rfaivre If you think I'm wrong, I'll be happy to hear how/why.

rfaivre commented 4 years ago

I understand your point of view. Because working with an admin without entity doesn't use all the power and possibility of an admin, and his main goal is to work with entity. I'm ok with this. And according to documentation, passing null seems to be not supported any more in the next versions.

But in my case, I want to use an admin without an entity to keep some features added by an admin like adding it to the sidebar with the rest of my admin page, amongst others features.

And about your proposed solution to create a custom view by extending the admin_layout, why not. But it's not only for rendering, there is some logic behind not related to only one entity.

So then if sonata contributors agree with you, we can cancel the pull request without a problem. But of course, documentation needs to be updated ;)

VincentLanglet commented 4 years ago

And according to documentation, passing null seems to be not supported any more in the next versions.

Indeed.

But I can also find some (string) $this->getClass() as if someone wanted to fix the case where the function wasn't returning a string value, this is weird.

But in my case, I want to use an admin without an entity to keep some features added by an admin like adding it to the sidebar with the rest of my admin page, amongst others features.

Adding it to the sidebar can be done without creating an admin.

sonata_admin:
    dashboard:
        groups: 
            my_group:
                items:
                    - route: my_custom_route
                      label: my_custom_label
                      roles: [ ROLE_SOMETHING ]
                icon: '<i class="fa fa-foo" aria-hidden="true"></i>'

Can you list the others feature you need ?

And about your proposed solution to create a custom view by extending the admin_layout, why not. But it's not only for rendering, there is some logic behind not related to only one entity.

Again, can you list the logic/feature needed ?

Maybe it's already implemented or can be implemented in another/better way :)

rfaivre commented 4 years ago

Indeed, no need to have an admin in my case.

Can you list the others feature you need?

I use some features of admin but it can be done without using an admin.

Then I use some configuration in sonata_admin.yaml like you have mentioned and it works well. Thanks for the tip ;)

Thanks for your help @VincentLanglet ;) I think we can close this ticket and cancel the PR