New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DependencyInjection] Calls does not apply to service via autoconfigure #23497

Closed
BoShurik opened this Issue Jul 13, 2017 · 11 comments

Comments

Projects
None yet
7 participants
@BoShurik
Contributor

BoShurik commented Jul 13, 2017

Q A
Bug report? yes/no
Feature request? yes/no
BC Break report? no
RFC? no
Symfony version 3.3.4
    _defaults:
        autoconfigure: true

    AppBundle\Controller\Admin\:
        resource: '../../../src/AppBundle/Controller/Admin'
        public: true
        calls:
            - ['setLogger', ['@logger']]
        tags: ['controller.service_arguments']

    AppBundle\Controller\Admin\Block\BlockController:
        tags:
            - { name: app.admin, group: site }
Information for Service "AppBundle\Controller\Admin\Block\BlockController"
==========================================================================

 ---------------- -------------------------------------------------- 
  Option           Value                                             
 ---------------- -------------------------------------------------- 
  Service ID       AppBundle\Controller\Admin\Block\BlockController  
  Class            AppBundle\Controller\Admin\Block\BlockController  
  Tags             app.admin (group: site)                        
                   controller.service_arguments                      
  Public           yes                                               
  Synthetic        no                                                
  Lazy             no                                                
  Shared           yes                                               
  Abstract         no                                                
  Autowired        no                                                
  Autoconfigured   yes                                               
 ---------------- -------------------------------------------------- 

Calls did not apply to the service.
If I remove additional definition for AppBundle\Controller\Admin\Block\BlockController with tag, then all works correct

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jul 13, 2017

Member

This is not related to autoconfigure which is just responsible for adding tags when the service class implements a given interface or extends a given class.

Here, you're registering your controllers as services using the PSR-4 based discovery, that basically means "this defines as much services as there are classes in this namespace/directory`.
Then, you're defining explicitly one of these service, which would have been auto-created by the PSR-4 discovery used just before. This means "this service is an exception, it is configured explicitly, don't auto-discover it".
By doing so, you're just overriding the previous (PSR-4 discovered) definition as if it does not exist. Thus there is no bug, you have to explicitly define the method calls for this service, there is no kind of inheritance between the first (PSR-4 discovered) service and the second (explicit) one.

Member

chalasr commented Jul 13, 2017

This is not related to autoconfigure which is just responsible for adding tags when the service class implements a given interface or extends a given class.

Here, you're registering your controllers as services using the PSR-4 based discovery, that basically means "this defines as much services as there are classes in this namespace/directory`.
Then, you're defining explicitly one of these service, which would have been auto-created by the PSR-4 discovery used just before. This means "this service is an exception, it is configured explicitly, don't auto-discover it".
By doing so, you're just overriding the previous (PSR-4 discovered) definition as if it does not exist. Thus there is no bug, you have to explicitly define the method calls for this service, there is no kind of inheritance between the first (PSR-4 discovered) service and the second (explicit) one.

@Hanmac

This comment has been minimized.

Show comment
Hide comment
@Hanmac

Hanmac Jul 13, 2017

i have done this with normal services (specially abstract ones), but is there a way for "AppBundle\Controller\Admin\Block\BlockController" to use "AppBundle\Controller\Admin\" as Parent?

Hanmac commented Jul 13, 2017

i have done this with normal services (specially abstract ones), but is there a way for "AppBundle\Controller\Admin\Block\BlockController" to use "AppBundle\Controller\Admin\" as Parent?

@Basster

This comment has been minimized.

Show comment
Hide comment
Contributor

Basster commented Jul 13, 2017

@BoShurik

This comment has been minimized.

Show comment
Hide comment
@BoShurik

BoShurik Jul 13, 2017

Contributor

parent does not work with _defaults

Contributor

BoShurik commented Jul 13, 2017

parent does not work with _defaults

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jul 13, 2017

Member

parent does work with _defaults, but you have to redefine the attributes that are defined in _defaults on the child service (it cannot inherit them). What breaks is:

AppBundle\Controller\Admin\Block\BlockController:
    parent: AppBundle\Controller\Admin\Block\

Because AppBundle\Controller\Admin\Block\ is not a service, it's one service per class found in this namespace.

Member

chalasr commented Jul 13, 2017

parent does work with _defaults, but you have to redefine the attributes that are defined in _defaults on the child service (it cannot inherit them). What breaks is:

AppBundle\Controller\Admin\Block\BlockController:
    parent: AppBundle\Controller\Admin\Block\

