Skip to content

[DependencyInjection] Better autowiring integration #20302

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

Closed
theofidry opened this issue Oct 25, 2016 · 33 comments
Closed

[DependencyInjection] Better autowiring integration #20302

theofidry opened this issue Oct 25, 2016 · 33 comments

Comments

@theofidry
Copy link
Contributor

theofidry commented Oct 25, 2016

Heavily inspired from DunglasActionBundle and ActionAutowire, I would like to suggest to integrate the following to the core:

framework:
  autowiring:
    # In this include statement, a list of directories can be given in which all PHP classes found
    # will be registered as auto-wired services
    include:
        - '/path/to/dir'                    # can be an absolute path
        - '%root.kernel_dir%/path/to/dir'   # can use parameters for the path
        - '@AppBundle/Http/Action'          # can use the @Bundle notation
        - '/path/to/dir':       # can specify the directory to list as a key and then give a list of elements to filter inside it
            - 'Foo.php'         #regular file, equivalent to '/path/to/dir/Foo.php'
            - '/.*Foo\.php/i'   # can also make use of a regex
    exclude: ~ # Same structure as include
    # This service section works pretty much as the `services` root declaration. The services
    # declared there declared as auto-wired (so no `autowire: true` required) and have a
    # simplified list of properties that can be set, as the other are irrelevant for auto-wired
    # services.
    services:
        App\FooInterface: @App\Foo # bind the App\FooInterface to the concrete class App\Foo

        # Complete list of the elements accepted for an autowired service
        App\Foo:
            public: false
            methods: # list of autowired methods
                - setBar
            tags: [ {name: mytag} ]
            calls:
                - [ setFoo, ['foo'] ]

This would help a lot to work with autowiring, trigger it as a default in specific places of your application, and ease the service registration for auto-wired services instead of having the classical one which is a bit cumbersome.

WDYT?

/cc @dunglas @TomasVotruba

@javiereguiluz
Copy link
Member

If we want to fully integrate autowiring in core, I'd recommend first to study how other frameworks solve this problem and their pros/cons.

For example, in the case of the Spring Framework, this is how it works:

  1. you can set the @Autowired annotation in constructors, properties and methods of your classes (not only setters):
public class MovieRecommender
{
    @Autowired
    private MovieCatalog movieCatalog;

    private CustomerPreferences preferences;

    @Autowired
    public MovieRecommender(CustomerPreferences preferences) {
        this.preferences = preferences;
    }

    @Autowired
    public void setMovieFinder(MovieFinder movieFinder) {
        this.movieFinder = movieFinder;
    }

    @Autowired
    public void prepare(MovieCatalog movieCatalog,
                        CustomerPreferences preferences) {
        this.movieCatalog = movieCatalog;
        this.preferences = preferences;
    }

    // ...
}
  1. If some dependency is optional, use the required option: @Autowired(required=false)

  2. When there are multiple autowiring candidates, you can use the @Qualifier
    annotation to help Spring decide which one to choose:

public class MovieRecommender {

    @Autowired
    @Qualifier("main")
    private MovieCatalog movieCatalog;

    // ...
}

The value of @Qualifier is defined by each bean:

<?xml version="1.0" encoding="UTF-8"?>
<beans ...>
  <bean class="example.SimpleMovieCatalog">
      <qualifier value="main"/>
  </bean>

  <bean class="example.SimpleMovieCatalog">
      <qualifier value="action"/>
  </bean>

  <bean id="movieRecommender" class="example.MovieRecommender"/>
</beans>

Read more abut autowiring collaborators in the Spring Framework manual.


How do other frameworks implement the autowiring feature?

@dunglas
Copy link
Member

dunglas commented Oct 26, 2016

@javiereguiluz Spring's solution works well because of how annotations are implemented in Java. This approach isn't suitable for Symfony because it will create a hard dependency between the user's class and the Symfony's annotation.

The solution of @theofidry / ActionBundle is nice because it is an external solution. The autowired PHP class doesn't have to know how its dependencies are injected. So, the class can be reused in other contexts (manual initialization, container without autowiring...).

@dunglas
Copy link
Member

dunglas commented Oct 26, 2016

Btw, with #20167 it will now be possible to autowire any method (not only setters), just like in Spring.

@javiereguiluz
Copy link
Member

@dunglas I'm not proposing to copy other solutions. I just say that a lot of other people have solve this very same problem previously. Let's see how did they solve it and learn from them.

For example, autowiring seems to be a very popular Laravel feature and their users are happy with it. How does autowiring work in Laravel? And in other frameworks?

@dunglas
Copy link
Member

