Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Explicitly passed options without value (or empty) should remain empty#21228
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Console] Explicitly passed options without value (or empty) should remain empty#21228
Uh oh!
There was an error while loading.Please reload this page.
Conversation
672786f to5a6dddcComparejaviereguiluz commentedJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I don't know if it makes sense, but I expected an empty string in the third example (instead of In fact, that was my original problem: there is a command that defines a default value for the "prefix" option. I want to use an empty string as the prefix, but there's no way to tell the command that I want an empty string. |
ro0NL commentedJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
👍 for empty string when given empty.. |
chalasr commentedJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yeah, problem is that the conversion of empty strings to null is a feature (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Input/ArgvInput.php#L224). |
ro0NL commentedJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Imo. the distinction between |
javiereguiluz commentedJan 10, 2017
@ro0NL I think we are talking about different behaviors. You say: And this is what I say: |
ro0NL commentedJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yep.. you're right. Maybe even better 👍 However i considered it (for my own usecase 👼) as follow
In the last scenario i'd like to throw if the command is running non-interactively ( But you're right, this is a different look at it. |
ro0NL commentedJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
We actually talking about the same :) 👍 edit: depends though.. i vote for |
chalasr commentedJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
It's what this actually fixes: // beforecmd// $input->getOption('foo') gives the default valuecmd --foo// $input->getOption('foo') gives the default valuecmd --foo=""// $input->getOption('foo') gives the default value// aftercmd// $input->getOption('foo') gives the default valuecmd --foo// $input->getOption('foo') gives NULLcmd --foo=""// $input->getOption('foo') gives NULL (remaining problem, should give an empty string) |
ro0NL commentedJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
My bad :), your lastafter example sounds really good (with the correct empty string handling). Maybe for the input definition we could/should make things opt-in? Ie. |
ogizanagi commentedJan 10, 2017
IMHO it must be |
ro0NL commentedJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I tend to agree null makes more sense as well.. as it's rather explicit. However, i could understand one saying "no value means the default value", as we technically did not pass a value. Other then Anyway.. concerning BC.. what about making the behavior opt-in? |
chalasr commentedJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
It would add too much complexity to something already quite complex imho, from a code pov as well as from a usage one: reading About empty strings, I think we can't handle them while keeping a consistent behavior. This fixes the fact that default value is used when giving an empty value, another issue could be opened for the fact that null is returned for an empty string, but I'm afraid we can't fix without totally changing the way we get and parse the input (and I don't see how actually). |
ro0NL commentedJan 10, 2017
👍 Imo. |
javiereguiluz commentedJan 10, 2017
@ro0NL I don't think it's the same: An empty string is not the same as null. To me, |
ro0NL commentedJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
But quotes are optional.. |
chalasr commentedJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Sadly, regarding php it is not: // php cli.php --foo='bar'dump(getopt('', ['foo::']));// ["foo" => "bar"]dump($argv);// [0 => "cli.php", 1 => "--foo=bar"]// php cli.php --foo=dump(getopt('', ['foo::']));// []dump($argv);// [0 => "cli.php", 1 => "--foo="]// php cli.php --foo=''dump(getopt('', ['foo::']));// []dump($argv);// [0 => "cli.php", 1 => "--foo="] But: // php cli.php --foo="''"dump(getopt('', ['foo::']));// []dump($argv);// [0 => "cli.php", 1 => "--foo=''"] edit: For the record, I tried some of the most used CLI packages (hoa/console,auraphp/Aura.Cli,nategood/commando andc9s/CLIFramework) and none of them behave "perfectly" regarding our need. The fact we use |
javiereguiluz commentedJan 11, 2017
@chalasr OK. It's clear now that this problem has no solution because of PHP. We should instead add a note in the docs explaining that you cannot pass an empty string as the value of an option. Thanks for investigating this! |
chalasr commentedJan 14, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@javiereguiluz As explained in#21215 (comment), there is actually 2 different issues:
The former can be solved by returning The latter is either full white or full black: we have to use an empty value of a predefined primitive type (currently |
ro0NL commentedJan 15, 2017
The implicit empty string to null conversion as mentioned before, should be fixed at least imo.; looking athttps://github.com/symfony/symfony/blob/v3.2.2/src/Symfony/Component/Console/Input/ArgvInput.php#L145 we already detect the difference between a value an no value depending on Including this PR the only drawback seems we cant differ between |
chalasr commentedJan 15, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I looked at improving the situation for empty strings, here is what I end up with: Code $application =newApplication();$application ->register('echo') ->addOption('prefix',null, InputOption::VALUE_OPTIONAL,null,'my-default') ->addArgument('value', InputArgument::REQUIRED) ->setCode(function ($input,$output) {var_dump($input->getOption('prefix')); });$application->run(); We get the default value only when don't passing the option explicitly. |
5fa7221 to06b5e7fComparero0NL commentedJan 15, 2017
Looks really good :) can you confirm I think the latter is an edge case where you get |
chalasr commentedJan 20, 2017
Any thought here? |
5a6ca14 to6502cb6Comparechalasr commentedFeb 28, 2017
changelog updated, ping deciders |
6502cb6 to4c3c06eCompare| CHANGELOG | ||
| ========= | ||
| 2.7.25 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should be removed, bug fixes are documented in the main CHANGELOG, which is automatically generated when I make a new release.
fabpot commentedFeb 28, 2017
This looks like a BC break to me (and the fact that you added a note in the CHANGELOG confirms that feeling). So, I think that's not something that can be merged in 2.7, master would be ok if clearly documented. |
4c3c06e to776653aCompared37516e to259e8beComparechalasr commentedFeb 28, 2017
@fabpot rebased on master and documented. |
af4719e tof3d678fComparef3d678f to8086742Comparefabpot commentedMar 1, 2017
Thank you@chalasr. |
…empty) should remain empty (chalasr)This PR was merged into the 3.3-dev branch.Discussion----------[Console] Explicitly passed options without value (or empty) should remain empty| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#21215#11572#12773| License | MIT| Doc PR | n/a (maybe look at updating the existing one)This conserves empty values for options instead of returning their default values.Code:```php// cli.php$application = new Application();$application ->register('echo') ->addOption('prefix', null, InputOption::VALUE_OPTIONAL, null, 'my-default') ->addArgument('value', InputArgument::REQUIRED) ->setCode(function ($input, $output) { var_dump($input->getOption('prefix')); });$application->run();```Before:After:Commits-------8086742 [Console] Explicitly passed options without value (or empty) should remain empty
ro0NL commentedMar 1, 2017
Thanks@chalasr. Made my day 😊 |
… (chalasr)This PR was merged into the 3.4 branch.Discussion----------[FrameworkBundle] Deprecate useless --no-prefix option| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aIt was a workaround, not needed since#21228. Let's deprecate it and remove it in 4.0.Commits-------f7afa77 [FrameworkBundle] Deprecate useless --no-prefix option
… (chalasr)This PR was merged into the 3.4 branch.Discussion----------[FrameworkBundle] Deprecate useless --no-prefix option| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aIt was a workaround, not needed sincesymfony/symfony#21228. Let's deprecate it and remove it in 4.0.Commits-------f7afa777d8 [FrameworkBundle] Deprecate useless --no-prefix option


Uh oh!
There was an error while loading.Please reload this page.
This conserves empty values for options instead of returning their default values.
Code:
Before:

After:
