CLI interface for minion tasks

[cyrrill]

• CLI_Command class is invoked from index.php to run Tasks
• Task factory takes a $name, and is executed with $params
  • CLI_Command::factory($task)->execute($params);
• Interfaces for standard I/O
  • CLI_Input
  • read()
  • options()
  • CLI_Output
  • write()
  • writeln()
  • CLI_Options
  • Handles Task selection and params collection
• Backwards-compatible with 3.3 Task interface

I tested the upgrade with an existing production application which contains several dozen tasks. All of the tasks ran correctly. Much more unit testing is needed in this module, as even before this PR it only had one test. We should add more to the branch as we go along.

[cyrrill]

@acoulton @WinterSilence @enov @kemo

I've done some refactoring of the Minion interfaces. Would greatly appreciate your review.

[WinterSilence]

@cyrgit What motivated your actions? Why introduce interfaces? There will be many different ways of IO? Very strange code, for example: Kohana_CLI::color - why not in class for output?

[cyrrill]

@WinterSilence These changes came about as per our conversation on https://github.com/kohana/minion/pull/102

The current Minion version breaks backward compatibility for tasks. As well, the factory method is coupling instantiation with execution, making reuse of Task classes within other Tasks clumsy.

The proposed changes allow us to separate out those factory parameters by using a name variable, rather than looking for the task name inside an array.

Please read the following comments: https://github.com/kohana/minion/pull/102#issuecomment-61472732

Why introduce interfaces? There will be many different ways of IO?

Dependency Injection

The current version is very tightly coupled among the classes, using static class methods directly, making it impossible to override any of the functionality.

The proposed changes allow for proper DI, so if you needed to change anything related to Input / Output, those changes would be independent of your Task and of the base Minion implementation.

For example, in a Task that is used for a cronjobs un-attended, you could provide an Output class such that write() sends an email, writes to a file, sends an SMS, pings an HTTP client, etc...

For the Input, your could choose to override read() with pulling data from a disk file, data from another locally running process, simulated user input, etc.... There is no need to restrict Tasks to only read from the command line explicitly.

[cyrrill]

Adding interfaces for Input and Output extensibility, is much better than tightly coupling all that behavior in a Task class, whose only concern should be the actual execute() task, and not how data is entered or sent out.

It is analogous to how there are Request and Response classes for Controllers. It is not the controller's concern how a response is rendered, or how the input parameters are parsed.

The proposed design incorporates a CLI_Command class as a wrapper for Task execution:

index.php ---->  Route ------> Controller (Request, Response)

index.php ----> CLI_Command ----> Task (Input, Output)

This is a well established design, which is also used in other libraries far superior to the current Minion implementation:

https://github.com/symfony/Console/blob/master/Input/InputInterface.php https://github.com/symfony/Console/blob/master/Output/OutputInterface.php https://github.com/symfony/Console/blob/master/Command/Command.php

    /**
     * Executes the current command.
     *
     * This method is not abstract because you can use this class
     * as a concrete class. In this case, instead of defining the
     * execute() method, you set the code to execute by passing
     * a Closure to the setCode() method.
     *
     * @param InputInterface  $input  An InputInterface instance
     * @param OutputInterface $output An OutputInterface instance
     *
     * @return null|int     null or 0 if everything went fine, or an error code
     *
     * @throws \LogicException When this abstract method is not implemented
     * @see    setCode()
     */
    protected function execute(InputInterface $input, OutputInterface $output)

[cyrrill]

Very strange code, for example: Kohana_CLI::color - why not in class for output?

I welcome your constructive opinions for improvements. This is a WIP which we can improve upon together. In this case, I chose to leave color() in the baseclass, as you could use that functionality in both the Input and Output classes. In the Output, it's evident, you'd want to echo display out in a different color. But then why in Input as well? Well, you'll notice the read() method has a prompt associated. You could choose to make that input prompt in whatever color you'd like as well. Keeping it in the base CLI class allows both streams to reuse it without duplication or tight coupling between the two (e.g. calling Output->color() inside Input->read(), would make it the dependency graph unnecessarily mixed)

[WinterSilence]

@cyrgit

The current version is very tightly coupled among the classes, using static class methods directly, making it impossible to override any of the functionality.

Where? We use empty classes for override, DI is other way.

It`s right steps (basically), but we must begin with core and not modules.

For example, in a Task that is used for a cronjobs un-attended, you could provide an Output class such that write() sends an email, writes to a file, sends an SMS, pings an HTTP client, etc...

It`s job for other modules

[WinterSilence]

@cyrgit no need duplicate interfaces (Kohana_%Interface_Name%)

[cyrrill]

@WinterSilence

The current version is very tightly coupled among the classes, using static class methods directly, making it impossible to override any of the functionality.

Where? We use empty classes for override, DI is other way.

https://github.com/cyrgit/minion/blob/3.4/develop/classes/Kohana/Minion/Task.php#L276

Task->execute() is coupled with a static call to Minion_CLI::write($view);

public function execute()
{
            ...
            // Display error
            $view = View::factory('minion/error/validation')
                ->set('task', Minion_Task::convert_class_to_task($this))
                ->set('errors', $validation->errors($this->_errors_file));
            Minion_CLI::write($view);
            ...

Such that if you wanted to override the ::write() method, you would have to extend:

class Minion_CLI extends Kohana_Minion_CLI {
            // override 
            public static function write($text){}
            public static function read($text = '', array $options){}
}

The problem is that since Minion_CLI handles not only Output but also Input, it would force you to use a class that overwrites the Input methods as well. You are not able to override write() and read() in different combinations, as they are coupled. Then for example, if I wanted to read input from the command line, but output to a file; the implementation of those methods would be locked into a static Minion_CLI::write() ::read() combination.

Splitting it up into Input and Output classes allows us to override each one independently, and they can be used in different combinations.

class CLI_Options extends Kohana_CLI_Options {
            public function read($text = '', array $options){}
}

class CLI_Output extends Kohana_CLI_Output {
            public function write($text){}
}

// $params comes from CLI_Options in the Command::factory()
public function execute($params)
{
            // CLI_Output
            $this->output->write($view, $params);
            ...

I find the extension of empty classes in the CFS to be quite useful, however this issue does not pertain to that behavior. You can of course override an entire Minion_CLI, but issue here is with methods that cannot be overriden independently of each other.

[WinterSilence]

@cyrgit you simply replace one problem another https://github.com/cyrgit/minion/blob/3.4/feature/cli-interface-refactor/classes/Kohana/Minion/Task.php#L110 https://github.com/cyrgit/minion/blob/3.4/feature/cli-interface-refactor/classes/Kohana/Minion/Task.php#L323

[cyrrill]

It`s right steps (basically), but we must begin with core and not modules.

Well, we have already started that in core. The Command --> Task execution pattern is analogous to the Request --> Controller pattern, in that Request and Response classes are injected to the Controller. Input and Output are injected to the Task in the same pattern.

I don't see a good reason not to extend the same established design to the modules?

For example, in a Task that is used for a cronjobs un-attended, you could provide an Output class such that write() sends an email, writes to a file, sends an SMS, pings an HTTP client, etc...

It`s job for other modules

Yes, I agree with you that the Minion module should not be implementing methods for those streams, but we are talking about the Interface( function write(){} ), not the implementations themselves.

[WinterSilence]

I don't a good reason not to extend the same established design to the modules?

Then need create basic interfaces Controller, Request, Response and use him as HTTP_Controller implements Controller, CLI_Controller implements Controller.

but we are talking about the Interface( function write(){} ), not the implementations themselves.

Why so complicate the code if it is not likely to be used?

You added a lot of interfaces, but most important is missing (interface Minion_Task).

[cyrrill]

Then need create basic interfaces Controller, Request, Response and use him as HTTP_Controller implements Controller, CLI_Controller implements Controller.

That used to be the design in the old versions of Minion:

https://github.com/Zeelot/kohana-minion/blob/1.0/develop/classes/controller/minion.php

class Controller_Minion extends Kohana_Controller

There were several discussions about the coupling between Minion and Controller, so the decision was taken to decouple them entirely.

You added a lot of interfaces, but most important is missing (interface Minion_Task).

The interface shouldn't be named "Minion_Task", the name needs to be more generic, probably "interface Task", so that it can be extended.

Minion_Task already exists in this form:

abstract class Minion_Task extends Kohana_Minion_Task {}

[WinterSilence]

Because Kohana_Controller is http controller, not abstract, but i`m talk about interfaces.

The interface shouldn't be named "Minion_Task", the name needs to be more generic, probably "interface Task", so that it can be extended.

it's not important, for example Minion_Task_Interface not used

@cyrgit you don't give answers on some questions.

[cyrrill]

I think that keeping the separation between Controllers and Tasks is important

Don't see the benefit in giving Tasks and Controllers a common ancestor, as they run mutually exclusive of each other depending on the current php_sapi().There is not much a Task could use from a Controller and viceversa.

The execution pattern of a Task and Controller may be analogous, but that doesn't mean they need to share an interface. Much the same that you don't give cache drivers and database adapters the same common interface, even when their class structure and usage is similar.

We could think of a top level interface for Command->execute() and Request->execute(), since they both are called as an entry point in index.php:

interface Executor {
    public function execute(); 
}

But beyond the outer ->execute() interface, HTTP and CLI are different realms.

[shadowhand]

I would strongly recommend using http://climate.thephpleague.com/ instead of rolling Yet Another terminal color solution.

[cyrrill]

@shadowhand Climate looks great. The color implementation in the code dates back to at least, b9b65e44ef861accb1b68836bf641892c4aaabf9 , would be better to use something more powerful as you indicate. Any suggestions as to how to best make it work with Minion?

[WinterSilence]

@shadowhand colored text is not so important

@cyrgit

I think that keeping the separation between Controllers and Tasks is important It is analogous to how there are Request and Response classes for Controllers. It is not the controller's concern how a response is rendered, or how the input parameters are parsed.

hm...

Much the same that you don't give cache drivers and database adapters the same common interface, even when their class structure and usage is similar.

I give - it`s need for DI

[shadowhand]

@shadowhand colored text is not so important

thanks for your wisdom, wise one. all i'm saying is that if color support is to be added, don't reinvent the wheel.

Any suggestions as to how to best make it work with Minion?

read the docs? ;) it should be as simple as dropping a use League\CLImate\CLImate statement at the top of the file that wants to use it. autoloading will be handled by Composer once the package is required.

[cyrrill]

@shadowhand Yes, that's what I was thinking to, install and autoload via Composer. What I meant was if you had suggestions for the class architecture, how to fit Climate into Minion such that it's decoupled well. We could just make use of Climate as the color provider for the CLI Output an Input classes, seems simple enough. I'll give it a go and make a commit for review. Thanks for the improvement

[cyrrill]

all i'm saying is that if color support is to be added, don't reinvent the wheel.

@shadowhand Just for clarification, this PR doesn't add the color functionality, it's same function that's been in Minion for a few releases now, it's just been moved.

I agree with you that using an existing good implementation of this would be best. I'll work on adding some support for that.

In addition the colorization, I am curious if you have any general feedback on the direction this PR is pointing? The general idea behind this code is to separate Task initialization from its execution. Running Tasks within a Command class allows us to build the Task instance and inject to it the I/O streams without having the Task class worry about it; giving us also the ability for that I/O to be extended where needed.

[cyrrill]

Adds CLImate support to Tasks. All the methods are available as follows:

abstract class Kohana_Task_Example extends Minion_Task {

    protected $_options = [
        'foo'  => 'bar',
        'opt' => NULL,
    ];

    /**
     * Example Task.
     *
     * 
     * @return void
     */
    protected function _execute(array $params)
    {
        $data = [
            [
              'PHP',
              phpversion(),
            ],
            [
              'Date',
              date(DATE_RSS),
            ],
            [
              'Params',
              implode(' ', $params)
            ]
        ];

        $this->output->whiteTable($data);

        $progress = $this->output->yellowProgress()->total(100);

        for ($i = 0; $i <= 100; $i++)
        {
            $progress->current($i);

            // Simulate something happening
            usleep(40000);
        }

        $this->output->error('Ruh roh.');
        $this->output->comment('Just so you know.');
        $this->output->whisper('Not so important, just a heads up.');
        $this->output->shout('This. This is important.');
        $this->output->info('Nothing fancy here. Just some info.');     
    }
}

[shadowhand]

any general feedback on the direction this PR is pointing?

In general, I'm for it. Interface contracts are imperitive for building systems that can be properly spec'ed and dependency injected. At the same time, I am completely unconvinced that Minion is superior to Symfony's Console component, which has a more consistent interface, better output options, and a more robust argument/option system. Does Minion offer anything different? Anything better? In my opinion, the answer to both of these questions is "no", and it forces my code to be more tightly couple with Kohana, which is a detriment.

tl;dr: Minion is inferior to alternatives, it should just go away.

[cyrrill]

@shadowhand I agree that Symfony/Console has much better code than the current Minion; but by the same token we could say that the Symfony framework or the Laravel framework have much better code than the entire Kohana framework, therefore the Kohana framework should just go away. Which is in fact the decision you took by abandoning the project in favor of Ohanzee.

Now, while I would just love it if my production apps magically transformed themselves into Laravel, that's just not going to happen for many of the apps I need to provide support for, which in practical daily terms it means I am "stuck" with Kohana, at least for those projects at the moment. Therefore I cannot just simply throw away Minion, since many of the core CLI-scripts in these apps run on top of it. Yes, I could refactor all the code to use Symfony/Console, but thats a pain for now. I'd rather spend some time improving Minion itself, at least to the point where it's extensible enough.

[enov]

Exactly.

On 11/10/2014 01:03 PM, Cyril Graze wrote:

@shadowhand https://github.com/shadowhand I agree that Symfony/Console has much better code than the current Minion; but by the same token we could say that the Symfony framework or the Laravel framework have much better code than the entire Kohana framework, therefore the Kohana framework should just go away. Which is in fact the decision you took by abandoning the project in favor of Ohanzee.

Now, while I would just love it if my production apps magically transformed themselves into Laravel, that's just not going to happen for many of the apps I need to provide support for, which in practical daily terms it means I am "stuck" with Kohana, at least for those projects at the moment. Therefore I cannot just simply throw away Minion, since many of the core CLI-scripts in these apps run on top of it. Yes, I could refactor all the code to use Symfony/Console, but thats a pain for now. I'd rather spend some time improving Minion itself, at least to the point where it's extensible enough.

— Reply to this email directly or view it on GitHub https://github.com/kohana/minion/pull/104#issuecomment-62369738.

[cyrrill]

@WinterSilence I've addressed some of the valid concerns you brought up with regards to DI. Instead of the Task using a static factory necessitating the concrete object of the provider, it now takes a Closure which acts as a builder.

That allows the Task class to be independent of View, Validation, CLI_Output, etc

Now it's just bound to the interface of these objects so that they can be injected by Command dynamically.

I also eliminated the unnecessary Client class, and implemented execute() directly in the Command. This cleans up the interface quite a bit.

To execute Minion, you would now have this index.php:

    $options = CLI::factory('Options');
    $output = CLI::factory('CLImate');

    Command::factory(TRUE, $options, $output)->execute();

You can overwrite the I/O streams, and the TRUE parameter indicates auto detection of "--task" from Options.

[shadowhand]

@crygit I understand your stance, but the effort put into this could be put into migrating your minion tasks to Symphony Console. That didn't mean your Console apps have to be free of Kohana code. Mine aren't. :)

