_execute() requires the declaration of $params

[cyrrill]

As can be seen in line 281, the execute() method is calling _execute() with the validated $options obtained from CLI.

We must declare this argument in the abstract function so that overriding Task classes are able to receive this array.

// Finally, run the task
$this->{$this->_method}($options);

The implementation in Minion has always been that "$this->_options" represents the list of available parameters with their default values. In runtime, "$params" represents the validated input from the user, or simply the default values in lieu of any input.

I don't see a reason to introduce an API breaking change as part of this refactor.

[acoulton]

+1 but we should maybe wait for the discussion in #92 / f9f4d47 before merging - we may need to update the docs/and or mark the $params argument as deprecated if there was a good reason for removing it.

[cyrrill]

Hi @acoulton, it seems the proposal has been accepted in #92's discussion. How should we go about updating the documentation?

[cyrrill]

@acoulton, I've now updated the documentation to reflect $params

[acoulton]

@cyrgit thanks. This looks good to me, but I can see @WinterSilence's point that having both $this->_options and $params is potentially confusing.

Seems the options are to leave it as he has it, forcing the BC break (-1 from me), revert (+1 from me) or revert but mark that style as deprecated.

@enov what do you think?

[WinterSilence]

An error occurred because I adapted code of my module https://github.com/WinterSilence/kohana-cli for backwards compatibility with kohana/minion, but it turned out not to end. kohana-cli is simpler and easier, but it`s not compatible with the minion tasks, so I did not offer to replace and tried to transfer some ideas.

[cyrrill]

What about keeping a separation between defaults and runtime parameters? Such that $this->_options never changes (remains as an indication of options available), and whatever is passed in CLI gets sent in through $params.

This allows _execute() to distinguish if the Task is being run with default parameters, or ones sent by the user. When set_options() runs, it overwrites the original defaults the class had defined, making it impossible to know what the default was.

It could be thought similar to a DB insert. You have the allowed columns, and the default values in the schema. When an insert runs, the parameters are checked against allowed columns.

I seriously do not think there will be confusion between $_options and $params, as it is clearly documented and has been working like this forever.

[WinterSilence]

@cyrgit Why do we need to store the default values​​? For example, Route does not it.

[cyrrill]

If the user passes an invalid parameter to CLI, the Task could choose to issue a warning and continue by using the default value. If the default value is overwritten by the factory, we have lost it even before reaching _execute().

Silly example:

~$ minon mrpotatohead --help

Create a Mr. Potatohead by giving me some parameters --head_color A color for it's head [brown, green, yellow], default green --eyes_count The number of eyes to add [1-4], default 2

~$ minon mrpotatohead --head_color=brown --eyes_count=two

Oops, I was expecting a number for "eyes_count", would you like continue with the default value of 2 (Y/n)? y

...

[WinterSilence]

@cyrgit if eyes_count not valid then task generate error before executing. I dont seen the console programs that would use something like that. Impossible to foresee everything, so module implements only what is most commonly used.

[cyrrill]

It's just an example, look even at Task Demo in the documentation, it uses a similar scenario to prompt the user. Think of a program that prompts a user for a file location to save the output. If the location is non-writable, it could revert to the default (say /tmp).

Another case could be that since PHP is not strongly typed, it could be useful to retain the default value in order to ascertain the variable type of $_options.

In any case, these are just an examples, the main concern is with keeping separation between what is initially declared in the class code, and what the user is passing in at runtime.

And of course, the prime point here is, why break the API backwards compatibility for a change that presents no huge advantages?

Anyone looking at the interface, phpdoc, or userguide can very easily see how this works, this is hardly an advanced algorithm. It's quite obvious that if you are to implement a method with signature: _execute(array $params), you would understand what $params stands for. I just don't buy the confusion argument at all.

[acoulton]

@cyrgit I see where you're going with the defaults being available - for example, the various openssl programs that prompt with a default if an option isn't provided on the CLI. However, your proposal still wouldn't fully support that, because the $_options defaults are also merged into $params, so you can't easily tell whether a user has explicitly provided the default argument, or left it out. So you'd need some separate storage of the defaults, and probably to handle all that before parameter validation, rather than in execute - unless you're going to do validation twice.

It's also probably not a common requirement - minion's not really the best choice anyway for complex interactive CLI applications.

If we were going to do it, we should rename $_options to something else - $_defaults perhaps - to make the BC break obvious for anyone who's already accessing that property in their tasks.

But I don't think it's necessary to have that partial solution for an infrequent problem in core.

For me it's a simple decision - does having $this->_options and $params with the same data have any downside or risk confusion? More I think about it, the more I think it could do - not because it's complex per se, but just because it's generally a bad idea to have two copies of the same data in scope.

I think we should deprecate - but not remove - $params.

[cyrrill]

@acoulton Thanks for the remarks. As I posted above, I'm aware of the mixing issues, and that this change alone doesn't fix it:

