nategood / commando

An Elegant CLI Library for PHP
MIT License
798 stars 80 forks source link

"Needs" behaviour #50

Open phil043 opened 9 years ago

phil043 commented 9 years ago

Needs behaves a bit strangely and should interact with "requires" but doesn't. It's actually acting as a defacto "requires". Even if a "needed" command isn't "required" Commando will still throw an exception if it is specified as needed by an unused argument.

E.g

$cmd
->option("a");

$cmd->option("b")
    ->needs("c")

$cmd->option("c");

The following command should run:

filename.php --a --b --c

Only this should fail

filename.php --a --b

But this also (incorrectly) fails:

filename.php --a

In the last one the "needs" clause says that "b needs c" even though c isn't a required argument and b wasn't even specified.

The fix in my local branch is to replace the current "needs" block in Command.php with the following:

    // See if our options have what they require
            foreach ($keyvals as $key => $value) {
                $option = $this->getOption($key);
                if(!is_null($option->getValue()) || $option->isRequired()){
                    $needs = $option->hasNeeds($this->options);
                    if ($needs !== true) {
                        throw new \InvalidArgumentException(
                            'Option "'.$option->getName().'" does not have required option(s): '.implode(', ', $needs)
                        );
                    }
                }
            }

But I'd like to know if the current behaviour of "needs" also acting as a defacto "requires" is actually the desired behaviour before I upload it and submit a pull request.

mancioshell commented 7 years ago

+1 for this behaviour

klobyone commented 7 years ago

Yup, just ran into this as well.