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]));
         }
 
@@ -347,9 +351,11 @@ public function hasNegation(string $name): bool
      *   1. {@see initialize()} and {@see interact()} are handed the raw parsed
      *      input by reference, in that order. Both can mutate the tokens before
      *      the framework interprets them against the declared definitions.
-     *   2. The resulting raw input is snapshotted into `$unboundArguments` and
-     *      `$unboundOptions` so the unbound accessors can report what the user
-     *      actually typed (as opposed to what defaults resolved to).
+     *   2. The post-hook input is snapshotted into `$unboundArguments` and
+     *      `$unboundOptions` so the unbound accessors can report the tokens
+     *      carried into binding (as opposed to what defaults resolved to).
+     *      Any mutations performed in `initialize()` or `interact()` are
+     *      therefore reflected in the snapshot.
      *   3. {@see bind()} maps the raw tokens onto the declared arguments and
      *      options, applying defaults and coercing flag/negation values.
      *   4. {@see validate()} rejects the bound result if it violates any of the
@@ -692,33 +698,38 @@ 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.
+                // in the previous loop. The leftover shortcut value can itself be an array when
+                // the shortcut was passed multiple times, so merge arrays and append scalars.
                 $option = $this->shortcuts[$name];
+                $values = is_array($value) ? $value : [$value];
 
                 if (array_key_exists($option, $boundOptions) && is_array($boundOptions[$option])) {
-                    $boundOptions[$option][] = $value;
+                    $boundOptions[$option] = [...$boundOptions[$option], ...$values];
                 } else {
-                    $boundOptions[$option] = [$boundOptions[$option], $value];
+                    $boundOptions[$option] = [$boundOptions[$option], ...$values];
                 }
 
                 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.
+                // to be not bound in the previous loop. The leftover negation value can
+                // be scalar (including a string when the negation was passed with a value)
+                // or an array — normalise to an array before mapping null → false.
                 $option = $this->negations[$name];
-                $value  = array_map(static fn (mixed $v): mixed => $v ?? false, $value ?? [null]);
+                $values = is_array($value) ? $value : [$value];
+                $values = array_map(static fn (mixed $v): mixed => $v ?? false, $values);
 
                 if (! is_array($boundOptions[$option])) {
                     $boundOptions[$option] = [$boundOptions[$option]];
                 }
 
-                $boundOptions[$option] = [...$boundOptions[$option], ...$value];
+                $boundOptions[$option] = [...$boundOptions[$option], ...$values];
 
                 continue;
             }
@@ -816,8 +827,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)) {
@@ -843,7 +860,12 @@ private function validateNegatableOption(string $name, Option $definition, array
             throw new OptionValueMismatchException(lang('Commands.negatedOptionNoValue', [$definition->negation]));
         }
 
-        if (array_values(array_intersect(array_unique($value), [true, false])) === [true, false]) {
+        // Both forms appearing together is the primary user mistake; flag it
+        // regardless of whether either form carried a value.
+        if (
+            array_key_exists($name, $this->unboundOptions)
+            && array_key_exists($definition->negation, $this->unboundOptions)
+        ) {
             throw new LogicException(lang('Commands.negatableOptionWithNegation', [$name, $definition->negation]));
         }
 
@@ -859,7 +881,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

@@ -153,6 +153,26 @@ public function getModernCommands(): array
         return $this->modernCommands;
     }
 
+    /**
+     * Checks if a legacy command with the given name has been discovered.
+     */
+    public function hasLegacyCommand(string $name): bool
+    {
+        return array_key_exists($name, $this->commands);
+    }
+
+    /**
+     * Checks if a modern command with the given name has been discovered.
+     *
+     * A name present in both registries signals a collision; legacy wins
+     * at runtime. Callers can combine this with {@see hasLegacyCommand()}
+     * to detect that case.
+     */
+    public function hasModernCommand(string $name): bool
+    {
+        return array_key_exists($name, $this->modernCommands);
+    }
+
     /**
      * @return ($legacy is true ? BaseCommand : AbstractCommand)
      *
@@ -217,11 +237,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 +270,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 +287,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/CLI/Console.php

@@ -69,7 +69,7 @@ public function run(array $tokens = [])
 
         $this->command = array_shift($arguments) ?? self::DEFAULT_COMMAND;
 
-        if (array_key_exists($this->command, $commands->getCommands())) {
+        if ($commands->hasLegacyCommand($this->command)) {
             return $commands->runLegacy($this->command, array_merge($arguments, $this->options));
         }
 

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