This feature is indeed a must have to better support the uses cases described in the linked issue.

What if someone wants to use autowiring but only wants to use constructor autowire and not setter autowire? Or not for all methods (in that case setter autowiring should not be used).

@sstok If acall is explicitly defined for a method, the autowiring system will ignore this method.

Supporting only constructor autowiring or both setter and constructor using a new flag inDefinition should be possible but it looks overkill too me. If you have such edge case for a service, the clean way is just to not use autowiring at all for this service. What do you think?

@fabpot support forfunction setDependencies(RouterInterface $router, Twig $twig) style added.

@dunglas I love it! I just tried the code, here are the issues / concerns I found:

1. The fact that would we start auto-wiring setters is a behavior change - it could be a BC break (setters are suddenly called that weren't before). Question: is this a BC break?

2. If I create a setter withno arguments, the setter is called. I don't think it should: in this case, there's nothing to inject.

3. If a setterdoes have an argument, but its optional and we can't autowire it, : I think we should do nothing, instead of calling the setter withnull.

4. If a setterdoes have an argument and it'srequired, but we can't auto-wire it (e.g. no type-hint), I think we should do nothing (don't call the method, but don't throw an exception - the user may manually add acalls for this).

Thanks!

@dunglas any feedback on@weaverryan comments?

They are all relevants, I need to find some time to work on this.

For the BC break, as@sstok has the same concern, I'll introduce a newaurowireSetter property inDefinition that will befalse by default and set it totrue in action services created by DunglasActionBundle.

@dunglas That will fix BC, but I'm concerned about usability - e.g.:

services:spring_weather_manager:class:...autowire:trueautowire_setter:true

If it weren't for the BC problem, my opinion would be: always do setter injection - after all, if it causes you any issues, don't use it. For that reason, I'd prefer a global flag to turn this on/off that goes away in 4.0.

On the other hand, the BC break is very limited: it will only impact people using autowiring (just a few), having classes with setters for services (unlikely, because it wasn't supported in the first version of the autowiring system), and having a setter that broke the class when triggered (very very unlikely). Maybe can we say that this "BC break" is acceptable, as it's more an edge case than a BC break.

Honestly, I'm inclined to agree. This is a "magic" feature you're opting into, it's only 1 version it's been available, it won't affect many people, and if it causes an issue, it'll happen at build time (very easy to debug).

👍 for the small BC break instead of yet another configuration setting.

I agree with you about accepting this small BC break (but we need to document it clearly in changelog files)

Ok, so BC break is ok. This leaves items 2-4#17608 (comment), when you have some time :)

@dunglas Any idea when you will be able to finish this one?

Before the end of this week.

All issues fixed. Can you review@weaverryan?

Hi Dunglas!

I need a few more days to get back a full report - I will make a very clear explanation of the functionality so that everyone is clear how it works (a few cases are almost subjective, so I want it to be clear to everyone).

The only issue I've found is that if we cannot autowire because the type-hint is for a non-existent class, it simply doesn't call the method. That's different than the constructor where it throws an exception.

Also, I think we (probably me, as I've had some recent PR's merged) may have introduced a bug into the master branch. I'm too short on time to look further right this moment, but I'm getting weird results if I type-hint something likeDataCollector orEventSusbcribterInterface. These are items that shouldnot be autowireable (as there are multiple services for them), but on master locally right now, theyare autowiring - withone of the matching services. This does not happen on 3.0.1. If someone else can verify this, awesome. Otherwise, I'll look more soon.

I think we're getting quite close to mastering the exact behavior we want. We need to get it right for 3.1, because whatever we end up with will probably stay with us due to BC.

I think this PR is ready. The question is: do we want it? It's opinionated (but so is autowiring in general, and it's opt-in). Some of the new autowiring features/uses (like those in DunglasActionBundle) are quite radical, and I think they need to be tested before considering moving further forward (in the core) in that direction. However, if this were merged, it allows more of these ideas to be tested out in the wild - like using traits e.g.RouterTrait) to automatically setter-inject services+shortcuts into your services.

I'm 👍, but I don't feel as confidently about this as I have other things in the past.

@symfony/deciders

👍 LGTM

Rebased. Can be merged.

@symfony/deciders what do we do of this PR? Should I close it or merge it?

Re-read everything here.@weaverryan and@dunglas did a great job at defining exactly what we should do and what is out of scope. I agree with everything@weaverryan said and as this is what@dunglas implemented, I'm 👍 (I would not advocate its usage for everything but as this is an opt-in feature anyway, this looks good to me).

@dunglas Looks like it needs a rebase

Rebased.

@dunglas I didn't notice but history is really ugly but as there are 2 contribs, gh won't be able to squash commits. Can you do that for me?

Should I merge all commits in 1 or should I keep the commit of@weaverryan?

I think you can merge everything into 1 commit as@weaverryan contrib is not huge.

Done.

Thank you@dunglas.

@dunglas Can you take care of the docs?

Yes, I've a lot of docs to write...