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

For $adminClasses, service id still assigned and passed to pool in AddDependencyCallsCompilerPass #7951

Closed tamcy closed 2 years ago

tamcy commented 2 years ago

Hi,

I just discovered that additional work is needed to completely resolve the "service id or admin code" problem in the admin pool class, which I overlooked in my previous patch.

The first one is the $adminClasses variable of the pool, which serves as the model class name lookup array. The variable is used in places like when building a many_to_one list field. Currently service ids are still wrongly assigned to it by AddDependencyCallsCompilerPass, which causes the lookup to fail.

Before I submit a PR for this, I would like to seek your advice for the following two matters which are also related to AddDependencyCallsCompilerPass:

  1. I feel the $id in the following code block also needs to be fixed, but I am not 100% sure, and I probably can't provide a useable test case for this one:

https://github.com/sonata-project/SonataAdminBundle/blob/1b919ccc08e3e924dbd4857756bf238f2909b593/src/DependencyInjection/Compiler/AddDependencyCallsCompilerPass.php#L109-L119

Please advice if I should also change the $id to $code in the PR.

  1. I spotted a typo in the deprecation message I last submitted and merged. Is it fine to also include the fix in the PR of this issue, or it's better to submit another PR?

Thank you!

VincentLanglet commented 2 years ago

The reproducer for 1 would be

admin.category:
        class: App\Admin\CategoryAdmin
        arguments: [~, App\Entity\Category, ~]
        tags:
            - { name: sonata.admin, code: custom_code, manager_type: orm, label: Category }

This is not a big deal, since if you start using the new key code, model_class, controller, you should stop using arguments. But a fix will be accepted.

Fixing (2) in the same PR is ok.

tamcy commented 2 years ago

Thank you, then I will a PR that also fixes (2) and (3).