dunglas commented Oct 26, 2016

Edit: here is a table

Library Autowiring Enabled by default Method Autowiring Interface binding
Symfony DIC yes no yes (#20167) yes
Laravel IoC yes yes no yes
PHP DI yes yes no yes
Zend DI yes no yes yes
PHP League's Container yes no no no
Aura DI yes no no yes
Nette yes yes yes yes

@javiereguiluz I studied other implementations before adding autowiring support to SF (especially Spring and Laravel).

For Laravel: https://laravel.com/docs/4.2/ioc#automatic-resolution

Method autowiring isn't supported. Interface binding is similar to the autowiring_types config key of Symfony. Autowiring in on by default for all services.

Other libs supporting autowiring in the PHP world:

  • PHP DI: similar to how Symfony works but autowiring is enabled by default, no method autowiring support
  • Zend DI: autowring is off by default. It supports method autowiring (called "runtime definition") and automatically autowire setters (Symfony do it at compile time and will not autowire setters by default when [DependencyInjection] Make method (setter) autowiring configurable #20167 will be merged)
  • PHP League's Container: autowiring off by default, doesn't support method autowiring nor interface binding
  • Aura DI: not enabled by default, interface binding supported, no method autowiring

@javiereguiluz
Copy link
Member

@dunglas thanks for the summary!

Let's ask to our friend @TomasVotruba whether the Nette Framework Autowiring has some innovative ideas or concepts different from other frameworks. Thanks!

@dunglas
Copy link
Member

dunglas commented Oct 26, 2016

@javiereguiluz I've updated my comment to add a summary table.

@theofidry
Copy link
Contributor Author

I'm personally all in favour of annotations as well, it would be consistent with the routing annotations. However even with the annotations, the main problem still remain: you might want to autowire complete folders (DunglasActionBundle is a good example, and is not alone). In Laravel for example, you don't have this issue because everything is autowired by default. But as soon as you need something more, adding a tag for example, you have to have a service definition like you would need here. I'm just proposing to make it much lighter for that part.

@GuilhemN
Copy link
Contributor

GuilhemN commented Oct 26, 2016

@theofidry using annotations requires an expansive process and i don't get what it brings against your current proposal.

BTW, just an idea, imo your proposal could be even more interesting:

framework:
    autoregistration:
        include: # ...
        services:
            *: { autowired: true }
            CustomCommandInterface:
                calls:
                    [ setCustomClass, [ "@custom_service", "%parameter" ] ]

Autowiring would not be by default and this feature could be extended to stuff not managed by autowiring (scalar arguments, services with the same implements, ...).
WDYT?

@hason
Copy link
Contributor

hason commented Oct 26, 2016

@javiereguiluz For service definition is great Neon (Nette). I made a bridge to Symfony. I like set autowiring explicitly, syntax in neon is very short: https://github.com/symfonette/neon-integration#autowiring

@theofidry
Copy link
Contributor Author

theofidry commented Oct 26, 2016

@Ener-Getick to avoid any configuration, like for JMSDiExtraBundle you can declare services without having to pass via a service declaration.

I'm not sure to understand what *: { autowired: true } is supposed to do, but it's true that we have to support calls and setters injection as well. I'm updating the description accordingly

As for @dunglas comment:

This approach isn't suitable for Symfony because it will create a hard dependency between the user's class and the Symfony's annotation.

I'm not sure to follow, if you are using a router annotation in an action/controller, you have a hard coupling all the same, it's up to the user to be willing to introduce such coupling or not.

@hason
Copy link
Contributor

hason commented Oct 26, 2016

@javiereguiluz The another great feature of Neon/Nette are typed services (as in @theofidry example). For Symfony exists #20264.

@GuilhemN
Copy link
Contributor

@theofidry * is a pattern, in this case it would be the default behavior.
It would be great imo if all applicable services definitions were merged, i.e. for the example i gave, classes implementing CustomCommandInterface would give:

services:
    my_service:
        autowired: true
        calls:
            [ setCustomClass, [ "@custom_service", "%parameter" ] ]

@theofidry
Copy link
Contributor Author

@Ener-Getick you mean to set autowiring with all setters by default or just autowiring with the constructor? What about a type key instead?

framework:
  autowiring:
    type: ~ # default to `constructor`, `*` for constructor + setter
    annotation: false # to be discussed, but that could be easily done if we were to want it

@GuilhemN
Copy link
Contributor

@theofidry I was thinking not enabling it at all by default and let the user choose through a global pattern in services just like other services options.

@TomasVotruba
Copy link
Contributor

@dunglas You can update the table for Nette, where all is YES.

@theofidry What exactly is the goal here? We can talk about wide range of topics around autowiring, which is happening in the comments. Rather express the GOAL and WHY .

@theofidry
Copy link
Contributor Author

@TomasVotruba updated, I think the goal is rather clear: make it easier to work with autowiring in Symfony.

@dunglas
Copy link
Member

dunglas commented Oct 26, 2016

@TomasVotruba Nette added to the table.

@TomasVotruba
Copy link
Contributor

@theofidry In what way exactly?

@dunglas Thank you

@theofidry
Copy link
Contributor Author

This would help a lot to work with autowiring, trigger it as a default in specific places of your application, and ease the service registration for auto-wired services instead of having the classical one which is a bit cumbersome.

I'm not sure what you want me to say more than that. To me it doesn't make sense to have autowiring everywhere (i.e. as global) as my domain is usually mostly composed of interfaces and there is a lot of third-party services for which the binding is not done and I'm not willing to do. There is however parts of my application where it makes sense to have auto-wiring enabled by default (e.g. command Handlers, Action/Controllers, Console commands). Also if you want to use it globally, you just have to include your src folder and that's it.

The regular service registration is also a bit awkward for auto-wired services. Mainly because this service declaration has not been designed for it (you can't use FQCN as an ID although #20264 may solve that) and autowiring is not the default. Having a dedicated part of the config for autowired services only can help to solve that.

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Oct 26, 2016

I'm curious. What issue does this solve? What code in your application made you think about this?
I look for real code and real issue. Theory is hard to solve and could go many wrong directions easily.

To your suggestions. It's very counter intuitive to use autowiring only in some specific path.
I know 2 groups of people:

  • those who use autowiring (there are many bundles for that)
  • and those who don't (default)

It's personal preference.

Personally, I would not use this feature, as it adds great complexity and doesn't solve any of my issues.
It's like passing 50% of dependencies to a class.

Also it would lead newcomers to use autowiring only somewhere, because it's possible. Another redundant duality in coding.

What only makes sense to me from this discussion and relevant links, is to make it configurable in config.yml, sth like:

autowire: on

autowire: off # now default

@theofidry
Copy link
Contributor Author

Having autowiring enabled by default for all services can't work, it will simply break everything as any service:

services:
  dummy:
    class: App\Dummy
    arguments:
      - '@foo' # typehints an interface
      - '@bar'

will break. So this cannot be done, unless patching all the third-party services registered to your application which is tedious and doesn't make much sense.

What code in your application made you think about this?

As said command handlers, actions, console commands as said. Or any part of my application where there is not a big layer of abstraction. For those parts, I don't want to have to declare my services at all, hence autowiring by default for certain parts of my application. This is partly what DunglasActionBundle does despite its naming.

Now obviously this won't always work. If you have a certain abstraction layer, for example by introducing an interface, you have to dirty a bit your hands and do some manual work. If you take the example on how you do it in Laravel with autowiring (you have the choice in Laravel after all), if you have an interface, you have two ways:

  1. Bind an interface to an implementation:
$this->app->bind('App\Contracts\EventPusher', 'App\Services\RedisEventPusher');
  1. Bind an interface to an implementation for a specific case (contextual binding):
$this->app->when('App\Handlers\Commands\CreateOrderHandler')
          ->needs('App\Contracts\EventPusher')
          ->give('App\Services\PubNubEventPusher');

(Note: @dunglas maybe you should add this contextual binding to your table)

The first one is easily done in Symfony, although I'm not a big fan the way it's done:

services:
  redis_event_pusher:
    class: App\Services\RedisEventPusher
    autowired: true
    autowired_types: [ App\Contracts\EventPusher ]

#20264 Would help a lot here as it would allow to simply do:

services:
  App\Contracts\EventPusher: @App\Services\RedisEventPusher

  App\Services\RedisEventPusher:
    autowired: true

As for the second one, I find it unmanageable. This is were I prefer to explicitly inject my dependencies.

There is also the case where you have to still add a few things to your autowired service: a tag or declaring it as private:

services:
  redis_event_pusher:
    class: App\Services\RedisEventPusher
    autowired: true
    public: false
    tags: [ { name: myTag } ]
    calls: [ [ setFoo, '%myparam%' ] ]

Again with #20264, this can be a bit more simplified:

services:
  App\Services\RedisEventPusher:
    autowired: true
    public: false
    tags: [ { name: myTag } ]
    calls: [ [ setFoo, '%myparam%' ] ]

My suggestion of framework.autowiring.services was done prior to me knowing about #20264 (and we don't have the guarantee this one is accepted yet), so hence the idea of having a place to declared autowired services and in this place to be able to use FQCN for ids as this makes much more sense for autowired services.

What issue does this solve?

I hope this answered your question. Maybe your uneasiness was from the fact that I'm suggesting to use autowiring only in some places (and not just newcomers), and this is exactly what I want to do. This is currently the case, I just want to make it easier. It's (IMO) not a redundancy: it's two ways to use a DIC, and I'm all for using both at the same time rather that constraining myself to one extreme.

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Oct 26, 2016

Having autowiring enabled by default for all services can't work, it will simply break everything as any service.

Actually, I use that approach in Symplify/DefaultAutowire bundle and it works well. There are few more bundles, that already supported that since about 3 years ago.

This is more about how to refactor autowire not-ready architecture to autowire ready. I've done that couple of times and it really is about following few principles. Like using DI over statics.
Since I use autowiring thanks to Nette\DI for last 6 years, I recommend not to add any bidirectional transition layer. It would definitely confuse me.

Much useful would be writing a series about refactoring of real use cases.

@theofidry
Copy link
Contributor Author

theofidry commented Oct 26, 2016

Hmm seems like I missed something. Apparently if you have the following:

services:
    app.dummy:
        class: AppBundle\Dummy
        arguments:
            - '@app.fooa' # typehint `FooInterface`
           - '%myparam%'
        autowire: true

This will work, because the constructor won't be autowired even though the service is declared as autowired. @dunglas I wasn't aware of this feature, is it intentional? If yes then yeah it would be possible to switch autowiring globally although the existing code will not provide any binding for the existing interfaces.

Assuming this behaviour is intentional and we can have a global switch, what I'm missing is this include bit, which I would default to src if the global switch is on, to avoid any service registration by default and have true autowiring.

@dunglas
Copy link
Member

dunglas commented Oct 26, 2016

@theofidry It's more subtile:

class Foo
{
    public function __construct(A $a, B $b, C $c) {}
}
services:
    app.foo:
        class: Foo
        arguments:
            - '' # autowired, will find and inject @app.b automatically
            - '@app.b' # not autowired, @app.b is explictly injected
           # the 3rd argument will be autowired too
        autowire: true

@theofidry
Copy link
Contributor Author

so in which case might we have a broken case when switching all services as autowired by default?

@GuilhemN
Copy link
Contributor

@theofidry caching may be an issue (in a dev env you may intentionally not inject a Cache provider into one of your services but autowiring will inject it). Most of the time an app won't be broken because you enable autowiring everywhere but you will face unexpected behaviors.

@theofidry
Copy link
Contributor Author

You mean if enabling autowiring in such a way may register services you didn't expect which may cause unexpected behaviour?

Well yeah I could see that actually, which is why I feel more comfortable with enabling autowiring for some directories only. So to recap, what I would suggest is:

framework:
  autowiring:
    enable: false # (default value), if enabled, turns on autowiring by default for any
                        # class found in the include section and service declared are
                        # all autowired
    include: [ '%root.kernel_dir%/../src' ]
    exclude: []

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Oct 26, 2016

@theofidry Now it looks much better and easier to understand.

I'm definitely for:

framework:
  autowiring:
    enable: false # true

@theofidry
Copy link
Contributor Author

@TomasVotruba I feel like the include/exclude are still necessary. Even if enabling autowiring globally won't break things, this will still have an impact: classes that are not necessarily services will be registered to the application. This may not be a desirable effect because:

  • You may just not want to have something like an entity registered to the DIC, even if it doesn't make sense in terms of usage, you may not want to allow that. I personally don't care much about this one, but I understand this could be an issue for some people.
  • Depending of your application, you may register a lot of non-services classes to your application, over-bloating your DIC, which may have a non negligible impact performance wise.

@dunglas
Copy link
Member

dunglas commented Nov 2, 2016

I don't like this approach:

framework:
  autowiring:
    enable: false

You may want to have some classes with autowiring on for convenience (the controller layer) and some others without autowiring disabled for strictness (the domaine layer).

I prefer the original approach of Action Bundle and @theofidry. It is flexible enough and covers all use cases.

@javiereguiluz
Copy link
Member

@theofidry do you consider that we can close this as "fixed"? We've added almost everything you asked for or will do soon.

@theofidry
Copy link
Contributor Author

Indeed, we settled for globs for now (can't remember the PR) which is good enough for me. Maybe some people would like more advanced features, but that's where it might be more interesting to go for a dedicated bundle rather than integrating that in the core.

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

No branches or pull requests

6 participants