Because AppBundle\Controller\Admin\Block\ is not a service, it's one service per class found in this namespace.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 13, 2017

Member

Note that we maybe could do something about this:
Imagine the psr4 loader creates a FQCN.prototype private service, aliased to its FQCN so that things work the same, then a child def could have FQCN.prototype as parent and achieve what we're talking about here. WDYT? Would like to give it a try?

Member

nicolas-grekas commented Jul 13, 2017

Note that we maybe could do something about this:
Imagine the psr4 loader creates a FQCN.prototype private service, aliased to its FQCN so that things work the same, then a child def could have FQCN.prototype as parent and achieve what we're talking about here. WDYT? Would like to give it a try?

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jul 13, 2017

Member

Makes sense to me, I will do!

For 3.3 the alternatives seems to create an abstract service extended by both

_defaults:
    autoconfigure: true

app.admin.abstract_controller:
    abstract: true
    public: true
    calls:
        - ['setLogger', ['@logger']]

AppBundle\Controller\Admin\:
    parent: app.admin.abstract_controller
    resource: '../../../src/AppBundle/Controller/Admin'
    tags: ['controller.service_arguments']

AppBundle\Controller\Admin\Block\BlockController:
    parent: app.admin.abstract_controller
    tags:
        - { name: controller.service_arguments }
        - { name: app.admin, group: site }

Or use _instanceof

_instanceof:
    Symfony\Bundle\FrameworkBundle\Controller\Controller: # better use your own base controller
        public: true
        tags: ['controller.service_arguments']
        calls:
             - ['setLogger', ['@logger']]

AppBundle\Controller\Admin\:
    resource: '../../../src/AppBundle/Controller/Admin'
 
AppBundle\Controller\Admin\Block\BlockController:
    tags:
        - { name: app.admin, group: site }
Member

chalasr commented Jul 13, 2017

Makes sense to me, I will do!

For 3.3 the alternatives seems to create an abstract service extended by both

_defaults:
    autoconfigure: true

app.admin.abstract_controller:
    abstract: true
    public: true
    calls:
        - ['setLogger', ['@logger']]

AppBundle\Controller\Admin\:
    parent: app.admin.abstract_controller
    resource: '../../../src/AppBundle/Controller/Admin'
    tags: ['controller.service_arguments']

AppBundle\Controller\Admin\Block\BlockController:
    parent: app.admin.abstract_controller
    tags:
        - { name: controller.service_arguments }
        - { name: app.admin, group: site }

Or use _instanceof

_instanceof:
    Symfony\Bundle\FrameworkBundle\Controller\Controller: # better use your own base controller
        public: true
        tags: ['controller.service_arguments']
        calls:
             - ['setLogger', ['@logger']]

AppBundle\Controller\Admin\:
    resource: '../../../src/AppBundle/Controller/Admin'
 
AppBundle\Controller\Admin\Block\BlockController:
    tags:
        - { name: app.admin, group: site }
@Basster

This comment has been minimized.

Show comment
Hide comment
@Basster

Basster Jul 14, 2017

Contributor

If its just about the logger, why not implementing Psr\Log\LoggerAwareInterface on services/controllers, that should log and create an _instanceof entry with it? Repeat for other ServiceAware Services/Controllers, e.g with your own ...AwareInterfaces. This way you also follow composition over inheritance. Or maybe I don't get your point...

Contributor

Basster commented Jul 14, 2017

If its just about the logger, why not implementing Psr\Log\LoggerAwareInterface on services/controllers, that should log and create an _instanceof entry with it? Repeat for other ServiceAware Services/Controllers, e.g with your own ...AwareInterfaces. This way you also follow composition over inheritance. Or maybe I don't get your point...

@BoShurik

This comment has been minimized.

Show comment
Hide comment
@BoShurik

BoShurik Jul 14, 2017

Contributor

@Basster It was just an example. Unfortunately, I try to refactor an lagacy app, so can't easily use interfaces. In my new app I use exactly this approach

Contributor

BoShurik commented Jul 14, 2017

@Basster It was just an example. Unfortunately, I try to refactor an lagacy app, so can't easily use interfaces. In my new app I use exactly this approach

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr
Member

chalasr commented Jul 17, 2017

See #23548

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 19, 2017

Member

Closing as "won't implement". See experiment in #23548, which didn't go well enough :)

Member

nicolas-grekas commented Jul 19, 2017

Closing as "won't implement". See experiment in #23548, which didn't go well enough :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment