-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Comments
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:
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;
}
// ...
}
public class MovieRecommender {
@Autowired
@Qualifier("main")
private MovieCatalog movieCatalog;
// ...
} The value of <?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? |
@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...). |
Btw, with #20167 it will now be possible to autowire any method (not only setters), just like in Spring. |
@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? |
Edit: here is a table
@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 Other libs supporting autowiring in the PHP world:
|
@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! |
@javiereguiluz I've updated my comment to add a summary table. |
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. |
@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, ...). |
@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 |
@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 As for @dunglas comment:
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. |
@javiereguiluz The another great feature of Neon/Nette are typed services (as in @theofidry example). For Symfony exists #20264. |
@theofidry services:
my_service:
autowired: true
calls:
[ setCustomClass, [ "@custom_service", "%parameter" ] ] |
@Ener-Getick you mean to set autowiring with all setters by default or just autowiring with the constructor? What about a 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 |
@theofidry I was thinking not enabling it at all by default and let the user choose through a global pattern in |
@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 . |
@TomasVotruba updated, I think the goal is rather clear: make it easier to work with autowiring in Symfony. |
@TomasVotruba Nette added to the table. |
@theofidry In what way exactly? @dunglas Thank you |
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 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. |
I'm curious. What issue does this solve? What code in your application made you think about this? To your suggestions. It's very counter intuitive to use autowiring only in some specific path.
It's personal preference. Personally, I would not use this feature, as it adds great complexity and doesn't solve any of my issues. 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 autowire: on
autowire: off # now default |
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.
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:
$this->app->bind('App\Contracts\EventPusher', 'App\Services\RedisEventPusher');
$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
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. |
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. Much useful would be writing a series about refactoring of real use cases. |
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 |
@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 |
so in which case might we have a broken case when switching all services as autowired by default? |
@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. |
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: [] |
@theofidry Now it looks much better and easier to understand. I'm definitely for: framework:
autowiring:
enable: false # true |
@TomasVotruba I feel like the
|
I don't like this approach: framework:
autowiring:
enable: false You may want to have some classes with autowiring I prefer the original approach of Action Bundle and @theofidry. It is flexible enough and covers all use cases. |
@theofidry do you consider that we can close this as "fixed"? We've added almost everything you asked for or will do soon. |
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. |
Heavily inspired from DunglasActionBundle and ActionAutowire, I would like to suggest to integrate the following to the core:
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
The text was updated successfully, but these errors were encountered: