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.
@@ -859,7 +863,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/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.',

tests/system/CLI/AbstractCommandTest.php

@@ -193,6 +193,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

tests/system/CLI/CommandsTest.php

@@ -79,13 +79,14 @@ public function testRunOnUnknownCommand(): void
 
         $this->assertSame(EXIT_ERROR, $commands->runLegacy('app:unknown', []));
         $this->assertArrayNotHasKey('app:unknown', $commands->getCommands());
-        $this->assertStringContainsString('Command "app:unknown" not found', $this->getStreamFilterBuffer());
+        $this->assertSame("\nCommand \"app:unknown\" not found.\n", $this->getUndecoratedBuffer());
 
         $this->resetStreamFilterBuffer();
+        CLI::resetLastWrite();
 
         $this->assertSame(EXIT_ERROR, $commands->runCommand('app:unknown', [], []));
         $this->assertArrayNotHasKey('app:unknown', $commands->getModernCommands());
-        $this->assertStringContainsString('Command "app:unknown" not found', $this->getStreamFilterBuffer());
+        $this->assertSame("\nCommand \"app:unknown\" not found.\n", $this->getUndecoratedBuffer());
     }
 
     public function testRunOnUnknownLegacyCommandButWithOneAlternative(): void
@@ -135,6 +136,7 @@ public function testRunOnUnknownLegacyCommandButWithMultipleAlternatives(): void
                 Command "app:" not found.
 
                 Did you mean one of these?
+                    app:about
                     app:destructive
                     app:info
 
@@ -143,6 +145,42 @@ public function testRunOnUnknownLegacyCommandButWithMultipleAlternatives(): void
         );
     }
 
+    public function testRunOnUnknownLegacyCommandAlsoSuggestsModernAlternatives(): void
+    {
+        $commands = new Commands();
+
+        $this->assertSame(EXIT_ERROR, $commands->runLegacy('app:ab', []));
+        $this->assertSame(
+            <<<'EOT'
+
+                Command "app:ab" not found.
+
+                Did you mean this?
+                    app:about
+
+                EOT,
+            $this->getUndecoratedBuffer(),
+        );
+    }
+
+    public function testRunOnUnknownModernCommandAlsoSuggestsLegacyAlternatives(): void
+    {
+        $commands = new Commands();
+
+        $this->assertSame(EXIT_ERROR, $commands->runCommand('app:inf', [], []));
+        $this->assertSame(
+            <<<'EOT'
+
+                Command "app:inf" not found.
+
+                Did you mean this?
+                    app:info
+
+                EOT,
+            $this->getUndecoratedBuffer(),
+        );
+    }
+
     public function testRunOnUnknownModernCommandButWithMultipleAlternatives(): void
     {
         $commands = new Commands();
@@ -169,7 +207,7 @@ public function testRunOnAbstractLegacyCommandCannotBeRun(): void
 
         $this->assertSame(EXIT_ERROR, $commands->runLegacy('app:pablo', []));
         $this->assertArrayNotHasKey('app:pablo', $commands->getCommands());
-        $this->assertStringContainsString('Command "app:pablo" not found', $this->getStreamFilterBuffer());
+        $this->assertSame("\nCommand \"app:pablo\" not found.\n", $this->getUndecoratedBuffer());
     }
 
     public function testRunOnKnownLegacyCommand(): void
@@ -178,7 +216,10 @@ public function testRunOnKnownLegacyCommand(): void
 
         $this->assertSame(EXIT_SUCCESS, $commands->runLegacy('app:info', []));
         $this->assertArrayHasKey('app:info', $commands->getCommands());
-        $this->assertStringContainsString('CodeIgniter Version', $this->getStreamFilterBuffer());
+        $this->assertSame(
+            sprintf("\nCodeIgniter Version: %s\n", CodeIgniter::CI_VERSION),
+            $this->getUndecoratedBuffer(),
+        );
     }
 
     public function testRunOnKnownModernCommand(): void
@@ -218,12 +259,14 @@ public function testDiscoveryWarnsWhenSameCommandNameExistsInBothRegistries(): v
         $this->copyCommand($modernFixture);
 
         try {
+            $message = wordwrap(
+                'Warning: The "dup:test" command is defined as both legacy (App\\Commands\\DuplicateLegacy) and modern (App\\Commands\\DuplicateModern). The legacy command will be executed. Please rename or remove one.',
+                CLI::getWidth(),
+            );
+
             $commands = new Commands();
 
-            $this->assertStringContainsString(
-                'Warning: command "dup:test" is defined as both legacy (App\\Commands\\DuplicateLegacy) and modern (App\\Commands\\DuplicateModern)',
-                $this->getUndecoratedBuffer(),
-            );
+            $this->assertSame("\n{$message}\n", $this->getUndecoratedBuffer());
             $this->assertArrayHasKey('dup:test', $commands->getCommands());
             $this->assertArrayHasKey('dup:test', $commands->getModernCommands());
         } finally {

tests/system/Commands/HelpCommandTest.php

@@ -173,6 +173,23 @@ public function testDescribeInexistentCommandButWithAlternatives(): void
         );
     }
 
