Btw what are some use cases for using DI in console commands? I'm wondering for example if it's possible to use DI already in console commands if the command logic is in a separate class that uses DI.

So would you be OK with the idea and me creating a new Piwik/Console component that would make the Symfony/Console more flexible?

The idea sounds good to me but i'll leave it to @diosmosis and @tsteur to comment.

If the logic is in a separate class using DI, then you have to get an instance of that class. You can either call StaticContainer::get($theClass) (which is not so good) or use dependency injection in the command to inject the class you want to call. DI everywhere is simple and consistent. The fact that we don't have DI in commands today (or other root classes) hinders using DI in other classes because it makes the whole thing more complex.

If the logic is not in a separate class, then ideally it should be. But if we are realistic, that's not the case at all in Piwik's codebase, so having DI in commands is also useful for that (non-negligible) scenario.

In short, it's like DI in controllers.

I've created the component as a private repository for now so that you can all get a better idea: https://github.com/piwik/component-console

It would be ok for me if the definition was still the Symfony way. That it resolves all dependencies would be ok for me unless it actually becomes a problem.

For commands where logic is not in a separate class yet we can refactor this when working on that command anyway. I wouldn't care about it otherwise.

@mattab I think the main use case is for classes like this: https://github.com/piwik/piwik/blob/master/plugins/CoreAdminHome/Commands/FixDuplicateLogActions.php

You could say, move the logic to another class, but if you look at the code, you'll see that most of the logic is for timing operations and printing out status updates. It would serve no purpose to move those to an extra class, since that class would never be reused.

@tsteur Sure we could give it a try, but from my experience we'll end up there eventually. Any problem/misconfiguration in DI would prevent the CLI console from even starting. E.g. impossible to clear faulty caches with core:clear-caches (because nothing runs). And what if a command requires something that does a check (e.g. database is UP, or Piwik is setup, or Piwik can be accessed, or can connect to Redis, or file is writable, or…): again any failure will prevent the application from running at all.

So theoretically there shouldn't be any problem, but in reality if we ship that we will be dealing with weird issues sooner or later.

I've extended the Readme of piwik/component-console:

But the main problem here is that command classes need to be instantiated to be registered. That means that dependencies injected in your commands will be resolved and initialized (and dependencies of those dependencies). There are several problems with that:

• a large part of your application might be instantiated for nothing
• any misconfiguration in your DI container will prevent the application from starting at all
• any other error happening in the init of your objects will prevent the application from starting at all
• admin and troubleshooting commands cannot be run if the application doesn't start (e.g. clear caches, start/stop server, show debug information, install the app, disable modules, …)
• any logic in your object's initialization will be run, which may require external services (database, remote cache, webservice, …) or may have side-effects

Regarding that:

It would be ok for me if the definition was still the Symfony way.

How would you do it? I don't see a way to call an object method without instantiating the object.

It would be ok for me if the definition was still the Symfony way.

@tsteur I understand that changing the way commands are configured is not ideal, but I don't see an alternative.

I've added additional stuff to prevent users from making mistakes, see https://github.com/piwik/component-console/commit/368041cc00e88601fb0a6c124fedc0499ce11759 That should help a lot because it is now impossible to register a command if it's not correctly implemented.

Please bear in mind that current commands are not affected and continue to work.

This solution isn't backwards compatible, is it? Also we do usually perform such checks rather only in development mode if possible to "silently" ignore such errors on production systems so incompatible plugins don't break everything. See eg. https://github.com/piwik/piwik/blob/master/core/Plugin/Tasks.php#L152

For me the problem is that we'd have to provide and update documentation, etc. It's very convenient to simply link to their documentation which is quite good. Also it is less intuitive for developers etc. but that's not a big deal since we generate commands based on templates. I didn't have a look at the code yet... Is there no way to execute eg dependencies via setters only before execute is executed? Haven't thought about it so there might be other problems eg it wouldn't be intuitive to only have dependencies in execute method. If they'd need a dependency in configure method (which they should not need) they could still request a constructor dependency but it'd be not intuitive either. On the other side this follows our design principle The complexity of our API should never exceed the complexity of your use case.. Eg we could write in the templates for the command generator:

    protected function configure()
    {
        // here you cannot access any dependencies. If you do need a dependency request them via __construct()
        $this->setName('yourplugin:commandname');
    }

I'm just thinking about random solutions so not saying any of this is good as I like to think about many different solutions to be able to pick the best. Maybe there is another way? Otherwise I do not really mind going with the proposed solution but would be nice to keep the API

For the record it is backward compatible as current commands are not affected and continue to work. ConsoleCommand is not changed.

The complexity of our API should never exceed the complexity of your use case.

I'd say the "more complex" solution should be used when DI is needed, and in other cases there would be no changes.

@tsteur @diosmosis what are your thoughts on this: should we merge, or is there more things to do, or do we maybe not need this yet? it's set to 2.12.0...

@mnapoli Here's an idea to merge Piwik\Console\Command w/ Piwik\Plugin\ConsoleCommand, instead of having two classes, maybe one base can have a static configure(...) method where the base just creates a new instance of the derived class, like:

class Command extends Symfony\Command
{
    public static function configure(CommandConfiguration $config)
    {
        $derivedCommandClass = $config->getCommandClass();
        $derivedCommand = new $derivedCommandClass();
        $config->loadFromCommand($derivedCommand);
    }
}

so commands that don't define the static method won't have DI, but commands that do can use DI.

@mattab I like the idea of DI being available in commands, and I'm ok w/ this solution. I does make things a bit more complicated for plugin developers (though it doesn't have to if we support both ways, like the pull request currently does). Only potential issue I can see is some possible confusion in the dev docs.

Maybe we can merge it but not document it (and not make it public API) for now so we will be able to change it in a while when/if we find another solution? I don't think it is currently very much needed so that should be ok

For the record I have submitted a pull request to Symfony to improve the Console with those changes and solve the problem of "It's not the Symfony API".

If it was submitted to Symfony would it make sense to close the PR or we still need it opened?

If we close the issue we won't have DI in commands. Symfony 2.7 feature freeze is today if I'm not mistaken, so the PR will not be merged before Symfony 3 if it is ever merged (which requires PHP 5.5 anyway). So that means we will never be able to use it.

We need to either make a decision on how to implement this, or choose to not implement it at all.

A summary of the possible solutions:

• current implementation (with or without @diosmosis suggestion because I don't think it changes the problems raised in the discussion)
• we also choose to use the PR I made to Symfony (slightly different implementation)
• injection in properties (or with setters) using annotations (or config) and lazy injection: setting up lazy injection for every service will be so much work and a risk for fails, and it was decided to not use annotations anyway: I say bad idea
• other?

Summary of the problems raised with the current implementation:

It would be ok for me if the definition was still the Symfony way

Impossible with the current Symfony way

This solution isn't backwards compatible, is it?

It is, the current commands still work and are not deprecated

For me the problem is that we'd have to provide and update documentation

Also it is less intuitive for developers

Summary of the problems raised with the current implementation:

Thanks for providing summary. considering the points being raised, let's postpone the PR for now and maybe re-visit in the future. Although we don't add DI in commands we will for sure continue to add DI in key places in Piwik 👍

I'm closing it rather than set to mid term (but we can re-open if needed)

For the record we've hit the problems I was describing here (more precisely in this comment) in #7759 (non-lazy commands can crash the console if they contain any error in their initialization)