From 005aa6b84a3cefd46eb629c486b5cda56f9b83a7 Mon Sep 17 00:00:00 2001 From: Marcel Voigt Date: Tue, 26 Jan 2016 22:34:16 +0100 Subject: [PATCH 1/3] Add failing test regarding missing input --- .../PHPSemVerChecker/Console/InputMergerTest.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/PHPSemVerChecker/Console/InputMergerTest.php b/tests/PHPSemVerChecker/Console/InputMergerTest.php index c375d1a..81c70bf 100644 --- a/tests/PHPSemVerChecker/Console/InputMergerTest.php +++ b/tests/PHPSemVerChecker/Console/InputMergerTest.php @@ -33,4 +33,20 @@ public function testMerge() $this->assertEquals('src-after config', $input->getArgument('source-after'), 'Missing input arguments must take on existing configuration'); $this->assertEquals(true, $config->get('full-path'), 'CLI option should use Configuration value and not CLI default'); } + + /** + * @expectedException \Symfony\Component\Console\Exception\RuntimeException + */ + public function testEmptyInputShouldThrowException() + { + // Default/empty configuration + $config = new Configuration([]); + // No input arguments + $input = new InspectableArgvInput([null]); + $command = new CompareCommand(); + $input->bind($command->getDefinition()); + $im = new InputMerger(); + $im->merge($input, $config); + $input->validate(); + } } From 1d67efa6c0e61a524f7ea1e50158fd73696c2dde Mon Sep 17 00:00:00 2001 From: Marcel Voigt Date: Tue, 26 Jan 2016 23:04:10 +0100 Subject: [PATCH 2/3] Fix input validation by using only known config values --- src/PHPSemVerChecker/Console/InputMerger.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/PHPSemVerChecker/Console/InputMerger.php b/src/PHPSemVerChecker/Console/InputMerger.php index b7fb249..9cf6c98 100644 --- a/src/PHPSemVerChecker/Console/InputMerger.php +++ b/src/PHPSemVerChecker/Console/InputMerger.php @@ -23,7 +23,10 @@ public function merge(InputInterface $input, Configuration $config) if ($input->hasArgumentSet($argument)) { $config->set($argument, $value); } else { - $input->setArgument($argument, $config->get($argument)); + $configValue = $config->get($argument); + if ($configValue !== null) { + $input->setArgument($argument, $configValue); + } } } @@ -31,7 +34,10 @@ public function merge(InputInterface $input, Configuration $config) if ($input->hasOptionSet($option)) { $config->set($option, $value); } else { - $input->setOption($option, $config->get($option)); + $configValue = $config->get($option); + if ($configValue !== null) { + $input->setOption($option, $configValue); + } } } } From 870a30bbaa6930e7b601b858347f499c3e7f8eb6 Mon Sep 17 00:00:00 2001 From: Marcel Voigt Date: Wed, 27 Jan 2016 00:35:52 +0100 Subject: [PATCH 3/3] Fix by simply checking InputDefinition for edge case --- .../Console/Command/BaseCommand.php | 2 +- src/PHPSemVerChecker/Console/InputMerger.php | 23 +++++++++++-------- .../Console/InputMergerTest.php | 4 ++-- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/PHPSemVerChecker/Console/Command/BaseCommand.php b/src/PHPSemVerChecker/Console/Command/BaseCommand.php index f098e9a..84fede8 100644 --- a/src/PHPSemVerChecker/Console/Command/BaseCommand.php +++ b/src/PHPSemVerChecker/Console/Command/BaseCommand.php @@ -26,7 +26,7 @@ protected function initialize(InputInterface $input, OutputInterface $output) $configPath = $input->getOption('config'); $this->config = $configPath ? Configuration::fromFile($configPath) : Configuration::defaults('php-semver-checker'); $inputMerger = new InputMerger(); - $inputMerger->merge($input, $this->config); + $inputMerger->merge($input, $this->getDefinition(), $this->config); // Set overrides LevelMapping::setOverrides($this->config->getLevelMapping()); diff --git a/src/PHPSemVerChecker/Console/InputMerger.php b/src/PHPSemVerChecker/Console/InputMerger.php index 9cf6c98..28d10e2 100644 --- a/src/PHPSemVerChecker/Console/InputMerger.php +++ b/src/PHPSemVerChecker/Console/InputMerger.php @@ -3,6 +3,7 @@ namespace PHPSemVerChecker\Console; use PHPSemVerChecker\Configuration\Configuration; +use Symfony\Component\Console\Input\InputDefinition; use Symfony\Component\Console\Input\InputInterface; /** @@ -14,29 +15,33 @@ class InputMerger { /** - * @param \Symfony\Component\Console\Input\InputInterface $input - * @param \PHPSemVerChecker\Configuration\Configuration $config + * @param \Symfony\Component\Console\Input\InputInterface $input Actual input + * @param \Symfony\Component\Console\Input\InputDefinition $inputDefinition Definition of input arguments/options + * @param \PHPSemVerChecker\Configuration\Configuration $config */ - public function merge(InputInterface $input, Configuration $config) + public function merge(InputInterface $input, InputDefinition $inputDefinition, Configuration $config) { foreach ($input->getArguments() as $argument => $value) { - if ($input->hasArgumentSet($argument)) { + if ($value !== null) { $config->set($argument, $value); } else { $configValue = $config->get($argument); + // Only set an argument from config if actually known if ($configValue !== null) { $input->setArgument($argument, $configValue); } } } - foreach ($input->getOptions() as $option => $value) { - if ($input->hasOptionSet($option)) { - $config->set($option, $value); + foreach ($input->getOptions() as $optionName => $value) { + $option = $inputDefinition->getOption($optionName); + // Make sure VALUE_NONE is only used when differing from default + if ((!$option->acceptValue() && $value !== $option->getDefault()) || ($option->acceptValue() && $value !== null)) { + $config->set($optionName, $value); } else { - $configValue = $config->get($option); + $configValue = $config->get($optionName); if ($configValue !== null) { - $input->setOption($option, $configValue); + $input->setOption($optionName, $configValue); } } } diff --git a/tests/PHPSemVerChecker/Console/InputMergerTest.php b/tests/PHPSemVerChecker/Console/InputMergerTest.php index 81c70bf..7e0230b 100644 --- a/tests/PHPSemVerChecker/Console/InputMergerTest.php +++ b/tests/PHPSemVerChecker/Console/InputMergerTest.php @@ -25,7 +25,7 @@ public function testMerge() $this->assertEquals('in-before cli', $input->getOption('include-before'), 'Test setup: Could not prepare input arguments'); $im = new InputMerger(); - $im->merge($input, $config); + $im->merge($input, $command->getDefinition(), $config); $this->assertEquals('in-before cli', $config->get('include-before'), 'Configuration must be overwritten by CLI option'); $this->assertEquals('src-before cli', $config->get('source-before'), 'Configuration must be overwritten by CLI argument'); $this->assertEquals('src-before cli', $input->getArgument('source-before'), 'Input arguments must not be overwritten by empty configuration'); @@ -46,7 +46,7 @@ public function testEmptyInputShouldThrowException() $command = new CompareCommand(); $input->bind($command->getDefinition()); $im = new InputMerger(); - $im->merge($input, $config); + $im->merge($input, $command->getDefinition(), $config); $input->validate(); } }