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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2a07c97
Feature: All console commands are now autowired
foglcz Feb 9, 2016
e147480
Feature: console commands: section is now parsed same as standard net…
foglcz Feb 9, 2016
2884357
Feature: new section console.helpers in config.neon, for defining SF …
foglcz Feb 9, 2016
c8182af
Feature: Added full-blown Nette DI to Kdyby/Console
foglcz Feb 9, 2016
10e2002
Feature: updated documentation for the new DI
foglcz Feb 9, 2016
efb42d7
Feature: Updated index readme to match new configuration options
foglcz Feb 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 :-/


```yml
extensions:
console: Kdyby\Console\DI\ConsoleExtension
```

And append your commands into `console` section:
```yml
console:
disable: false # optional, can be used to disable console extension entirely
helpers: # optional, helpers go here
- App\Console\FooHelper
commands: # define your commands in this section. Full Nette DI is supported.
- App\Console\SendNewslettersCommand
- App\Console\AnotherCommand
- App\Console\AnotherCommand2
```


Documentation
------------
Expand Down
97 changes: 53 additions & 44 deletions docs/en/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,66 +1,83 @@
Quickstart
==========
kdyby/console
Copy link
Member

Choose a reason for hiding this comment

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

Please leave the title as it was before

=============

This extension is here to provide integration of [Symfony Console](https://github.com/symfony/console) into Nette Framework.
This extension provides integration of [Symfony Console](https://github.com/symfony/console) into [Nette Framework](https://www.nette.org).

It allows you to create command-line commands directly within your application. These commands can be used for recurring tasks, as cronjobs, maintenances, imports and/or big things like sending newsletters.


Installation
-----------

The best way to install Kdyby/Console is using [Composer](http://getcomposer.org/):
Fastest way is to use [Composer](http://getcomposer.org/) - run following command in your project root:

```sh
$ composer require kdyby/console
```

You can enable the extension using your neon config.
Minimal configuration
---------------------

```yml
First register new 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.

maybe better would be "register the extension" ? not sure about this one


```
extensions:
console: Kdyby\Console\DI\ConsoleExtension
```


Minimal configuration
---------------------

This extension creates new configuration section `console`, the absolute minimal configuration might look like this
This creates new configuration section `console`, the absolute minimal configuration might look like this:

```yml
console:
url: http://www.kdyby.org
```

The `url` key specifies reference url that allows you to generate urls using Nette `UI\Presenter` in CLI (which is not possible otherwise). Another useful key is `commands` where you can register new commands. Look at the [Extending](#extending) part.
The `url` key specifies reference url, that allows you to generate urls using Nette `UI\Presenter` in CLI (which is not possible otherwise).
Copy link
Member

Choose a reason for hiding this comment

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

this is gonna need more rewriting, but that's definitely out of the scope of this PR


Now, your nette installation is ready to run commands! Try it:

```sh
$ php www/index.php
```

Writing commands
----------------

Commands are like controllers, but for Symfony Console. Example command might look like this
Example command might look like this:

```php
namespace App\Console;

use App\Models;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

class SendNewslettersCommand extends Command
{
protected function configure()
/** @var Models\NewsletterSender */
private $newsletterSender;
Copy link
Member

Choose a reason for hiding this comment

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

this was definitely bad, but I really don't wanna encourage constructor injection in console commands

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allright, recommended approach would be using @inject?

(I hate it but that's a matter of preference :) )


/**
* @param Models\NewsletterSender $sender
*/
protected function __construct(Models\NewsletterSender $sender)
{
$this->setName('app:newsletter')
->setDescription('Sends the newsletter');
parent::__construct('app:newsletter'); // <-- run with `php www/index.php app:newsletter`
$this->setDescription('Sends the newsletter');
$this->newsletterSender = $sender;
}

/**
* @param InputInterface $input
* @param OutputInterface $output
* @return int
*/
protected function execute(InputInterface $input, OutputInterface $output)
{
$newsletterSender = $this->getHelper('container')->getByType('Models\NewsletterSender');

try {
$newsletterSender->sendNewsletters();
$output->writeLn('Newsletter sended');
$this->newsletterSender->sendNewsletters();
$output->writeLn('Newsletter sent');
return 0; // zero return code means everything is ok

} catch (\Nette\Mail\SmtpException $e) {
Expand All @@ -71,29 +88,25 @@ class SendNewslettersCommand extends Command
}
```

The configure method is to name the command and specify arguments and options.
They have a lot of options and you can read about them in Symfony Documentation.

When the command is executed, the execute method is called with two arguments.
First one is command input, which contains all the parsed arguments and options.
The second one is command output which should be used to provide feedback to the developer which ran the command.
In `__construct`, we setup the name of the command, then set some description and set all dependencies.
Copy link
Member

Choose a reason for hiding this comment

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

In __construct, we setup the name of the command

where have you found this information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I do constructor dependencies as a matter of religion. :)

When you override the method, autogenerated stub contains parameter $name.
So I tried setting the name directly there, and it worked, so I figured that's an approach, which is there for some reason. for a very, very, very long time, and 3.0 did not remove it.

Also, I don't like using arbitraty configure() for no reason, as it is called from constructor and these days, it serves no purpose other than BC.
Admittedly, it was useful in first versions of console - since those really "prepped" the command for use.

Neither of which is reflected in Symfony documentation. I suppose I should also create a PR for SF/console :)

Copy link
Member

Choose a reason for hiding this comment

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

I disagree about the configure method #50 (comment).

Anyway, this PR is tryting to solve too many issues at once, making it hard to discuss them all at once and come up with a solution :-/


Every command contains an `execute` function, which is called by Symfony console, whenever the command should be executed. The arguments handle either input parameters, and/or allow you to write to output stream.

Best practice is to return an exit code, which specifies if the command ran successfully or not. This code can be read by other applications, when they execute your app. This is useful for cronjobs.

Best practise is to return an exit code which specifies if the command ran successfully and can be read by other applications when executed.
Every command needs to be registered in commands section of `config.neon`:

```yml
console:
commands:
- App\Console\SendNewslettersCommand
```

Extending
---------

To add a command, simply register it as a service with tag `kdyby.console.command`

```yml
services:
newsletterCommand:
class: App\Console\SendNewslettersCommand
tags: [kdyby.console.command]
```

Alternatively you can use shorter syntax for registering command (without tag). It's useful when you have a lot of commands:
To add a command, simply register it inside of the `console.commands` section:
Copy link
Member

Choose a reason for hiding this comment

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

I like that you encourage registering it through the console section, but you've completely removed the part about the tag, which is usefull for 3rd party extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe a new section or a specific note then? Will update the docs accoardingly.

Also, is this used and/or do you want to encourage this? Because if I'd write a Nette extension, that uses console, I'd include adding it to config.neon into my extension readme. IMHO update of config.neon should be best-practice of nextensions, shouldn't it?

My personal opinion is: do not use tags, but keep them for BC. Since all commands are also available as a service, you can inject them into your presenter/whatever and use them, as I did:

BTW -- do you want to add this to docs? Could add a section Calling command from a command, which would also explain the DI.

class CleanupFocus extends Command
{
    /** @var Paths */
    private $pathsCommand;

    public function execute(InputInterface $input, OutputInterface $output)
    {
        // ...
        $frameInput = new ArgvInput(['actionname', '--all'], $this->pathsCommand->getDefinition());
        $this->pathsCommand->execute($frameInput, $output);
        // ...
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

IMHO update of config.neon should be best-practice of nextensions, shouldn't it?

Definitely not, if you install an extension, you don't wanna copy boilerplate to your config - one line with extension registration should be enough in most cases. It should just work™.

Copy link
Member

Choose a reason for hiding this comment

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

My personal opinion is: do not use tags, but keep them for BC.

Tags are a big part of binding extensions together, even in symfony. I'm all for encouraging the end-users to add commands in config section, but the tags are a vital part to it.

Copy link
Member

Choose a reason for hiding this comment

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

Since all commands are also available as a service, you can inject them into your presenter/whatever and use them, as I did:

Let's be hones, that's a pretty nasty hack (which I've also commited myself several times). The clean solution to this is to extract the logic to some facade and use that facade in both commands.

Could add a section Calling command from a command, which would also explain the DI.

I think I've already answered this one :)


```yml
console:
Expand All @@ -103,14 +116,10 @@ console:
- App\Console\AnotherCommand2
```

This is called anonymous registration (look at hyphens). You can name your command (`newsletterCommand: App\Console\SendNewslettersCommand`) but mostly it's not necessary.

To add a helper, simply register it as a service with tag `kdyby.console.helper`

If you want to add a new [console helper](http://symfony.com/doc/current/components/console/helpers/index.html), use following syntax:

```yml
services:
fooHelper:
class: App\Console\FooHelper
tags: [kdyby.console.helper]
console:
helpers:
- App\Console\FooHelper
```
18 changes: 8 additions & 10 deletions src/Kdyby/Console/DI/ConsoleExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,18 +82,16 @@ public function loadConfiguration()
Nette\Utils\Validators::assert($config, 'array');
foreach ($config['commands'] as $i => $command) {
$def = $builder->addDefinition($this->prefix('command.' . $i));
list($def->factory) = Nette\DI\Compiler::filterArguments(array(
is_string($command) ? new Statement($command) : $command
));

if (class_exists($def->factory->entity)) {
$def->class = $def->factory->entity;
}

$def->setAutowired(FALSE);
$def->setInject(FALSE);
Nette\DI\Compiler::parseService($def, $command);
$def->addTag(self::TAG_COMMAND);
}

isset($config['helpers']) ?: $config['helpers'] = [];
foreach ($config['helpers'] as $i => $helper) {
$def = $builder->addDefinition($this->prefix('helper.' . $i));
Nette\DI\Compiler::parseService($def, $helper);
$def->addTag(self::TAG_HELPER);
}
Copy link
Member

Choose a reason for hiding this comment

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

I need the disabled inject here, it's an optimalization - it's meant to allow you get your sevices using property injection which is called in runtime Kdyby\Console\Application::doRunCommand() so you don't have to initialize the dependency graph of all commands which are not gonna be run anyway.

Copy link
Member

Choose a reason for hiding this comment

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

We can discuss if it's needed or not, but I would like to leave it out of the scope of this PR, as I like the parsing the service using the "smart way".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, isn't the service graph initialized only when parsing config.neon and generating the container? The injects should be part of the generated code, shouldn't they?

If it's during the generation only, it doesn't matter if the graph is or isn't init'ed -- it should be in fact better this way, as the container code will match the use-case of your command more exactly.

Copy link
Member

Choose a reason for hiding this comment

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

I think you misunderstood me.

If you run the app through console, Sf application takes over and loads all commands to ask them about their names and descriptions (at least) - this list is not cached and is fetched over and over in runtime. Which means the command has to be instantiated - which means all it's constructor dependencies have to be instantiaded, and if it has any inject properties or methods, that were wired in compile-time, they're now part of instantiating that service and will be also instantiated in memory. Which means all their dependencies are too.

If you're following the rule to have light constructors, this shouldn't be a big deal for you, as it only creates "a few" (depending on the console command dependencies) services. But it has other problems, or you might be using some library that doesn't have exactly light constructors.

Having the inject disabled "by force" means that if you use inject properties (or methods) for the console commands, they will be resolved and injected on runtime. Which means a fraction of your object graph will be created. Unlike when all the commands use strictly constructor injection.

That's a choice you can make by simply using a constructor or inject properties. Both will work. Both has advantages/disadvantages. It's up to the programmer to choose one.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, that should be out of the scope of this PR. We can continue this discussion in separate thread (if you still think that it would be good to change it). I would like to make more smaller steps with PR's.

}


Expand Down
Loading