[Console] Fix parsing optionnal options with empty value in argv #19946

chalasr · 2016-09-15T21:04:15Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#19884
License	MIT

If a command takes an option accepting an optional value, passing an empty value to this option will make it parsed as null, e.g:

bin/console dummy --foo "" gives ['foo' => null].
bin/console dummy --foo="" gives ['foo' => null].

Problems appear when adding an argument with a required value (let's call it bar):

bin/console dummy --foo "" "bar-val" gives ['foo' => null, 'bar' => 'bar-val'] which is OK.

But:

bin/console dummy --foo="" "bar-val"

[RuntimeException]
Not enough arguments (missing: "bar").

The empty value is never considered, as $argv just return "--foo=" for the option, the current implementation doesn't handle the empty value when using an equal as separator, so the bar argument value is considered as the foo one, giving a missing required argument at runtime.

This fixes it by explicitly considering the empty value if there is nothing immediately after the equal sign, so args/options correctly take their respective values.

chalasr · 2016-09-15T21:06:19Z

src/Symfony/Component/Console/Input/ArgvInput.php

@@ -232,7 +235,7 @@ private function addLongOption($name, $value)
            if (isset($next[0]) && '-' !== $next[0]) {
                $value = $next;
            } elseif (empty($next)) {
-                $value = '';
+                $value = null;


This change is needed, making the behavior always consistent with the expected one.

ogizanagi · 2016-09-16T16:33:44Z

src/Symfony/Component/Console/Input/ArgvInput.php

@@ -145,7 +145,10 @@ private function parseLongOption($token)
        $name = substr($token, 2);

        if (false !== $pos = strpos($name, '=')) {
-            $this->addLongOption(substr($name, 0, $pos), substr($name, $pos + 1));
+            if (!$value = substr($name, $pos + 1)) {


Still sure something like --foo=0 won't cause any issue with this check ? :/ (Maybe add this test case below ?)

Nice catch, thank you @ogizanagi

chalasr · 2016-09-16T17:24:17Z

Added a test case, base switched on 2.7, it's ok now.

ro0NL · 2016-09-17T12:27:08Z

src/Symfony/Component/Console/Tests/Input/ArgvInputTest.php

+                array('cli.php', '--foo='),
+                array(new InputOption('foo', 'f', InputOption::VALUE_OPTIONAL)),
+                array('foo' => null),
+                '->parse() parses long options with optionnal value which is empty (with a = separator) as null',


*optional

And below also.

Thank you @ro0NL

fabpot · 2016-09-17T16:41:39Z

Thank you @chalasr.

…n argv (chalasr) This PR was merged into the 2.7 branch. Discussion ---------- [Console] Fix parsing optionnal options with empty value in argv | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19884 | License | MIT If a command takes an option accepting an optional value, passing an empty value to this option will make it parsed as `null`, e.g: `bin/console dummy --foo ""` gives `['foo' => null]`. `bin/console dummy --foo=""` gives `['foo' => null]`. Problems appear when adding an argument with a required value (let's call it `bar`): `bin/console dummy --foo "" "bar-val"` gives `['foo' => null, 'bar' => 'bar-val']` which is OK. But: `bin/console dummy --foo="" "bar-val"` > [RuntimeException] Not enough arguments (missing: "bar"). The empty value is never considered, as `$argv` just return `"--foo="` for the option, the current implementation doesn't handle the empty value when using an equal as separator, so the `bar` argument value is considered as the `foo` one, giving a missing required argument at runtime. This fixes it by explicitly considering the empty value if there is nothing immediately after the equal sign, so args/options correctly take their respective values. Commits ------- 8952155 [Console] Fix empty optionnal options with = separator in argv

xabbuh · 2016-09-21T14:26:29Z

src/Symfony/Component/Console/Input/ArgvInput.php

@@ -145,7 +145,10 @@ private function parseLongOption($token)
        $name = substr($token, 2);

        if (false !== $pos = strpos($name, '=')) {
-            $this->addLongOption(substr($name, 0, $pos), substr($name, $pos + 1));
+            if (0 === strlen($value = substr($name, $pos + 1))) {


What about using '' === $value = substr($name, $pos + 1) instead?

Looks fine. See #20008
Btw, could you take a look on this check? I think we can do the same

Yes, looks like a candidate that could be changed.

…n translation:update (chalasr) This PR was merged into the 2.7 branch. Discussion ---------- [FrameworkBundle] Convert null prefix to an empty string in translation:update | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20044 | License | MIT | Doc PR | n/a This command needs the ability to use an empty string as prefix, which is not possible using `bin/console translation:update --prefix=""` because `$argv` doesn't parse empty strings thus the value is converted to `null` by `ArgvInput` (only since #19946, before the option was not considered to be set, giving the default `'__'` thus this should be fine from a BC pov). Here I propose to explicitly convert the `prefix` value to an empty string if set to `null`, as it is a very specific need and we can't guess that from `ArgvInput`. An other way to fix it could be to add a `--no-prefix` option to the command but I don't think it is worth it, and it couldn't be treated as a bug fix thus not fixed before `3.2`. Commits ------- f02b687 [FrameworkBundle] Convert null prefix to an empty string in translation:update command

carsonbot added Status: Needs Review Console Bug labels Sep 15, 2016

chalasr commented Sep 15, 2016

View reviewed changes

chalasr mentioned this pull request Sep 15, 2016

Console commands: named optional parameters not accepting empty values #19884

Closed

ogizanagi reviewed Sep 16, 2016

View reviewed changes

chalasr changed the base branch from master to 2.7 September 16, 2016 16:41

chalasr force-pushed the console/fix_option_emptyval_withequal branch from b569ebb to 1177289 Compare September 16, 2016 17:18

ro0NL reviewed Sep 17, 2016

View reviewed changes

chalasr force-pushed the console/fix_option_emptyval_withequal branch from 1177289 to 6b05e7f Compare September 17, 2016 12:38

[Console] Fix empty optionnal options with = separator in argv

8952155

chalasr force-pushed the console/fix_option_emptyval_withequal branch from 6b05e7f to 8952155 Compare September 17, 2016 12:42

fabpot merged commit 8952155 into symfony:2.7 Sep 17, 2016

chalasr deleted the console/fix_option_emptyval_withequal branch September 17, 2016 16:55

xabbuh reviewed Sep 21, 2016

View reviewed changes

chalasr mentioned this pull request Sep 21, 2016

[Console] Clarify behavior in ArgvInput #20008

Closed

linaori mentioned this pull request Sep 24, 2016

Allow to define an empty prefix for translation:update command #20044

Closed

This was referenced Oct 3, 2016

Release v2.7.19 #20140

Merged

Release v2.8.12 #20143

Merged

Release v3.1.5 #20144

Merged

chalasr mentioned this pull request Oct 8, 2016

[FrameworkBundle] Convert null prefix to an empty string in translation:update #20184

Merged

xabbuh mentioned this pull request Oct 10, 2016

Console BC break 2.8.12? #20194

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Fix parsing optionnal options with empty value in argv #19946

[Console] Fix parsing optionnal options with empty value in argv #19946

chalasr commented Sep 15, 2016 •

edited

Loading

chalasr Sep 15, 2016 •

edited

Loading

ogizanagi Sep 16, 2016 •

edited

Loading

chalasr Sep 16, 2016

chalasr commented Sep 16, 2016 •

edited

Loading

ro0NL Sep 17, 2016

chalasr Sep 17, 2016

fabpot commented Sep 17, 2016

xabbuh Sep 21, 2016

chalasr Sep 21, 2016

xabbuh Sep 21, 2016

[Console] Fix parsing optionnal options with empty value in argv #19946

[Console] Fix parsing optionnal options with empty value in argv #19946

Conversation

chalasr commented Sep 15, 2016 • edited Loading

chalasr Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

ogizanagi Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

chalasr Sep 16, 2016

Choose a reason for hiding this comment

chalasr commented Sep 16, 2016 • edited Loading

ro0NL Sep 17, 2016

Choose a reason for hiding this comment

chalasr Sep 17, 2016

Choose a reason for hiding this comment

fabpot commented Sep 17, 2016

xabbuh Sep 21, 2016

Choose a reason for hiding this comment

chalasr Sep 21, 2016

Choose a reason for hiding this comment

xabbuh Sep 21, 2016

Choose a reason for hiding this comment

chalasr commented Sep 15, 2016 •

edited

Loading

chalasr Sep 15, 2016 •

edited

Loading

ogizanagi Sep 16, 2016 •

edited

Loading

chalasr commented Sep 16, 2016 •

edited

Loading