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.

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

Negatable options can contradict each other #304

Open
manarth opened this issue Dec 20, 2023 · 4 comments
Open

Negatable options can contradict each other #304

manarth opened this issue Dec 20, 2023 · 4 comments

Comments

@manarth
Copy link

manarth commented Dec 20, 2023

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

  • Create a command class with an option "foo" whose default value is set to TRUE.
  • Verify that the --help output demonstrates a --foo and a --no-foo option.
  • Run the command with a --no-foo parameter.

Expected behavior

Tell us what should happen

  • The option value for --no-foo (acquired using $input->getOption()) should be set to TRUE.
  • The option value for --foo (acquired using $input->getOption()) should be set to FALSE.

Actual behavior

Tell us what happens instead

  • ✓ The option value for --no-foo (acquired using $input->getOption()) is set to TRUE.
  • ❌ The option value for --foo (acquired using $input->getOption()) is also set to TRUE.

System Configuration

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

  • PHP 8.2.12
  • Alpine Linux (container)
@manarth
Copy link
Author

manarth commented Dec 20, 2023

I've raised a potential change as pull-request #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
Copy link
Member

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
Copy link
Author

manarth commented Dec 21, 2023

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants