Skip to content

CLI: exempt commands with positional arguments from the sibling-subcommand check#19492

Open
dereuromark wants to merge 4 commits into
cakephp:5.nextfrom
dereuromark:cli-subcommand-guard-positional-args
Open

CLI: exempt commands with positional arguments from the sibling-subcommand check#19492
dereuromark wants to merge 4 commits into
cakephp:5.nextfrom
dereuromark:cli-subcommand-guard-positional-args

Conversation

@dereuromark
Copy link
Copy Markdown
Member

Fixes the core half of #19491 (Cluster A).

Problem

The sibling-subcommand guard added in #19425 rejects an unknown token after a command that has registered subcommands, on the assumption that the token is a mistyped subcommand:

Unknown command `cake bake template authors`.
Available subcommands: `bake template all`.

That assumption holds for parent commands that take no arguments (i18n, cache), but it is wrong for commands that declare their own positional arguments. There the trailing token is a legitimate argument, not a subcommand.

Bake is the concrete victim: BakePlugin registers both bake template and bake template all (and the same for controller, model, fixture). TemplateCommand accepts a name argument, so bake template Articles is a valid invocation, yet the guard refuses it because bake template all shares the prefix. Only --flag-first invocations slipped through, which is why some tests stayed green.

The original change already documented the intended exemption ("Commands without siblings, commands with declared positional arguments, and option-like tokens are unaffected") - the positional-argument part was just never implemented.

Fix

A token can only be told apart from a subcommand by the matched command's own option parser. So resolve and instantiate the command first, then skip the guard when its parser declares positional arguments:

$command = $this->getCommand($io, $commands, $name);

if (
    isset($argv[0])
    && !str_starts_with($argv[0], '-')
    && $this->hasCommandsWithPrefix($commands, $name)
    && !$this->commandHasArguments($command)
) {
    // ... reject unknown subcommand
}

commandHasArguments() reads the resolved command's option parser and returns whether it defines any positional arguments. The command is now instantiated once and reused for both the check and execution.

Typo protection for argument-less parent commands (cake i18n nonsense) is preserved.

Notes

  • This is the core-side fix only. Cluster B in the issue (uninitialized typed property in Bake's *AllCommand::buildOptionParser()) is a Bake bug and is fixed separately in the bake plugin.
  • New regression test: a command declaring a positional argument alongside a sibling subcommand runs with its argument instead of erroring. Existing guard tests (argument-less parent still errors; option-like token unaffected) continue to pass.

…mmand check

The sibling-subcommand guard added for 5.4 rejects an unknown token after a
command that has registered subcommands, treating it as a mistyped subcommand.
That assumption is wrong for commands which declare their own positional
arguments: there the trailing token is a legitimate argument, not a subcommand.

This broke every Bake command of that shape, e.g. `bake template Articles` is
rejected because `bake template all` is also registered, even though
`TemplateCommand` accepts a `name` argument. Only the command's own option
parser can tell a positional argument apart from a subcommand name.

Resolve the command first, then skip the guard when its parser declares
positional arguments. This restores the behavior the original change already
promised ("commands with declared positional arguments are unaffected") while
keeping the typo protection for parent commands that take no arguments.
protected function commandHasArguments(CommandInterface $command): bool
{
if (!$command instanceof BaseCommand) {
return false;
Copy link
Copy Markdown
Member

@ADmad ADmad Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we err on the side of caution and assume the command does have positional arguments? Ensuring a valid command runs is more important than providing a meaningful error for an invalid usage.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Inverted the helper to isArgumentlessCommand(): it now only returns true when the parser can be inspected and declares zero arguments. If the parser can't be determined (not a BaseCommand, or getOptionParser() throws) it returns false, so the guard is skipped and the command runs. That also stops a command whose parser throws (e.g. the Bake *AllCommand uninitialized-property case) from being masked by a bogus unknown-subcommand error. Pushed in bf2baa8.

@dereuromark dereuromark added this to the 5.4.0 milestone Jun 5, 2026
…ined

When the option parser cannot be inspected, assume the command may take
positional arguments and skip the sibling-subcommand check, so a valid
command runs instead of being rejected with an unknown-subcommand error.
@dereuromark dereuromark marked this pull request as ready for review June 6, 2026 14:12
@dereuromark dereuromark requested a review from markstory June 6, 2026 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants