From 80867422acd83d2602187155e7eb978d6c3d0d44 Mon Sep 17 00:00:00 2001 From: Robin Chalas Date: Tue, 10 Jan 2017 13:19:41 +0100 Subject: [PATCH] [Console] Explicitly passed options without value (or empty) should remain empty --- UPGRADE-3.3.md | 56 +++++++++++++++++++ src/Symfony/Component/Console/CHANGELOG.md | 2 + .../Component/Console/Input/ArgvInput.php | 22 ++++---- .../Component/Console/Input/ArrayInput.php | 4 +- src/Symfony/Component/Console/Input/Input.php | 2 +- .../Console/Tests/Input/ArgvInputTest.php | 30 ++++++++-- .../Console/Tests/Input/ArrayInputTest.php | 8 ++- .../Console/Tests/Input/InputTest.php | 8 +++ 8 files changed, 111 insertions(+), 21 deletions(-) diff --git a/UPGRADE-3.3.md b/UPGRADE-3.3.md index 25769c9b3d..5097daf334 100644 --- a/UPGRADE-3.3.md +++ b/UPGRADE-3.3.md @@ -6,6 +6,62 @@ ClassLoader * The component is deprecated and will be removed in 4.0. Use Composer instead. +Console +------- + +* `Input::getOption()` no longer returns the default value for options + with value optional explicitly passed empty. + + For: + + ```php + protected function configure() + { + $this + // ... + ->setName('command') + ->addOption('foo', null, InputOption::VALUE_OPTIONAL, '', 'default') + ; + } + + protected function execute(InputInterface $input, OutputInterface $output) + { + var_dump($input->getOption('foo')); + } + ``` + + Before: + + ``` + $ php console.php command + "default" + + $ php console.php command --foo + "default" + + $ php console.php command --foo "" + "default" + + $ php console.php command --foo= + "default" + ``` + + After: + + ``` + $ php console.php command + "default" + + $ php console.php command --foo + NULL + + $ php console.php command --foo "" + "" + + $ php console.php command --foo= + "" + ``` + Debug ----- diff --git a/src/Symfony/Component/Console/CHANGELOG.md b/src/Symfony/Component/Console/CHANGELOG.md index c24c24c5d1..940aaba1f8 100644 --- a/src/Symfony/Component/Console/CHANGELOG.md +++ b/src/Symfony/Component/Console/CHANGELOG.md @@ -6,6 +6,8 @@ CHANGELOG * added `ExceptionListener` * added `AddConsoleCommandPass` (originally in FrameworkBundle) +* [BC BREAK] `Input::getOption()` no longer returns the default value for options + with value optional explicitly passed empty 3.2.0 ------ diff --git a/src/Symfony/Component/Console/Input/ArgvInput.php b/src/Symfony/Component/Console/Input/ArgvInput.php index f626c33e59..85cd779849 100644 --- a/src/Symfony/Component/Console/Input/ArgvInput.php +++ b/src/Symfony/Component/Console/Input/ArgvInput.php @@ -148,7 +148,12 @@ class ArgvInput extends Input if (false !== $pos = strpos($name, '=')) { if (0 === strlen($value = substr($name, $pos + 1))) { - array_unshift($this->parsed, null); + // if no value after "=" then substr() returns "" since php7 only, false before + // see http://php.net/manual/fr/migration70.incompatible.php#119151 + if (PHP_VERSION_ID < 70000 && false === $value) { + $value = ''; + } + array_unshift($this->parsed, $value); } $this->addLongOption(substr($name, 0, $pos), $value); } else { @@ -221,23 +226,16 @@ class ArgvInput extends Input $option = $this->definition->getOption($name); - // Convert empty values to null - if (!isset($value[0])) { - $value = null; - } - if (null !== $value && !$option->acceptValue()) { throw new RuntimeException(sprintf('The "--%s" option does not accept a value.', $name)); } - if (null === $value && $option->acceptValue() && count($this->parsed)) { + if (in_array($value, array('', null), true) && $option->acceptValue() && count($this->parsed)) { // if option accepts an optional or mandatory argument // let's see if there is one provided $next = array_shift($this->parsed); - if (isset($next[0]) && '-' !== $next[0]) { + if ((isset($next[0]) && '-' !== $next[0]) || in_array($next, array('', null), true)) { $value = $next; - } elseif (empty($next)) { - $value = null; } else { array_unshift($this->parsed, $next); } @@ -248,8 +246,8 @@ class ArgvInput extends Input throw new RuntimeException(sprintf('The "--%s" option requires a value.', $name)); } - if (!$option->isArray()) { - $value = $option->isValueOptional() ? $option->getDefault() : true; + if (!$option->isArray() && !$option->isValueOptional()) { + $value = true; } } diff --git a/src/Symfony/Component/Console/Input/ArrayInput.php b/src/Symfony/Component/Console/Input/ArrayInput.php index a44b6b2815..434ec0240d 100644 --- a/src/Symfony/Component/Console/Input/ArrayInput.php +++ b/src/Symfony/Component/Console/Input/ArrayInput.php @@ -179,7 +179,9 @@ class ArrayInput extends Input throw new InvalidOptionException(sprintf('The "--%s" option requires a value.', $name)); } - $value = $option->isValueOptional() ? $option->getDefault() : true; + if (!$option->isValueOptional()) { + $value = true; + } } $this->options[$name] = $value; diff --git a/src/Symfony/Component/Console/Input/Input.php b/src/Symfony/Component/Console/Input/Input.php index 474a020312..244e7d4e58 100644 --- a/src/Symfony/Component/Console/Input/Input.php +++ b/src/Symfony/Component/Console/Input/Input.php @@ -158,7 +158,7 @@ abstract class Input implements InputInterface, StreamableInputInterface throw new InvalidArgumentException(sprintf('The "%s" option does not exist.', $name)); } - return isset($this->options[$name]) ? $this->options[$name] : $this->definition->getOption($name)->getDefault(); + return array_key_exists($name, $this->options) ? $this->options[$name] : $this->definition->getOption($name)->getDefault(); } /** diff --git a/src/Symfony/Component/Console/Tests/Input/ArgvInputTest.php b/src/Symfony/Component/Console/Tests/Input/ArgvInputTest.php index 9cdac36648..8287bce521 100644 --- a/src/Symfony/Component/Console/Tests/Input/ArgvInputTest.php +++ b/src/Symfony/Component/Console/Tests/Input/ArgvInputTest.php @@ -48,7 +48,7 @@ class ArgvInputTest extends TestCase $input = new ArgvInput($input); $input->bind(new InputDefinition($options)); - $this->assertEquals($expectedOptions, $input->getOptions(), $message); + $this->assertSame($expectedOptions, $input->getOptions(), $message); } public function provideOptions() @@ -75,14 +75,32 @@ class ArgvInputTest extends TestCase array( array('cli.php', '--foo='), array(new InputOption('foo', 'f', InputOption::VALUE_OPTIONAL)), - array('foo' => null), - '->parse() parses long options with optional value which is empty (with a = separator) as null', + array('foo' => ''), + '->parse() parses long options with optional value which is empty (with a = separator) as empty string', ), array( array('cli.php', '--foo=', 'bar'), array(new InputOption('foo', 'f', InputOption::VALUE_OPTIONAL), new InputArgument('name', InputArgument::REQUIRED)), + array('foo' => ''), + '->parse() parses long options with optional value without value specified or an empty string (with a = separator) followed by an argument as empty string', + ), + array( + array('cli.php', 'bar', '--foo'), + array(new InputOption('foo', 'f', InputOption::VALUE_OPTIONAL), new InputArgument('name', InputArgument::REQUIRED)), array('foo' => null), - '->parse() parses long options with optional value which is empty (with a = separator) followed by an argument', + '->parse() parses long options with optional value which is empty (with a = separator) preceded by an argument', + ), + array( + array('cli.php', '--foo', '', 'bar'), + array(new InputOption('foo', 'f', InputOption::VALUE_OPTIONAL), new InputArgument('name', InputArgument::REQUIRED)), + array('foo' => ''), + '->parse() parses long options with optional value which is empty as empty string even followed by an argument', + ), + array( + array('cli.php', '--foo'), + array(new InputOption('foo', 'f', InputOption::VALUE_OPTIONAL)), + array('foo' => null), + '->parse() parses long options with optional value specified with no separator and no value as null', ), array( array('cli.php', '-f'), @@ -252,14 +270,14 @@ class ArgvInputTest extends TestCase $input = new ArgvInput(array('cli.php', '--name=foo', '--name=bar', '--name=')); $input->bind(new InputDefinition(array(new InputOption('name', null, InputOption::VALUE_OPTIONAL | InputOption::VALUE_IS_ARRAY)))); - $this->assertSame(array('name' => array('foo', 'bar', null)), $input->getOptions(), '->parse() parses empty array options as null ("--option=value" syntax)'); + $this->assertSame(array('name' => array('foo', 'bar', '')), $input->getOptions(), '->parse() parses empty array options as null ("--option=value" syntax)'); $input = new ArgvInput(array('cli.php', '--name', 'foo', '--name', 'bar', '--name', '--anotherOption')); $input->bind(new InputDefinition(array( new InputOption('name', null, InputOption::VALUE_OPTIONAL | InputOption::VALUE_IS_ARRAY), new InputOption('anotherOption', null, InputOption::VALUE_NONE), ))); - $this->assertSame(array('name' => array('foo', 'bar', null), 'anotherOption' => true), $input->getOptions(), '->parse() parses empty array options as null ("--option value" syntax)'); + $this->assertSame(array('name' => array('foo', 'bar', null), 'anotherOption' => true), $input->getOptions(), '->parse() parses empty array options ("--option value" syntax)'); } public function testParseNegativeNumberAfterDoubleDash() diff --git a/src/Symfony/Component/Console/Tests/Input/ArrayInputTest.php b/src/Symfony/Component/Console/Tests/Input/ArrayInputTest.php index a3e938acd3..a1b5a2030b 100644 --- a/src/Symfony/Component/Console/Tests/Input/ArrayInputTest.php +++ b/src/Symfony/Component/Console/Tests/Input/ArrayInputTest.php @@ -90,9 +90,15 @@ class ArrayInputTest extends TestCase '->parse() parses long options with a default value', ), array( - array('--foo' => null), + array(), array(new InputOption('foo', 'f', InputOption::VALUE_OPTIONAL, '', 'default')), array('foo' => 'default'), + '->parse() uses the default value for long options with value optional which are not passed', + ), + array( + array('--foo' => null), + array(new InputOption('foo', 'f', InputOption::VALUE_OPTIONAL, '', 'default')), + array('foo' => null), '->parse() parses long options with a default value', ), array( diff --git a/src/Symfony/Component/Console/Tests/Input/InputTest.php b/src/Symfony/Component/Console/Tests/Input/InputTest.php index 7a2c64eb64..4410d2f591 100644 --- a/src/Symfony/Component/Console/Tests/Input/InputTest.php +++ b/src/Symfony/Component/Console/Tests/Input/InputTest.php @@ -37,6 +37,14 @@ class InputTest extends TestCase $input = new ArrayInput(array('--name' => 'foo'), new InputDefinition(array(new InputOption('name'), new InputOption('bar', '', InputOption::VALUE_OPTIONAL, '', 'default')))); $this->assertEquals('default', $input->getOption('bar'), '->getOption() returns the default value for optional options'); $this->assertEquals(array('name' => 'foo', 'bar' => 'default'), $input->getOptions(), '->getOptions() returns all option values, even optional ones'); + + $input = new ArrayInput(array('--name' => 'foo', '--bar' => ''), new InputDefinition(array(new InputOption('name'), new InputOption('bar', '', InputOption::VALUE_OPTIONAL, '', 'default')))); + $this->assertEquals('', $input->getOption('bar'), '->getOption() returns null for options explicitly passed without value (or an empty value)'); + $this->assertEquals(array('name' => 'foo', 'bar' => ''), $input->getOptions(), '->getOptions() returns all option values.'); + + $input = new ArrayInput(array('--name' => 'foo', '--bar' => null), new InputDefinition(array(new InputOption('name'), new InputOption('bar', '', InputOption::VALUE_OPTIONAL, '', 'default')))); + $this->assertNull($input->getOption('bar'), '->getOption() returns null for options explicitly passed without value (or an empty value)'); + $this->assertEquals(array('name' => 'foo', 'bar' => null), $input->getOptions(), '->getOptions() returns all option values'); } /**