A big 👎 from me ... unless we find a great and simple alternative to access services from commands.

unless we find a great and simple alternative to access services from commands.

Sure, there should still be an easy way to register commands which need dependencies, but IMO not any service should be accessible from a command (what are use cases for?).

Let's take the assets:install command as an example of unnecessary container injection: it gets the filesystem service once (that could also be instantiated directly given it has no deps) to set it as a private $filesystem prop, and it gets the kernel service to access bundles instances, which can be avoided by using the kernel.bundles_metadata parameter.
Registering as a service to inject it only what's actually needed would be easy. Same for almost all commands of the framework.

To compensate the fact that ContainerAwareCommand doesn't require any configuration (that is useful in userland), what about the autowiring alternative?

Before:

class DoSomethingCommand extends ContainerAwareCommand
{
    public function configure()
    {
        $fooHandler = $this->getContainer()->get('foo.handler'); // exception, not accessible yet
    }

    public function execute($input, $output)
    {
        $fooHandler = $this->getContainer()->get('foo.handler'); // what is this?
    }
}

After:

class DoSomethingCommand extends Command implements AutowiredCommandInterface
{
    private $fooHandler;
    
    public function __construct(FooHandler $handler)
    {
        // command has been auto registered as a an autowired service
        $this->fooHandler = $fooHandler;
    }

    // $fooHandler is available everywhere
}

@chalasr thanks for the explanation ... but let me show you a counter-example.

The Problem

I want to log messages during the command execution

The current solution

1. Extend from 1 class (ContainerAwareCommand)
2. Get the logger as $this->getContainer()->get('logger')

The proposed solution

1. Extend from 1 class (Command)
2. Implement 1 interface (AutowiredCommandInterface)
3. Add a __construct() method
4. Find the class associated with the service that you want. But how? Which class? Is it Logger? LoggerInterface? AutowirableLogger? etc.
5. Wait until Symfony magically maps that class to the service that you really want (logger)

It's quite clear with auto-wire for services (not so easy from DX side as mentioned by Javier), but remember that container it's not only services, there are also parameters.

@javiereguiluz @stloyd Fair enough. ContainerAware stays unfortunately the easier way to solve what you said and is probably not that evil in userland. Thanks!

I would still like to avoid making use of it in the core (where the container and the whole auto registration are not needed) and register them as services instead, making it easy to identify their scope and discover their dependencies. Any thought on that?

Closing due to the mentioned cons.

Note that there is no need for any AutowiredCommandInterface nor to extend any base Command.
Item 4. Javier mentioned is already solved - it's the responsibility of autowiring or explicit deps configuration.
I don't get item 5.: there is nothing to call "logger" since there is a constructor argument and a property.

The means the only remaining boilerplate is adding a constructor. That's something I'd be happy to accept if I gain dropping this mischievous "extends ContainerAwareCommand".

But then, there is item 6.: laziness.

We have some new solutions about that: service locators and getter injection could be to the rescue.
But that's to early to say for now.

I share the goal of this issue thought.

@nicolas-grekas item 4) is the problem to me. If I always think in terms of services (e.g. logger) why should I start thinking in terms of the class related to services? Where can I find the class for the service that I want? What's the class for the logger service? And if I use Monolog channels, what's the class for my monolog.logger.foo service?

@javiereguiluz I don't get how this is related to commands? You're just describing a problem that is solved by service configuration. Something very usual to SF devs, isn't it?

Let me explain with an example:

Traditionally I use this to get the logger inside a command:

$this->getContainer()->get('logger');

So easy! If you want the logger, get the logger service!

This proposal says that I must do this instead:

private $logger;

public function __construct(Logger???? $logger)
{
    $this->logger = $logger;
}

My problem is ... which class should I put in Logger???? Is it Logger ? LoggerInterface ? AutowirableLogger ? CommandLogger ?

And if I use channels (e.g. $this->getContainer()->get('monolog.logger.foo')), which class should I use? MonologLoggerFoo ? LoggerFoo ? etc.

In the same way you know that there is something you can "get" and which is called "logger" or "monolog.logger.foo": by reading some doc (which does not exist yet) - or by running a command that does service inspection.

$this->getContainer()->get('logger');

maybe that's easy, but that's hiding and (even worse) hardcoding dependencies - which is a bad practice really (not me saying that - just joining the main stream on that topic). If we don't seek for alternative ways that might have the potential to fix this "bad" part, while making DX as good as possible - same or better than current - I don't see how we can find any.

This issue is just about that to me - wondering about a better path.

My problem is ... which class should I put in Logger???? Is it Logger ? LoggerInterface ? AutowirableLogger ? CommandLogger ?

