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

Cannot use a custom controller on master branch #6773

Closed VincentLanglet closed 3 years ago

VincentLanglet commented 3 years ago

Some informations can be found https://github.com/sonata-project/SonataAdminBundle/pull/6770

Both

    Sonata\AdminBundle\Tests\App\Admin\FooAdmin:
        arguments: [~, Sonata\AdminBundle\Tests\App\Model\Foo, Sonata\AdminBundle\Controller\CRUDController]
        tags:
            - {name: sonata.admin, manager_type: test, label: Foo}

and

    Sonata\AdminBundle\Tests\App\Admin\FooAdmin:
        arguments: [~, Sonata\AdminBundle\Tests\App\Model\Foo, Sonata\AdminBundle\Tests\App\Controller\CustomCRUDController]
        tags:
            - {name: sonata.admin, manager_type: test, label: Foo}

configuration doesn't work.

Only

Sonata\AdminBundle\Tests\App\Admin\FooAdmin:
        arguments: [~, Sonata\AdminBundle\Tests\App\Model\Foo, ~]
        tags:
            - {name: sonata.admin, manager_type: test, label: Foo}

does.

The error are

[critical] Uncaught PHP Exception LogicException: ""Sonata\AdminBundle\Controller\CRUDController" has no container set, did you forget to define it as a service subscriber?" at /home/runner/work/SonataAdminBundle/SonataAdminBundle/vendor/symfony/framework-bundle/Controller/ControllerResolver.php line 39

It seems related to the fact that we changed how we register the default controller on dependency injection. Before that change it was a Symfony controller that it was not explicitly declared on DI (the old way of registering controllers). After the change it was explicitly declared on DI: https://github.com/sonata-project/SonataAdminBundle/blob/master/src/Resources/config/core.php#L164-L167

franmomu commented 3 years ago

Adding @required annotation to CRUDController:setContainer and defining the controller public and with autowire to true, make the controller work, maybe with https://github.com/sonata-project/SonataAdminBundle/pull/6615 it is not necessary the autowiring.

It would be nice to add a test for custom controller in 3.x (could be using a class and other with service).

VincentLanglet commented 3 years ago

It would be nice to add a test for custom controller in 3.x (could be using a class and other with service).

I can add them, but then it won't work on master ; so we'll have to skip them on master on the next merge until we find a fix.

franmomu commented 3 years ago

I think this fixes the issue:

Adding @required annotation to CRUDController:setContainer and defining the controller public and with autowire to true, make the controller work, maybe with #6615 it is not necessary the autowiring.

VincentLanglet commented 3 years ago

I think this fixes the issue:

Adding @required annotation to CRUDController:setContainer and defining the controller public and with autowire to true, make the controller work, maybe with #6615 it is not necessary the autowiring.

Can you approve https://github.com/sonata-project/SonataAdminBundle/pull/6770 then ? Then I'll wait for #6615 to check if it fix the issue :)