8000 [Console] Input options with InputOption::VALUE_OPTIONAL and InputOption::VALUE_NONE not working as expected · Issue #8135 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Console] Input options with InputOption::VALUE_OPTIONAL and InputOption::VALUE_NONE not working as expected #8135

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.

Sign up for GitHub

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

Jump to bottom
Closed
tiagojsag opened this issue May 25, 2013 · 13 comments
Closed

[Console] Input options with InputOption::VALUE_OPTIONAL and InputOption::VALUE_NONE not working as expected #8135

tiagojsag opened this issue May 25, 2013 · 13 comments
Labels
Console

Comments

@tiagojsag
Copy link
Contributor

Hi,

In the Console component, there are a couple of bugs regarding input options.
If a option is specified with just InputOption::VALUE_OPTIONAL, and command is called with that option with no value (php app/console my:command --my-option), then $input->getOption('my-option') returns null. It's the same as if the option was not defined.

On the other hand, if you use the same code, with InputOption::VALUE_NONE, it behaves like InputOption::VALUE_OPTIONAL should: option with no value returns true, option with value returns the value and not an error, as it would be expected, I think.

This can be demoed with the obvious modifications to the code available on the sf2.2 docs page.

cheers
@fabpot
Copy link
Member
fabpot commented May 25, 2013

That's not a bug. When you use VALUE_OPTIONAL and if it is not set, Symfony returns the default value set in the definition, which happens to be null. But you can specify whatever you want for the default value.

@fabpot fabpot closed this as completed May 25, 2013
@tiagojsag
Copy link
Contributor Author

Oh, ok, my bad then, sorry :/
And what about the case when InputOption::VALUE_NONE actually accepts values without complaining?

@tiagojsag
Copy link
Contributor Author

@fabpot I've followed your suggestion and passed a 5th argument in the addOption() call. In that case, I still get what I believe to be a buggy behavior.

->addOption(
                    'notify',
                    null,
                    InputOption::VALUE_OPTIONAL,
                    'Send the user a report after the process is finished. If no user is specified, an email address must be provided.',
                    true
            )

With this setup, I tested these two cases:

php app/console some:command --notify
php app/console some:command

In both cases, $input->getOption('notify') returns the default value, 'true' in this case. If a value is passed on the command call, everything works as expected. I've tried with default values other than true, like strings, and the behavior is the same.

On the other hand, I still believe VALUE_NONE is bugged, as it accepts a value and passes it when $input->getOption() is called. It basically does what VALUE_OPTIONAL should, but without accepting a default value on the definition.

Can you please take a look a this a see if I'm doing some silly mistake, or if one/two bugs actually exist?

cheers

@tiagojsag
Copy link
Contributor Author

@fabpot can you please take a look at my last comment, and see if it's a bug or a mistake on my part? cheers

@tiagojsag tiagojsag mentioned this issue Jun 5, 2013
Closed
fabpot added a commit that referenced this issue Jun 13, 2013
@fabpot
merged branch tiagojsag/console_input_options_fix (PR #8199)
9a76c4f 
This PR was submitted for the master branch but it was merged into the 2.2 branch instead (closes #8199).

Discussion
----------

[Console] Throw exception if value is passed to VALUE_NONE input optin, long syntax

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #8135
| License       | MIT

Input options with InputOption::VALUE_NONE accept values in both short and long syntaxes:
- When using the long syntax, no exception is thrown;
- When using short, a "The %s option does not exist" exception is thrown.

This PR only addresses the long syntax case. The short syntax case would require considerable refactoring of the parse code, which I believe should be discussed.

I included a test that illustrates the above mentioned problem for the long syntax scenario.

Commits
-------

32ea77f Throw exception if value is passed to VALUE_NONE input, long syntax
@temp
Copy link
temp commented Nov 28, 2013

Sorry to get back to this, but I think this is still confusing.

How do I define a parameter (e.g. --value) with these three states:

app/console test 
// => getOption('value') = false
app/console test --value 
// => getOption('value') = true
app/console test --value=5 
// => getOption('value') = 5

I thought this would suffice:

new InputOption('value', null, InputOption::VALUE_OPTIONAL, 'test', true)

But now even without --value I get the default value "true".

How do I achieve a bevhaviour like the described one?

@mathewrapid
Copy link

This should be reopened per #8135 (comment)

@psynaptic
Copy link

Agreed. I think the current behavior is suboptimal as it doesn't allow the three states mentioned above.

Use case:

I have an option called --default, it performs as a way of setting and getting a default value:

  • When the --default option is not provided, I would like it to be set to FALSE.
  • When the --default option is provided but has no value, I'd like it to be set to TRUE (or NULL would suffice--not sure which is more semantically correct, the value is set but it is empty).
  • When the --default option is provided and it has a value, I'd like it to be set to the value.

This allows me to distinguish between whether or not I should descend into the default setting/getting handling code:

    $default = $input->getOption('default');
    // If the --default option was provided at all.
    if ($default !== FALSE) {
      // A NULL value means the --default option was provided without a value
      // so get the default value and display it.
      if ($default === NULL) {
        // Get the default value.
      }
      // Any other values means we should attempt to set the default to the
      // given value.
      else {
        // Set the default value.
      }
    }
    // Further code to handle the other options.

@psynaptic
Copy link

@fabpot: Would you consider re-opening this issue? I'm working around the problem using an additional optional argument but it is starting to get a bit unwieldy juggling the options and arguments without this feature.

@psynaptic
Copy link

I think it would be best if the default was only applied if the option flag is passed. When the option is not passed, it should be NULL.

  • The option flag was not passed: NULL.
  • The option flag was passed without a value: default value.
  • The option flag was passed with a value: specified value.

@OndraM
Copy link
Contributor
OndraM commented Nov 26, 2014

+1 for #8135 (comment), or this could be handled in another InputOption type to not introduce BC.

@OndraM
Copy link
Contributor
OndraM commented Nov 26, 2014

BTW as workaround you can use getParameterOption() in a similar way:

// in configure()
$this->addOption('foo', null, InputOption::VALUE_OPTIONAL, 'Foo description', 'default');

// in execute()
if ($input->getParameterOption('--foo') === false) { // when option was not used at all
    ...
} else { // option was used, either with (--foo=bar) or without value (--foo)
    $foo = $input->getOption('foo'); // get passed value or default if nothing passed
}

@SiliconMind
Copy link

@OndraM Unfortunately your solution won't work if there will be another option after --foo. For example:

app/console something:test --foo --another=option
var_dump($input->getParameterOption('--foo'));

// Prints:
// string(16) "--status=option"

So I'd say it's a bug as there seems to be no way to work around the issue described in #8135 (comment)

@OndraM
Copy link
Contributor
OndraM commented Nov 28, 2014

@SiliconMind IMO it will still work. Considering your example and my code above, note the getParameterOption() is not used to retrieve the option value, but only to check whether the option was present at all.

Once we know this (this is when getParameterOption() does not exactly equals to false), we get the value using getOption(). It is kind of a hack, a thus I'd definitely prefer a clean way hot to do this.

@OndraM OndraM mentioned this issue Nov 30, 2014
Closed
@remkop remkop mentioned this issue Feb 28, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Console
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@psynaptic @fabpot @temp @OndraM @SiliconMind @tia 2C89 gojsag @mathewrapid
0