+    public function testDescribeInexistentCommandSuggestsLegacyAlternatives(): void
+    {
+        command('help app:inf');
+
+        $this->assertSame(
+            <<<'EOT'
+
+                Command "app:inf" not found.
+
+                Did you mean this?
+                    app:info
+
+                EOT,
+            $this->getUndecoratedBuffer(),
+        );
+    }
+
     public function testDescribeUsingHelpOption(): void
     {
         command('cache:clear --help');

user_guide_src/source/changelogs/v4.8.0.rst

@@ -28,6 +28,8 @@ Behavior Changes
 - **Commands:** The ``-h`` option to ``routes`` command which was mapped previously to the ``--handler`` option is now removed.
     Use ``--handler`` instead to sort the routes by handler when running the ``routes`` command.
 - **Commands:** The ``filter:check`` command now requires the HTTP method argument to be uppercase (e.g., ``spark filter:check GET /`` instead of ``spark filter:check get /``).
+- **Commands:** Several built-in commands have been migrated from ``BaseCommand`` to the modern ``AbstractCommand`` style. Applications that extend a built-in command to override
+    behaviour may need to re-implement against the modern API (``configure()`` + ``execute()`` and the ``#[Command]`` attribute) once the class it extends is migrated, or, preferably, compose instead of extending. Invocations on the command line are unaffected.
 - **Database:** The Postgre driver's ``$db->error()['code']`` previously always returned ``''``. It now returns the 5-character SQLSTATE string for query and transaction failures (e.g., ``'42P01'``), or ``'08006'`` for connection-level failures. Code that relied on ``$db->error()['code'] === ''`` will need updating.
 - **Filters:** HTTP method matching for method-based filters is now case-sensitive. The keys in ``Config\Filters::$methods`` must exactly match the request method
     (e.g., ``GET``, ``POST``). Lowercase method names (e.g., ``post``) will no longer match.

user_guide_src/source/cli/cli_modern_commands.rst

@@ -141,6 +141,17 @@ A few quirks are worth knowing:
 
 Configuration-time violations raise ``InvalidOptionDefinitionException``.
 
+.. note::
+
+    The command-line parser does **not** understand bundled shortcuts or
+    shortcuts with a glued value:
+
+    - ``-abc`` is read as one option named ``abc``, *not* as ``-a -b -c``.
+    - ``-fvalue`` is read as one option named ``fvalue``; it is not split into
+      shortcut ``-f`` with value ``value``.
+
+    Pass shortcut values with ``-f=value`` or ``-f value`` instead.
+
 *************************
 Interacting With the User
 *************************
@@ -235,6 +246,79 @@ formatted output the framework uses for uncaught exceptions, call
 ``$this->renderThrowable($exception)``. The helper is safe to call from any
 command, and it will not disturb the currently shared request.
 
+**********************************
+Migrating From ``BaseCommand``
+**********************************
+
+The modern command system is a superset of the legacy ``BaseCommand`` API — the
+same capabilities are there, just expressed through an attribute and explicit
+definitions rather than class properties and ad-hoc lookups.
+
+**Identity**
+
+``protected $name``
+    Moves to ``name:`` on the ``#[Command]`` attribute.
+
+``protected $description``
+    Moves to ``description:`` on the ``#[Command]`` attribute.
+
+``protected $group``
+    Moves to ``group:`` on the ``#[Command]`` attribute. An empty group skips the command at discovery.
+
+**Input surface (declare inside** ``configure()`` **)**
+
+``protected $usage``
+    The default usage line is generated from the declared arguments. Register extras with ``addUsage()``.
+
+``protected $arguments``
+    One ``addArgument(new Argument(...))`` call per argument.
+
+``protected $options``
+    One ``addOption(new Option(...))`` call per option. A long name is required; a legacy ``-x``-style
+    option becomes ``new Option(name: 'something', shortcut: 'x')``.
+
+**Runtime**
+
+``run(array $params)``
+    No longer the extension point — ``run()`` is ``final`` on ``AbstractCommand`` and drives the lifecycle itself.
+    Move the body into ``execute(array $arguments, array $options): int``, which must return an ``EXIT_*`` status.
+
+``$params[0]``
+    Use ``$arguments['name']`` or ``$this->getValidatedArgument('name')``.
+
+``$params['name']`` / ``CLI::getOption('name')``
+    Use ``$options['name']`` or ``$this->getValidatedOption('name')``.
+    Call ``$this->hasUnboundOption('name')`` when you need to know whether the flag was actually passed.
+
+``$this->call('other', $params)``
+    Becomes ``$this->call('other', $arguments, $options)``; only from inside ``execute()``.
+    To forward the caller's own raw input, pass ``$this->getUnboundArguments()`` and ``$this->getUnboundOptions()``.
+
+``$this->showError($e)``
+    Becomes ``$this->renderThrowable($e)``.
+
+``showHelp()`` override
+    Gone. The built-in ``help`` command builds the help output itself from the declared arguments, options, and usages.
+
+Prompting the user mid-run stays with ``CLI::prompt()``, but the idiomatic spot moves from ``run()``
+to ``interact()`` so validation can see whatever the user provides interactively.
+
+A typical ``BaseCommand`` implementation:
+
+.. literalinclude:: cli_modern_commands/009.php
+
+…becomes, as a modern command:
+
+.. literalinclude:: cli_modern_commands/010.php
+
+Two behavioural changes are worth calling out explicitly:
+
+- **Validated, not raw.** Arguments and options are parsed, defaulted, and validated before ``execute()`` runs.
+  If a required argument is missing or a ``requiresValue`` option was passed without a value, the framework
+  raises a typed exception and your command is never entered.
+- **Exit codes are mandatory.** Legacy ``run()`` could return ``null``. The modern ``execute()`` must return an
+  integer; the framework emits a deprecation notice for any legacy command that still returns ``null``.
+
 ********************************
 Coexistence With Legacy Commands
 ********************************