[cyrrill]

It would require a lot more effort to rewrite all those tasks, there are a lot of them. And it wouldn't be just me, it would be anyone with a legacy Kohana app they need to support. It's easy to suggest to everyone to use another tool, but clients are not willing to invest in that without a pressing need.

The business reality is that tons of sites out there are not going to jump ship and rewrite their apps in another framework immediately. It's a process that will take time, maybe even years, so we might as well provide a better tool chain in the meantime.

This PR maintains backward compatibility for all those tasks written in 3.3, while providing the I/O improvements seamlessly. I've already replaced it on one large site I maintain, and so far so good.

At this point I don't think the implementation needs much more work, besides any design improvements other people may want to address. I'm happy to contribute this fork, as it's work I needed to do anyways. I look forward to finding at least some of this in use by the community.

[cyrrill]

@shadowhand

Well, it seems nothing will happen on this front. If this project is so closed up, it refuses new contributors, no sense in putting more effort here.

Good luck moving Ohanzee out of the dark basement it lives in. With Kohana broken and abandoned, no one in this planet with any programming sense will invest in Ohanzee either. Unless the main publicly known project rescues its image, I don't see how this project has any chances of success. With K4.0 effectively neutered, and core developers wasting their time defending static global vars, it makes it abundantly clear this project has no future.

It would be like Yugo coming out with a 2015 model, you think there will be any buyers?