@javiereguiluz No matter if the typehint is LoggerInterface or Logger, you'll ends up with the very same logger service (except if you decided to redefine this in somewhere in your configuration). MonologBundle is responsible for the good autowiring of this service (it has been one of the first since it is a very common dependency).

About targeting a specific channel, I personally use monolog.logger tags for that, always injecting the main logger service. In this case that would require to explicitly register the command as a service, maybe it's not that bad since, to me, using channels is already an advanced usages of services (the fact it's not managed by autowiring enforces this fact to me). I can understand that this opinion is not shared though, and there's still a chance we can find a way to handle that.

A "more important" issue is parameters as mentioned before, that would require an explicit service registration too.
But if you ask me (understanding that it may not be shared too) avoiding this practice is worth forcing users to register their commands when these have dependencies. That's how dependency injection works and using a DIC as service locator is not good, as discussed already (commands should not have any special meaning regarding this).

Looking for a way to make this happen without loosing anything that should be possible to achieve .

I agree with both camps :) My question would be: do we really need to deprecate ContainerAwareCommand? If yes, why?

Playing the devil's advocate: we don't force people to use controllers as services, nor we intend to deprecate the base Controller class implementing ContainerAwareInterface AFAIK. So why should we enforce this for commands?
We should probably better educate people not to blindly use ContainerAwareCommand when not required. But the issue is ContainerAwareCommand still have valid usages right now, as running an Application means instantiating all the registered commands, along with their dependencies.
Thus, as long as this design is not changed to get lazy-loaded commands with something like #13946, deprecating ContainerAwareCommand doesn't look sensible to me.

New features like the service locators, or getter injection may help right now, but both are still experimental and require to design your commands "especially for it", rather than using classic dependencies declaration.

So IMHO, having lazy-loaded commands is a must have before deprecating this class.

do we really need to deprecate ContainerAwareCommand? If yes, why?

I'd say yes on the long run, because calling Container::get() should not happen as soon as there is really no need for.

Note that when extending this class, $this->getContainer() is just a shortcut for $this->getApplication()->getKernel()->getContainer(), so deprecating would just be giving less visibility to a bad practice and requiring more efforts to use it, which makes sense to me.

So IMHO, having lazy-loaded commands is a must have before deprecating this class.

Yet a big advantage, not a must have IMHO. ContainerAwareCommand and the container itself are actually overused is these commands. Also I think experimental features should be taken into account here, when command has such needs its design should reflect it I think; better have a wired typed getter than a getContainer() or a $locator than a $container :).

I would be very happy with commands where the constructor is 100% mine. Current issues:

• I need to set a name via my command constructor, which makes it impossible to be lazy
• Configure is called via the constructor

Way before there is any real intention to deprecate ContainerAwareCommand it is important to provide a documented "upgrade path" that provides examples of the new "best practices" (using auto-wiring or locators or whatever). After a few minor versions of the well-documented alternative, assuming no horrible backlash, it would then, possibly, be prudent to deprecate it, assuming doing such would be a net-gain.

With that said, Symfony has always been extremely capable while remaining non-opinionated and often providing multiple ways to achieve the same effect. I understand where the current mainstream thought is on hard-coding dependencies, but please take a moment to consider whether deprecating something is needed for the sake of maintainability (or some otherwise important reason), or is simply the act of being opinionated about how others should code. Please don't go the opinionated route of Laravel. This is one of the reasons why I love and have pushed every company I work for to use Symfony: it's non-opinionated.

To clarify, I'm not against changing and pushing new best-practices. I just don't think we should remove an alternate method altogether without reason.

It's too early. Let's close

@javiereguiluz

$this->getContainer()->get('logger');
So easy! If you want the logger, get the logger service!

My problem is ... which class should I put in Logger???? Is it Logger ? LoggerInterface ? AutowirableLogger ? CommandLogger ?

How do you use the logger service if you don't know the interface?

the main drawback of commands as a service is that all commands are eager-loaded to get all available command names. If this requires loading the object graph of all commands, it will be a huge nightmare (even more for cache:warmup and cache:clear) and will slow down all commands.
Autowiring does not help here. The only thing which could help is to use getter injection for all your dependencies (which can then be autowired though, to help you). but this makes writing commands much harder.

So I'm also on the side of not deprecating it for now.

@stof

the main drawback of commands as a service is that all commands are eager-loaded to get all available command names.

This is an issue I'd like to see solved either way. I'm using command as a service, but this greatly slows down our CLI functionality.

@javiereguiluz accessing services through $logger = $this->getContainer()->get('logger'); is fast and simple, but there is a big disadvantage: you don't really know what you get. How do you know if calling $logger->info() is valid since you don't know what is the result of getting from container? Yes, you don't know it, you only think you know it ;-)

