consolidation / annotated-command

Create Symfony Console commands from annotated command class methods.
Other
221 stars 37 forks source link

Negatable options can contradict each other #304

Open manarth opened 11 months ago

manarth commented 11 months ago

Context

Symfony Console and Annotated Command support negateable options. An example can be found in the Drush cache:rebuild command which offers the --cache-clear and --no-cache-clear options.

Negateable options are expected to be in balance, if --cache-clear is set (and therefore TRUE), then --no-cache-clear should be FALSE.

The options documentation describes the behaviour where a default of true is provided

If the user adds --no-foo on the commandline, then the value of foo will be false.

https://github.com/consolidation/annotated-command#option-default-values

Steps to reproduce

Expected behavior

Tell us what should happen

Actual behavior

Tell us what happens instead

System Configuration

Which O.S. and PHP version are you using?

manarth commented 11 months ago

I've raised a potential change as pull-request https://github.com/consolidation/annotated-command/pull/305

It switches to the approach using by Symfony Console. However, an input option with a type of InputOption::VALUE_NONE cannot have a default value. so this would break backward compatibility.

One challenge is that the CommandInfo::addOption() method uses the $defaultValue parameter to infer both the $defaultValue and the $mode properties of a Symfony InputOption.

Another approach might be to add a separate addOption() method which allows $mode to be explicitly specified. We'd need to consider how the Symfony Negatable option may operate in parallel to the implicit "no" options added in Annotated Command.

greg-1-anderson commented 11 months ago

One very important aspect here is that we need our options to have a default value. Without a default value, then you cannot set option values with config. I thought that the Symfony Console negatable option allowed for this, but I have not investigated or tried your PR yet.

Regarding your report, if your option is defined as foo, then calling getOption on no-foo should be undefined. The option value for --foo in your scenario should be FALSE, so if you're getting TRUE then that would be a bug. Are there no tests for negatable options? It would be helpful if there were tests, especially tests that mirror what the Config project does when it sets option values, to show that negatable options can be set via Config (setDefault).

manarth commented 11 months ago

Here are the relevant Symfony Console InputOption statements for InputOption::VALUE_NONE and InputOption::VALUE_NEGATABLE:

https://github.com/symfony/console/blob/7.0/Input/InputOption.php#L113C1-L115C10

        if ($this->isNegatable() && $this->acceptValue()) {
            throw new InvalidArgumentException('Impossible to have an option mode VALUE_NEGATABLE if the option also accepts a value.');
        }

https://github.com/symfony/console/blob/7.0/Input/InputOption.php#L181

        if (self::VALUE_NONE === (self::VALUE_NONE & $this->mode) && null !== $default) {
            throw new LogicException('Cannot set a default value when using InputOption::VALUE_NONE mode.');
        }

Negatable options are always expected to be used without a value, either --foo or --no-foo. They don't support --foo=false, --foo=0 etc (or --no-foo=true and similar).

With a negatable option, if neither --foo nor --no-foo are set, getOption() will return NULL for both values. If either one is set, getOption will return TRUE for the one value and FALSE for the other.

This behaviour can be useful when the developer wishes to detect if an option has been explicitly set, but does mean the mechanism of providing a default value isn't available , and the developer would have to explicitly check for NULL and then apply a value programmatically.

greg-1-anderson commented 11 months ago

Very unfortunate. I explained the need for binary options to be able to accept a default value when I submitted the original PR for that feature. Sounds like the explanation was lost at some point in the ensuing years.