Skip to content
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

[BC BREAK] Full blown DI for console commands #50

Closed
wants to merge 6 commits into from
Closed

[BC BREAK] Full blown DI for console commands #50

wants to merge 6 commits into from

Conversation

foglcz
Copy link
Contributor

@foglcz foglcz commented Feb 9, 2016

Type Feature
Fixes issues fixes #46, fixes #48
Documentation updated
BC Break YES
Tests updated yes

Updated the ConsoleExtension, so that it uses Nette\DI for parsing individual commands.

Therefore, full-blown Nette DI is possible - you can set inject and autowire and all options on every single individual service. However, this is a BC break, because previously all commands were not autowired and were injected instead.

This change, alongside with the documentation update, should adhere to nette coding practice of defining all dependencies on constructor level. Even though symfony recommends to use arbitraty configure method, using constructor is cleaner design IMHO.

edit see this link for new abilities in config.neon

@@ -24,6 +24,25 @@ The best way to install Kdyby/Console is using [Composer](http://getcomposer.or
$ composer require kdyby/console
```

Then enable the extension in your `config.neon`:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want this in the readme, it adds work for me to maintain two documents :-/

@fprochazka
Copy link
Member

This change, alongside with the documentation update, should adhere to nette coding practice of defining all dependencies on constructor level.

I disagree about this one. I don't wanna prevent it, but I also don't wanna ecourage it.

@fprochazka
Copy link
Member

Even though symfony recommends to use arbitraty configure method, using constructor is cleaner design IMHO.

I would like the avoid this discussion entirely and let everybody do it as they like. The configure method is a standard for the sf console and I don't wanna go against the documentation here. The command configure section can get quite long and having it in separate method (which is in fact called from the parent constructor) is a good thing IMHO.

@fprochazka
Copy link
Member

Also, having them autowired should not cause any applications to break. Is there anything else that can be considered a BC Break?

@foglcz
Copy link
Contributor Author

foglcz commented Feb 10, 2016

So I see multiple stuff being discussed here, let's consolidate.

  1. Docs. Your call - will update.
  2. Constructor injection - What is your preferred approach? Let me know, I'll update.
  3. __construct vs configure -- agreed, I'll update the docs

  1. BC break
    the PR fix actually breaks using @inject and injectSomething() methods. Since the autowire happens by default on constructor, unless you define inject: true in your config.neon, the inject annotations & methods are not called.

As far as I understand it, this happens because I used Nette\DI\Compiler::parseService($def, $command); For some reason, it does not call the injectors or does not inject the properties.

Can be seen here:

@foglcz
Copy link
Contributor Author

foglcz commented Feb 10, 2016

BC break add - OH, I see, injects are called in doRunCommand.

But that means, if I inject a command into another command using DI, I won't have injects called properly. Maybe it would be cleaner to just remove the doRunCommand override from Application.php completely? That way DIC would take care of injects, as it should, imho.

@fprochazka
Copy link
Member

Since the autowire happens by default on constructor

The construtor injection always worked, you just haven't had the full blown capabilities of configuring the service as in the services section.

As far as I understand it, this happens because I used Nette\DI\Compiler::parseService($def, $command); For some reason, it does not call the injectors or does not inject the properties.

to resolve the inject methods/properties is the job of InjectExtension, which must be enabled by the tag.

@fprochazka
Copy link
Member

But that means, if I inject a command into another command using DI, I won't have injects called properly.

Yes, that's right, the "propper" way to invoke the command (if you must) is calling it through application. As there might be more to it's initialization (events, helpers, ...).

Or you know... you extract the logic so you don't have to hack it :)

@fprochazka
Copy link
Member

Sorry, but I don't wanna introduce any more ways of doing the same thing. If you wanna better configuration of command services, just configure them as services and properly tag them.

@fprochazka fprochazka closed this May 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command cannot depend on other command DI injection for commands?
2 participants