fix: do not read an undefined offset when a value argument is last
The argument parser looked up the next command-line token via $argv[$key + 1] guarded only by count($argv) === 0. The remaining arguments array is not reindexed after earlier arguments are removed, so a value-taking argument placed last (after an unconsumed token) read an undefined offset and raised an Undefined array key warning under PHP 8, and the argument was set to empty instead of falling back to its default. Check that the next offset exists with isset().
This commit is contained in:
1 parent
e65a9b267b
commit
94942b3aff
3 files changed
+119
-1
No files matched your search
@@ -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`.
|
||||
Reference in new issue
Block a user