When set_options() runs, it overwrites the original defaults the class had defined, making it impossible to know what the default was.

I am proposing to make another commit on top of this one to resolve that issue. In other words, to never mix $params and $this->_options. That would eliminate the whole argument of having 2 copies of the same data.

minion's not really the best choice anyway for complex interactive CLI applications

Minion is the CLI tool of choice for those using Kohana, so it would be good to strive to make this tool better. I mean we could always use something from the packagist ecosystem that's better, but I'd rather try and improve Kohana's own toolchain where possible. That's what we're here for, no?

I insist that having runtime arguments separate from the default definition to be the best design. For example, look how Symfony's beautiful Console class handles this:

http://symfony.com/doc/current/components/console/introduction.html

class GreetCommand extends Command
{
    protected function configure()
    {
        $this
            ->setName('demo:greet')
            ->setDescription('Greet someone')
            ->addArgument(
                'name',
                InputArgument::OPTIONAL,
                'Who do you want to greet?'
            )
            ->addOption(
               'yell',
               null,
               InputOption::VALUE_NONE,
               'If set, the task will yell in uppercase letters'
            )
        ;
    }

    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $name = $input->getArgument('name');
        if ($name) {
            $text = 'Hello '.$name;
        } else {
            $text = 'Hello';
        }

        if ($input->getOption('yell')) {
            $text = strtoupper($text);
        }

        $output->writeln($text);
    }
}

You have your definition of arguments and defaults in one place. And then in execute() method, you have access to STDIN and STDOUT interfaces.

Defaults and runtime values are completely separate and don't get mixed into a mysterious protected variable. The execute command has an explicit argument called "$input" which makes it very clear which data it has to work with.

I understand refactoring this would require more consensus to extend the implementation, and doesn't have to be done just yet, but I really believe deprecating $params is a move in the wrong the direction. It would be best to leave it there, and instead to rework the internals of the class to handle the input processing without having the duplication.

I propose we keep $params there, as it would keeps the existing interface without breaking backwards compatability, and because it will allow us to improve the class to achieve a more solid design in the future.

[cyrrill]

In reality there really is no reason to have the set_options() method at all. Currently this is being run from the factory, but the factory method itself has a dirty interface, in which we are expecting an array key to be named "task" (or the fist value when no "--task" is used), and then everything else passed is a runtime argument.

It would be much cleaner to separate out the actual name of the task to run, from the runtime params, such that factory only needs to receive a string "$name" which indicates which Task we want to instantiate.

The rest of those values being passed, should not be included in the factory, but be given directly to execute().

Currently this is what we have in index.php:

Minion_Task::factory(Minion_CLI::options())->execute();

That is a bit dirty, the factory shouldn't have to dig inside an array to find which class it needs to instantiate. Something like this seems better:

$options = Minion_Task::resolve_options(Minion_CLI::options());

Minion_Task::factory($options['task'])->execute($options['params']);

Minion_Task::resolve_options() would do the work currently done by the dirty factory interface of finding our task name, and return any runtime parameters if any.

That way there is no need for the factory() method to need to call set_options(), mixing defaults and runtime values. Inside the execute() method, we would merge the defaults and runtime params, to get the actual execution values to be validated and dispatched to _execute($params).

public function execute(array $params)
{
    $params = array_merge($this->_options, $params);

    // Validate options
    $validation = Validation::factory($params);
    (...)
    $this->{$this->_method}($params);

Cleaning up the factory also allows us to be able to call Tasks from within other Tasks in a much cleaner way, without needing to hack up a combined array of Task name, and Task params. It would also allow us to instantiate a Task once with the factory, and run it multiple times with different parameters.

Currently

abstract protected function _execute()
{
    $subparams = ['task' => 'clean', 'file' => $this->_options['file1']];
    Minion_Task::factory($subparams)->execute();

    $subparams = ['task' => 'clean', 'file' => $this->_options['file2'], 'notify' => true];
    Minion_Task::factory($subparams)->execute();

}

With proposed interface

abstract protected function _execute(array $params)
{
    $task = Minion_Task::factory('clean');
    $task->execute(['file' => $params['file1]]);
    $task->execute(['file' => $params['file2], 'notify' => true]);
}

Mixing instantiation with execution params, means that if we wanted to run a Task with different params, we have to construct a whole new object to do so.

This is much in the same way you run $db = Database::instance($name) to get an object, and then later you explicitly call $db->query() to perform an action.

You wouldn't ever think of having the query parameters bundled up in a hacky array passed to instance() method. You wouldn't create a whole new Database object for every query, you reuse the same object, and just pass it new parameters.

[cyrrill]

Also, if I wanted to run a Task with no parameters and just use the default options, I'd have to do this:

Minion_Task::factory(['task' => 'foobar'])->execute();

Which is clearly inferior to running:

Minion_Task::factory('foobar')->execute();

as you could with the clean factory interface.

[enov]

@enov what do you think?

@acoulton, I have never used this module. So I am not sure if my input would be of any help.