Looks good to me! 👍

What about allowing a collection of setters to autowire;
setSettersAutowired(['setBar', 'setBaz']); // but not setFoo()?

@ro0NL I don't think there is much benefit compared to the existing

        calls:
            - [setMailer, ['@app.mailer']]

To clarify on why this change is needed, enabling auto-wiring without this flag could cause a BC break. Given following example service:

class Foo
{
    private $baz;
    public function __construct(Bar $bar)
    {
        $this->baz = $bar;
    }

    public function setBaz(Taz $taz)
    {
        $this->baz = $taz;
    }
}

Turning auto-wiring on in earlier DependencyInjection versions calls __construct.

Current master calls __construct and then setBaz (the risk is injecting a different instance here).

This PR reverts the behavior to just __construct, while setBaz is to be enabled by the consumer.

@dunglas guess so, it's considerable though. There will be situations you want to ideally autowire 9/10 methods and face a all or nothing choice: go with autowiring or configure 9 services. We fully lose the autowiring benefit then.. which can be a, pragmatic, pain :-)

which can be a, pragmatic, pain :-)

It's still out of the 80/20 use-case, from the looks of it.

Agree :)

edit: consider

calls:
  - [setBar, [@arg]]
  - setFoo # autowired

Yeah, i think i like

autowire: true
[autowire_]calls:
   - setBar
   - setFoo

better than

setters_autowired: true

Or setters_autowired: true should be an equivalent of calls: <all_setters>

👍

Should we consider this for inheritance? At least make children inherit the flag and maybe allow them to change it? Actually doing it afterwards for the autowire flag in #19643.

👍

I'm worried about usability. If you want full autowiring, it'll now be:

autowire: true
autowire_setters: true

That's a lot of work for something that's purely for rapid development. But, we need to keep BC also.

What about this suggestion, which includes part of @ro0NL's idea:

# A) deprecated, but would only autowire the constructor, allows BC
autowire: true

# B) would autowire everything, including setters
autowiring: true

# C) or, use an array to opt into only certain methods
autowiring: [__construct]

This is also - accidentally - more flexible.

Even more flexible, IMHO having to list the methods to autowire would indeed reduce the benefit of using this feature rather than fully fill calls, as pointed out by @dunglas.

The proposed flag sounds nice, keeping autowire managing only constructors per default which stay logic to me.

Otherwise, what about a choice?

autowiring: constructor
autowiring: setters
autowiring: [constructor, setters]

(Depreciating the current flag)

@weaverryan regarding usability, this is a reminder that setter injection is generally harmful, and while it's a nice-to-have, it's usually to be discouraged.

this is a reminder that setter injection is generally harmful

This, imo, explains why it should be verbose. autowire_setters: true #magic happens

edit: i rather be explicit in what methods are called (autowired or not). The shortcut (true) will be the nice-to-have imo.

I also agree with @Ocramius, we should discourage setter injection (it should be opt-in).

I added initial support for setter autowiring (#17608) to ease the creation of Symfony controllers that don't depend of the container (#18193). Such classes will basically be automatically registered (e.g.: DunglasActionBundle), the user will not have to care about enabling setters autowiring by itself.

Developpers wanting to always enable setters autowiring in other contexts can create a userland a compiler pass to do it.

this is a reminder that setter injection is generally harmful

Is it harmful? The user is already opting into magic. I'm not saying this is wrong, but I want to challenge the statement. Is there some evidence we can look at to support / contest this?

I added initial support for setter autowiring (#17608) to ease the creation of Symfony controllers that don't depend of the container

I'm interested in setter autowiring because of the potential to have traits with setters and helper methods - e.g. RouterTrait with setRouter and generateUrl methods.

My proposal (#19631 (comment)) offers BC, ease-of-use and control... but assumes the best default mode for autowiring is constructors and setters.

Is it harmful?

Yes, due to basic design of service dependency immutability.

The user is already opting into magic.

The comment about setter injection can be taken completely out of the scope of this PR or of the dependency injection component.

I'm interested in setter autowiring because of the potential to have traits with setters and helper methods - e.g. RouterTrait with setRouter and generateUrl methods.

Yes, it's basically the same use case. But for such cases, a compiler pass can automatically enable setters autowiring (for instance if the service implements a given interface).

Ok, I'm getting out-voted, and the arguments are sound.

👍 for the current approach

Just to clarify; this will call any defined set..() method, after you configured autowire_settings: true, right/?. I can live with it, no problem, but I hope you understand my concern why that's tricky and bad DX.

IMO, it's the same trickiness what we currently try to avoid.

@ro0NL any applicable setter. See #17608 and tests for details.

I see. How does this solve optional dependencies? I don't want setFoo(Bar $bar = null) to be called... im stuck?

Point is: you need to know at all time, whenever you define a setter, on whatever level in the class-chain, it's gonna be called (if enabled). It's tricky, period. :)

edit: i understand it's somewhat the current behavior, hence i can live with this approach. Just saying ;-)

What do you think about the following config, it should satisfy everybody's needs and is backward compatible:

autowire: true # constructor autowiring
autowire: [__construct, setFoo, setBar] # autowire whitelisted methods only
autowire: '*' # autowire constructor + every setters (following existing rules for setters autowiring)

What about

autowire: true
# enables constructor autowiring by default, if disabled no autowiring whatsoever happens

autowire_calls: [autowireThis, andThat]
# additional method calls to be autowired

autowire_setters: true
# adds all setters to autowire_calls

This looks intuitive.. but is limiting in a way, you can only autowire method calls when the constructor is autowired.

I prefer @dunglas proposal because the various configurations are mutually exclusive, but having several keys allows setting several of them at once, and from a DX perspective requires remembering 3 keys.

Okay then.. let's do it :) Thanks for letting me clarify my point.

@dunglas just thought of using bitwise flags, on php level it's pretty verbose;

$definition->setAutowired(Definition::CONSTRUCTOR|Definition::SETTERS);
// true = Definition::CONSTRUCTOR
// array = whitelist
if ($definition->getAutowired() & Definition::SETTERS) { }

For yaml Definition::FULL could work (CONSTRUCTOR|SETTERS), ie;

autowire: !php/const:Definition::FULL

or allow the * shortcut anyway.

autowire: !php/const:Definition::FULL is a no go for me.

Understand, hence i proposed allowing the * shortcut. It's more about the PHP structure for this. The * can be allowed for file loaders, or directly in Definition::setAutowired.

My proposal is more flexible and easier to learn (though)

Easier to learn, i guess so ;-) my proposal allows for adding Definition::SOME_FUTURE_GROUP_OF_METHODS, which will never happen.. but if you wanna talk about flexibility; it's their).

Anyway, lets move on. true|array|"*" is 👍

@ro0NL your proposal shows the wrong Yaml snippet. It would be like this:

autowire: !php/const:Symfony\Component\DependencyInjection\Definition::FULL

Left that out intentionally 👼 i know it's a no-go in some ways (too verbose i guess), but im looking at this from the php side first.

Status: needs work

As said, it would be a BC break to don't get this merged for 3.2.
@dunglas Are you fine or do you want some help?

I'll do it for the 3.2 release. If it's not ready, we'll revert the introduction of setter autowiring (and reintroduce it in 3.3) but I'll find the time to finish this PR.

I opened a new PR implementing #19631 (comment): #20167