[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

                $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


        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(
+                array('cli.php', '--foo='),
+                array(new InputOption('foo', 'f', InputOption::VALUE_OPTIONAL)),
+                array('foo' => 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


        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

Uh oh!

Uh oh!

[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

Uh oh!

chalasr Sep 15, 2016 •

edited

Loading

Uh oh!

ogizanagi Sep 16, 2016 •

edited

Loading

Uh oh!

chalasr Sep 16, 2016

Uh oh!

chalasr commented Sep 16, 2016 •

edited

Loading

Uh oh!

ro0NL Sep 17, 2016

Uh oh!

chalasr Sep 17, 2016

Uh oh!

fabpot commented Sep 17, 2016

Uh oh!

xabbuh Sep 21, 2016

Uh oh!

chalasr Sep 21, 2016

Uh oh!

xabbuh Sep 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

[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 Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chalasr Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr Sep 16, 2016

Choose a reason for hiding this comment

Uh oh!

chalasr commented Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ro0NL Sep 17, 2016

Choose a reason for hiding this comment

Uh oh!

chalasr Sep 17, 2016

Choose a reason for hiding this comment

Uh oh!

fabpot commented Sep 17, 2016

Uh oh!

xabbuh Sep 21, 2016

Choose a reason for hiding this comment

Uh oh!

chalasr Sep 21, 2016

Choose a reason for hiding this comment

Uh oh!

xabbuh Sep 21, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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