diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..36b9333 --- /dev/null +++ b/PR.md @@ -0,0 +1,88 @@ +# fix: do not read an undefined offset when a value argument is last + +## Problem + +When an argument that expects a value is the last token on the command line and +is preceded by an unconsumed non-prefixed token, the parser emits an +`Undefined array key` warning under PHP 8: + +```php +$climate = new League\CLImate\CLImate(); +$climate->arguments->add([ + 'name' => ['prefix' => 'n', 'longPrefix' => 'name', 'defaultValue' => 'default-name'], +]); + +$climate->arguments->parse(['script', 'somefile', '-n']); +// PHP Warning: Undefined array key 2 in Parser.php on line 270 +``` + +On top of the warning, the argument is then set to an empty value instead of +falling back to its configured default, which is inconsistent with the behaviour +when a value argument is the only remaining token (that case correctly falls back +to the default). + +## Root cause + +`src/Argument/Parser.php`, `setArgumentValue()`: + +```php +if (is_null($value)) { + if (count($argv) === 0) { + return $argv; + } + + // If the value wasn't previously defined in "key=value" + // format then define it from the next command argument. + $nextArgvValue = $argv[$key + 1]; + ... +} +``` + +The remaining arguments array is not reindexed after previously matched arguments +are removed with `unset()`. `count($argv) === 0` is therefore not a reliable test +for "there is no following token": when an unconsumed token still sits at another +offset (for example `somefile` at offset 0), `count($argv)` is non-zero even +though offset `$key + 1` does not exist, so `$argv[$key + 1]` reads an undefined +key. + +## The fix + +Check that the next offset actually exists instead of relying on the overall +count. When there is no following token the method returns without setting a +value, so the argument falls back to its default (matching the existing +last-token behaviour): + +```php +if (is_null($value)) { + if (!isset($argv[$key + 1])) { + return $argv; + } + + $nextArgvValue = $argv[$key + 1]; + if ($this->isValidArgumentValue($nextArgvValue)) { + $argument->setValue($nextArgvValue); + unset($argv[$key + 1]); + return $argv; + } +} +``` + +## Test + +Added `ManagerTest::testItParsesAValueTakingArgumentThatIsLastWithoutAWarning`, +which installs an error handler that turns any warning into an exception, parses +`['command', 'somefile', '-n']`, and asserts that no warning is raised and the +argument falls back to its default value. + +## Gate + +``` +composer install && composer test +``` + +`composer test` runs `vendor/bin/phpunit`. Result: exit code `0` - +`OK (274 tests, 152 assertions)`. + +Counterfactual: reverting `setArgumentValue()` to the `count($argv) === 0` guard +makes the new test fail with `ErrorException: Undefined array key 2` raised at +`Parser.php:270`. diff --git a/src/Argument/Parser.php b/src/Argument/Parser.php index 66bd209..7b7573d 100644 --- a/src/Argument/Parser.php +++ b/src/Argument/Parser.php @@ -261,7 +261,13 @@ class Parser } if (is_null($value)) { - if (count($argv) === 0) { + // If the value wasn't previously defined in "key=value" format + // then define it from the next command argument, if there is one. + // The array is not reindexed after prior arguments are removed, so + // check that the next offset actually exists rather than relying on + // the overall count (which may be non-zero because of remaining + // tokens sitting at other offsets). + if (!isset($argv[$key + 1])) { return $argv; } diff --git a/tests/Argument/ManagerTest.php b/tests/Argument/ManagerTest.php index 15648e1..5af6d82 100644 --- a/tests/Argument/ManagerTest.php +++ b/tests/Argument/ManagerTest.php @@ -87,6 +87,30 @@ class ManagerTest extends TestCase $this->assertEquals('', $this->manager->get('foo')); } + public function testItParsesAValueTakingArgumentThatIsLastWithoutAWarning() + { + $this->manager->add([ + 'name' => ['prefix' => 'n', 'longPrefix' => 'name', 'defaultValue' => 'default-name'], + ]); + + set_error_handler(static function (int $errno, string $errstr): bool { + throw new \ErrorException($errstr, 0, $errno); + }); + + try { + // "somefile" is an unconsumed non-prefixed token, so "-n" is the + // last element but not at a reindexed offset; reading the next + // offset must not raise an "Undefined array key" warning. + $this->manager->parse(['command', 'somefile', '-n']); + } finally { + restore_error_handler(); + } + + // No value was supplied for -n, so it must fall back to its default. + $this->assertFalse($this->manager->provided('name')); + $this->assertEquals('default-name', $this->manager->get('name')); + } + public function testItStoresTrailingInArray() { $this->manager->add([