You will say "hey, I know, because I know how MonologBundle works and I know there's logger service!". So if you know that you need to get logger from container, you also know that you could use logger in DI configuration to inject it into command. And then you get all the stuff you don't get when using ContainerAwareInterface - code completion, contract between your code and vendor. When you're getting something from container you have very loose contract because what you want can be there or not, can be what you want or not...

You can say then "hey, I can use /* @var $logger \Monolog\Logger */ and get code completion!". Yes, you can, but still it's only assumption, because you really don't know what's under 'logger' alias. With strict typehinting in constructor you will be 100% sure you get what you want. And it's a lot easier for unit testing since you must only mock used dependencies, not whole container.

Find the class associated with the service that you want. But how? Which class? Is it Logger? LoggerInterface? AutowirableLogger? etc.

I'm sorry, but in fact you're saying: "I want something, but I don't know what it is".

@Wirone I have a question for you. Imagine that I'm using Monolog channels. Today I use this:

$this->getContainer()->get('monolog.logger.foo')

With this proposal, I should use this:

private $logger;

public function __construct(XXXXXXX $logger)
{
    $this->logger = $logger;
}

The question is: which class should I put in XXXXXXX and please, tell me how did you find it out (without using PHPStorm of course ... lots of Symfony developers don't use that).

which class should I put in XXXXXXX and please, tell me how did you find it out

@javiereguiluz simple Psr\Logger\LoggerInterface is enough, there is no need to couple your code with Monolog details.

@unkind are you saying that, if I put this code:

private $logger;

public function __construct(\Psr\Logger\LoggerInterface $logger)
{
    $this->logger = $logger;
}

The logger is going to use the foo Monolog channel?

monolog.logger.foo is an inner working of how monolog saves the service ids. Imo you should not rely on those dynamically generated services ids. This is exactly where service locating is fragile. If you change your config to not use the foo channel anymore, this code will eventually break in production without knowing it.

@javiereguiluz Channels are an implementation detail of the logger: no matter which channel is used by the logger service you injects, your command must work. The only thing that makes sure your command works is that you're using the public api of Psr\Logger\LoggerInterface, which is guaranteed with an explicit LoggerInterface dependency, and is not using Container::get() since redefining the logger service to another class would make your command invalid after construction.

The logger is going to use the foo Monolog channel?

It looks like you're trying to find arguments against auto-wiring feature and not against DI for commands in general. I think that the primary goal of this issue is to get rid of the ContainerAwareCommand, configuration (auto-wiring, explicit configuration) is detail.

@iltar @chalasr @unkind none of you have answered my question yet 😄 Please, show me the full code needed to log messages in the foo Monolog channel (and without using ContainerAwareCommand). Thanks!

@javiereguiluz Actually:

class FooCommand {
    public function __construct(LoggerInterface $logger) {
        // ...
    }
}

And one of the following:

1. Inject the service created for this channel

App\FooCommand: ['@monolog.logger.foo']

2. Declare the channel to use with a tag

App\FooCommand: 
    arguments: ['@logger']
    tags:
        - { name: monolog.logger, channel: foo }

Of course this requires more effort than the current approach, but allows for well designed commands respecting good DI practices, that is widely worth it imho.

@javiereguiluz it doesn't matter if you use channel or not, at the end you will get \Monolog\Logger instance and you can use it for typehinting or use \Psr\Log\LoggerInterface (which Monolog implements) for decoupling your command/service from Monolog.

I could answer your question with question: so how do you use your logger after getting it from container, if you don't know what it is? Or if you know, what's the problem with typehint?

But here is full example of command as service with channel-based Monolog injection:

namespace Your\App\Bundle\Command;

use Psr\Log\LoggerInterface;

class FooCommand {
    /**
     * @var LoggerInterface
    */
    private $logger;

    public function __construct(LoggerInterface $logger) {
        $this->logger = $logger;
    }
}

# app/config/config.yml
monolog:
    channels: ['foo']

# src/Your/App/Bundle/Resources/config/config.yml
services:
    foo.command:
        class: Your\App\Bundle\Command\FooCommand
        arguments: ['@monolog.logger.foo']
        tags:
            - { name: console.command }

At any moment, if you would like to use some custom logger, you can configure DI to inject it to foo.command without any changes in command itself (note: this applies only if you typehint interface and your custom logger implements it).

I guess @javiereguiluz actually wonders how to achieve this by using autowiring, as autowiring was one of the proposed "replacement to get commands with dependencies without config for newcomers and RAD". You should either declare a dedicated type to use as typehint instead of LoggerInterface or configure the command dependencies manually.

But that's only a limitation of the autowiring for a very specific case, and doesn't really challenge the issue to me. Just use explicit commands as services definitions for such cases. Your command shouldn't be aware of the logger instance nor channel used.