address reviews

Author: paulbalandan

system/CLI/AbstractCommand.php

@@ -217,7 +217,7 @@ public function addArgument(Argument $argument): static
     {
         $name = $argument->name;
 
-        if (array_key_exists($name, $this->argumentsDefinition)) {
+        if ($this->hasArgument($name)) {
             throw new InvalidArgumentDefinitionException(lang('Commands.duplicateArgument', [$name]));
         }
 
@@ -253,15 +253,19 @@ public function addOption(Option $option): static
     {
         $name = $option->name;
 
-        if (array_key_exists($name, $this->optionsDefinition)) {
+        if ($this->hasOption($name)) {
             throw new InvalidOptionDefinitionException(lang('Commands.duplicateOption', [$name]));
         }
 
-        if ($option->shortcut !== null && array_key_exists($option->shortcut, $this->shortcuts)) {
+        if ($this->hasNegation($name)) {
+            throw new InvalidOptionDefinitionException(lang('Commands.optionClashesWithExistingNegation', [$name, $this->negations[$name]]));
+        }
+
+        if ($option->shortcut !== null && $this->hasShortcut($option->shortcut)) {
             throw new InvalidOptionDefinitionException(lang('Commands.duplicateShortcut', [$option->shortcut, $name, $this->shortcuts[$option->shortcut]]));
         }
 
-        if ($option->negation !== null && array_key_exists($option->negation, $this->optionsDefinition)) {
+        if ($option->negation !== null && $this->hasOption($option->negation)) {
             throw new InvalidOptionDefinitionException(lang('Commands.negatableOptionNegationExists', [$name]));
         }
 
@@ -692,7 +696,7 @@ private function bind(array $arguments, array $options): array
 
         // 4. If there are still options left that are not defined, we will mark them as extraneous.
         foreach ($options as $name => $value) {
-            if (array_key_exists($name, $this->shortcuts)) {
+            if ($this->hasShortcut($name)) {
                 // This scenario can happen when the command has an array option with a shortcut,
                 // and the shortcut is used alongside the long name, causing it to be not bound
                 // in the previous loop.
@@ -707,7 +711,7 @@ private function bind(array $arguments, array $options): array
                 continue;
             }
 
-            if (array_key_exists($name, $this->negations)) {
+            if ($this->hasNegation($name)) {
                 // This scenario can happen when the command has a negatable option,
                 // and both the option and its negation are used, causing the negation
                 // to be not bound in the previous loop.
@@ -816,8 +820,14 @@ private function validateOption(string $name, Option $definition, array|bool|str
             throw new OptionValueMismatchException(lang('Commands.nonArrayOptionWithArrayValue', [$name]));
         }
 
-        if ($definition->requiresValue && ! is_string($value) && ! is_array($value)) {
-            throw new OptionValueMismatchException(lang('Commands.optionRequiresValue', [$name]));
+        if ($definition->requiresValue) {
+            $elements = is_array($value) ? $value : [$value];
+
+            foreach ($elements as $element) {
+                if (! is_string($element)) {
+                    throw new OptionValueMismatchException(lang('Commands.optionRequiresValue', [$name]));
+                }
+            }
         }
 
         if (! $definition->negatable || is_bool($value)) {
@@ -859,7 +869,7 @@ private function validateNegatableOption(string $name, Option $definition, array
      */
     private function getOptionDefinitionFor(string $name): Option
     {
-        if (! array_key_exists($name, $this->optionsDefinition)) {
+        if (! $this->hasOption($name)) {
             throw new LogicException(sprintf('Option "%s" is not defined on this command.', $name));
         }
 

system/CLI/Commands.php

@@ -217,11 +217,13 @@ public function discoverCommands()
 
         foreach (array_keys(array_intersect_key($this->commands, $this->modernCommands)) as $name) {
             CLI::write(
-                lang('Commands.duplicateCommandName', [
-                    $name,
-                    $this->commands[$name]['class'],
-                    $this->modernCommands[$name]['class'],
-                ]),
+                CLI::wrap(
+                    lang('Commands.duplicateCommandName', [
+                        $name,
+                        $this->commands[$name]['class'],
+                        $this->modernCommands[$name]['class'],
+                    ]),
+                ),
                 'yellow',
             );
         }
@@ -248,7 +250,7 @@ public function verifyCommand(string $command, array $commands = [], bool $legac
 
         $message = lang('CLI.commandNotFound', [$command]);
 
-        $alternatives = $this->getCommandAlternatives($command, legacy: $legacy);
+        $alternatives = $this->getCommandAlternatives($command);
 
         if ($alternatives !== []) {
             $message = sprintf(
@@ -265,24 +267,22 @@ public function verifyCommand(string $command, array $commands = [], bool $legac
     }
 
     /**
-     * Finds alternative of `$name` among collection of commands.
+     * Finds alternative of `$name` across both legacy and modern commands.
      *
      * @param legacy_commands $collection (no longer used)
      *
      * @return list<string>
      */
-    protected function getCommandAlternatives(string $name, array $collection = [], bool $legacy = true): array
+    protected function getCommandAlternatives(string $name, array $collection = []): array
     {
         if ($collection !== []) {
             @trigger_error(sprintf('Since v4.8.0, the $collection parameter of %s() is no longer used.', __METHOD__), E_USER_DEPRECATED);
         }
 
-        $commandCollection = $legacy ? $this->commands : $this->modernCommands;
-
         /** @var array<string, int> */
         $alternatives = [];
 
-        foreach (array_keys($commandCollection) as $commandName) {
+        foreach (array_keys($this->commands + $this->modernCommands) as $commandName) {
             $lev = levenshtein($name, $commandName);
 
             if ($lev <= strlen($commandName) / 3 || str_contains($commandName, $name)) {

system/Commands/ListCommands.php

@@ -43,10 +43,11 @@ protected function execute(array $arguments, array $options): int
 
     private function describeCommandsSimple(): int
     {
-        $commands = [
-            ...array_keys($this->getCommandRunner()->getCommands()),
-            ...array_keys($this->getCommandRunner()->getModernCommands()),
-        ];
+        // Legacy takes precedence on key collision so the listing reflects the
+        // command that would actually be invoked.
+        $commands = array_keys(
+            $this->getCommandRunner()->getCommands() + $this->getCommandRunner()->getModernCommands(),
+        );
         sort($commands);
 
         foreach ($commands as $command) {
@@ -64,10 +65,11 @@ private function describeCommandsDetailed(): int
         $entries = [];
         $maxPad  = 0;
 
-        foreach ([
-            ...$this->getCommandRunner()->getCommands(),
-            ...$this->getCommandRunner()->getModernCommands(),
-        ] as $command => $details) {
+        // Legacy takes precedence on key collision so the listing reflects the
+        // command that would actually be invoked.
+        $all = $this->getCommandRunner()->getCommands() + $this->getCommandRunner()->getModernCommands();
+
+        foreach ($all as $command => $details) {
             $maxPad = max($maxPad, strlen($command) + 4);
 
             $entries[] = [$details['group'], $command, $details['description']];
@@ -105,7 +107,7 @@ private function describeCommandsDetailed(): int
                 CLI::write(sprintf(
                     '%s%s',
                     CLI::color($this->addPadding($command[0], 2, $maxPad), 'green'),
-                    CLI::wrap($command[1]),
+                    CLI::wrap($command[1], 0, $maxPad),
                 ));
             }
         }

system/Commands/Server/Serve.php

@@ -37,9 +37,9 @@ class Serve extends AbstractCommand
     protected function configure(): void
     {
         $this
-            ->addOption(new Option(name: 'php', description: 'The PHP binary to use.', acceptsValue: true, default: PHP_BINARY))
-            ->addOption(new Option(name: 'host', description: 'The host to serve on.', acceptsValue: true, default: 'localhost'))
-            ->addOption(new Option(name: 'port', description: 'The port to serve on.', acceptsValue: true, default: '8080'));
+            ->addOption(new Option(name: 'php', description: 'The PHP binary to use.', requiresValue: true, default: PHP_BINARY))
+            ->addOption(new Option(name: 'host', description: 'The host to serve on.', requiresValue: true, default: 'localhost'))
+            ->addOption(new Option(name: 'port', description: 'The port to serve on.', requiresValue: true, default: '8080'));
     }
 
     protected function execute(array $arguments, array $options): int

system/Language/en/Commands.php

@@ -20,7 +20,7 @@
     'arrayOptionEmptyArrayDefault'          => 'Array option "--{0}" cannot have an empty array as the default value.',
     'argumentAfterArrayArgument'            => 'Argument "{0}" cannot be defined after array argument "{1}".',
     'duplicateArgument'                     => 'An argument with the name "{0}" is already defined.',
-    'duplicateCommandName'                  => 'Warning: command "{0}" is defined as both legacy ({1}) and modern ({2}); the legacy command will execute. Please rename or remove one.',
+    'duplicateCommandName'                  => 'Warning: The "{0}" command is defined as both legacy ({1}) and modern ({2}). The legacy command will be executed. Please rename or remove one.',
     'duplicateOption'                       => 'An option with the name "--{0}" is already defined.',
     'duplicateShortcut'                     => 'Shortcut "-{0}" cannot be used for option "--{1}"; it is already assigned to option "--{2}".',
     'emptyCommandName'                      => 'Command name cannot be empty.',
@@ -47,6 +47,7 @@
     'noArgumentsExpected'                   => 'No arguments expected for "{0}" command. Received: "{1}".',
     'nonArrayArgumentWithArrayDefault'      => 'Argument "{0}" does not accept an array default value.',
     'nonArrayOptionWithArrayValue'          => 'Option "--{0}" does not accept an array value.',
+    'optionClashesWithExistingNegation'     => 'Option "--{0}" clashes with the negation of negatable option "--{1}".',
     'optionNoValueAndNoDefault'             => 'Option "--{0}" does not accept a value and cannot have a default value.',
     'optionNotAcceptingValue'               => 'Option "--{0}" does not accept a value.',
     'optionalArgumentNoDefault'             => 'Argument "{0}" is optional and must have a default value.',

…ts/_support/_command/DuplicateLegacy.php …/_support/Duplicates/DuplicateLegacy.phptests/_support/_command/DuplicateLegacy.php renamed to tests/_support/Duplicates/DuplicateLegacy.php tests/_support/_command/DuplicateLegacy.php renamed to tests/_support/Duplicates/DuplicateLegacy.php

@@ -11,16 +11,19 @@
  * the LICENSE file that was distributed with this source code.
  */
 
-namespace App\Commands;
+namespace Tests\Support\Duplicates;
 
 use CodeIgniter\CLI\BaseCommand;
 
 /**
+ * Lives outside any `Commands/` directory so discovery does not pick it up
+ * automatically. Tests inject this via a mocked FileLocator.
+ *
  * @internal
  */
 final class DuplicateLegacy extends BaseCommand
 {
-    protected $group       = 'Fixtures';
+    protected $group       = 'Duplicates';
     protected $name        = 'dup:test';
     protected $description = 'Legacy fixture that collides with a modern command of the same name.';
 

…ts/_support/_command/DuplicateModern.php …/_support/Duplicates/DuplicateModern.phptests/_support/_command/DuplicateModern.php renamed to tests/_support/Duplicates/DuplicateModern.php tests/_support/_command/DuplicateModern.php renamed to tests/_support/Duplicates/DuplicateModern.php

@@ -11,15 +11,18 @@
  * the LICENSE file that was distributed with this source code.
  */
 
-namespace App\Commands;
+namespace Tests\Support\Duplicates;
 
 use CodeIgniter\CLI\AbstractCommand;
 use CodeIgniter\CLI\Attributes\Command;
 
 /**
+ * Lives outside any `Commands/` directory so discovery does not pick it up
+ * automatically. Tests inject this via a mocked FileLocator.
+ *
  * @internal
  */
-#[Command(name: 'dup:test', description: 'Modern fixture that collides with a legacy command of the same name.', group: 'Fixtures')]
+#[Command(name: 'dup:test', description: 'Modern fixture that collides with a legacy command of the same name.', group: 'Duplicates')]
 final class DuplicateModern extends AbstractCommand
 {
     protected function execute(array $arguments, array $options): int

tests/system/CLI/AbstractCommandTest.php

@@ -26,6 +26,8 @@
 use CodeIgniter\Exceptions\LogicException;
 use CodeIgniter\Test\CIUnitTestCase;
 use CodeIgniter\Test\StreamFilterTrait;
+use Config\App;
+use Config\Services;
 use PHPUnit\Framework\Attributes\After;
 use PHPUnit\Framework\Attributes\Before;
 use PHPUnit\Framework\Attributes\CoversClass;
@@ -193,6 +195,14 @@ public static function provideCollectionLevelOptionRegistrationIsRejected(): ite
                 ['name' => 'test', 'negatable' => true, 'default' => false],
             ],
         ];
+
+        yield 'option name clashes with existing negation' => [
+            'Option "--no-test" clashes with the negation of negatable option "--test".',
+            [
+                ['name' => 'test', 'negatable' => true, 'default' => false],
+                ['name' => 'no-test'],
+            ],
+        ];
     }
 
     public function testRenderThrowable(): void
@@ -204,6 +214,21 @@ public function testRenderThrowable(): void
         $this->assertStringContainsString('Invalid "background" color: "Background".', $this->getStreamFilterBuffer());
     }
 
+    public function testRenderThrowableSwapsNonCliRequestAndRestores(): void
+    {
+        // Seed the shared request with a non-CLI instance so renderThrowable()
+        // exercises the swap-and-restore branch.
+        Services::createRequest(config(App::class));
+        $incoming = Services::get('request');
+
+        $command = new AppAboutCommand(new Commands());
+
+        $this->assertSame(EXIT_ERROR, $command->bomb());
+        $this->assertStringContainsString('Invalid "background" color: "Background".', $this->getStreamFilterBuffer());
+
+        $this->assertSame($incoming, Services::get('request'));
+    }
+
     public function testCheckingOfArgumentsAndOptions(): void
     {
         $command = new Help(new Commands());
@@ -509,6 +534,24 @@ public static function provideValidationOfOptions(): iterable
             ['foo' => null],
         ];
 
+        yield 'array option requiring value passed without value [app:about a --baz]' => [
+            OptionValueMismatchException::class,
+            'Option "--baz" requires a value to be provided.',
+            ['baz' => null],
+        ];
+
+        yield 'array option requiring value passed without value multiple times [app:about a --baz --baz]' => [
+            OptionValueMismatchException::class,
+            'Option "--baz" requires a value to be provided.',
+            ['baz' => [null, null]],
+        ];
+
+        yield 'array option requiring value mixing value and no-value [app:about a --baz=v1 --baz]' => [
+            OptionValueMismatchException::class,
+            'Option "--baz" requires a value to be provided.',
+            ['baz' => ['v1', null]],
+        ];
+
         yield 'option not accepting value passed with value [app:about a --no-header=value]' => [
             OptionValueMismatchException::class,
             'Option "--no-header" does not accept a value.',