fix(legacy): make hidden command aliases resolve again - #178
Conversation
Commands are lazy-loaded via the DI container, and Symfony's AddConsoleCommandPass only maps the name and aliases from the AsCommand attribute. Its setAliases() method call also runs after the constructor, so aliases added by setHiddenAliases() in configure() were overwritten. As a result, aliases such as `snapshots`, `logs`, `user:role` and `environment:sql` failed with "Command is not defined". Hidden aliases are now declared with a #[HiddenAliases] attribute. HiddenAliasesPass adds them as extra console.command tags before AddConsoleCommandPass runs, so they are in the lazy command map. CommandBase reads the attribute to keep them out of help, lists and command name completion. The JSON descriptor now has a `hidden_aliases` field, so a command index built from `list --format=json` can include them. A PHPStan stub types Command::getAliases() as string[], which removes several baseline entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Note
Reviewed — No blocking findings · 🔵 2 minor points
🔍 Full review · 29 files reviewed
🔵 Minor points
legacy/src/Application.php:157— Registering hidden aliases in the DI command map also registers their namespaces with Symfony's Application, and namespaces that contain only hidden aliases now shadow the "command not defined" error.snapshot:*,int:actandi:actcreate the namespacessnapshot,intandi;Application::doRun()callsfindDescribableNamespace(),find()throws (ambiguous),findNamespace()succeeds, andlistNamespace()renders a listing thatDescriptorUtils::describeNamespaces()empties again (it skips every non-canonical name). Verified by running the CLI on this checkout:cli snapshot,cli intandcli ieach print onlyAvailable commands for the "<x>" namespace:with no commands and exit 1, andcli help snapshotprints the same with exit 0. On the base branch these names are unknown, so the user getsCommand "snapshot" is not defined.with suggestions.legacy/src/Console/CustomJsonDescriptor.php:150— The newhidden_aliasesfield survives only in the phar's own output.upsun list --format=jsonis served by the Go wrapper (commands/list.go), which unmarshals the legacy JSON intocommands.Command(commands/list_models.go:215-224 — Name, Usage, Aliases, Description, Help, Examples, Definition, Hidden) and re-encodes it withJSONListFormatter; the struct has no field forhidden_aliases, so the key is dropped from the shipped CLI's JSON. Any index built from the released binary'slist --format=json, rather than from the phar directly, will not see hidden aliases.
Verification
- All 20
setHiddenAliases()call sites on the base map 1:1 onto#[HiddenAliases([...])]attributes with identical alias lists; no alias was lost. - No hidden alias collides with another command's name or visible alias, so no entry in AddConsoleCommandPass's lazy command map is silently overwritten.
- HiddenAliasesPass is added before AddConsoleCommandPass at the same priority, so the extra
console.commandtags exist when the lazy map is built — running the CLI resolvessnapshots,int:actand the abbreviationsql-dum. - Descriptors and
Application::complete()operate on resolved CommandBase instances (DescriptorUtils unwraps LazyCommand), solist backupand_completeoutput show only visible aliases. array_values()ingetVisibleAliases()keeps the JSONaliasesfield a list even when the first alias is hidden.
The diff adds legacy/tests/Console/HiddenAliasesTest.php (every declared hidden alias resolves, plus help/JSON visibility) and integration-tests/hidden_aliases_test.go (four aliases via help --format json, plus list backup); these run in the legacy-php and integration-test GitHub Actions jobs, which also run phpstan and php-cs-fixer and report passing for this head. Nothing covers the namespace side effect reported above, and no test asserts on the Go list --format=json payload.
Review details
- Commit: 736c3c7
- Model: claude-opus-5
Review 1 of 10 for this pull request · View the full run
Registering hidden aliases in the lazy command map also made them define namespaces (`snapshot`, `int`, `i`) and match abbreviations. `upsun snapshot` printed an empty namespace listing instead of "Command is not defined", and hidden aliases appeared in suggestions. Hidden aliases now only work in full: - HiddenAliasesPass lists them in a container parameter. - HiddenAliasesCommandLoader leaves them out of getNames(), which Symfony uses for abbreviations and suggestions. - Application::addCommand() removes them from each command's aliases, so they do not define namespaces, and Application::find() resolves them by exact name. The Go list model now keeps the `hidden_aliases` field, so it is not dropped from `list --format=json`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Note
Reviewed — No blocking findings · 🔵 4 minor points
🔁 Incremental · 6 files reviewed
🔵 Minor points
legacy/src/Application.php:143— DeclaringaddCommand()here kills theisCommandEnabled()filter inadd()above it. Symfony 7.4'sApplication::init()picks its adder with a BC shim:(new \ReflectionMethod($this,'add'))->getDeclaringClass() !== (new \ReflectionMethod($this,'addCommand'))->getDeclaringClass()selects$this->add(...), otherwise$this->addCommand(...)(vendor Application.php:1351-1357). Before this commit onlyadd()was overridden here, so the default commands (help, list, _complete, completion) went through the config check; now both methods are declared in this class, the classes compare equal, andgetDefaultCommands()is registered via the newaddCommand(), which does not consultConfig::isCommandEnabled(). Combined withhas()/all()already callingaddCommand()directly for lazily loaded services (vendor line 642),add()'s check is now dead code for every registration path, and a name inapplication.disabled_commands/wrapped_disabled_commandsis still registered and runnable.legacy/src/Application.php:146— Stripping the hidden aliases off the command object makeshas()andget()disagree for a hidden alias:Application::has('snapshots')still returns true (the loader has it, andaddCommand()returns the command), but the command is now registered only underbackup:list, soApplication::get('snapshots')reaches the!isset($this->commands[$name])branch and throwsCommandNotFoundException: The "snapshots" command cannot be found because it is registered under multiple names.Callers that guard withhas()beforeget()(a normal Symfony idiom; the repo's own guards happen to use canonical names only) get a misleading exception instead of the command.legacy/src/Console/HiddenAliasesCommandLoader.php:38—getNames()subtracts the hidden-alias list from the loader's names by value, with nothing guarding against a hidden alias that collides with another command's real name or visible alias. No such collision exists today (I compared all 23#[HiddenAliases]entries against every#[AsCommand]name and alias), but if one is ever added the real command silently disappears fromall(), fromlist, from abbreviation matching and from completion, andApplication::find()resolves the name to the hidden alias's command instead.HiddenAliasesPasscould detect the clash at compile time and fail the build.legacy/src/Application.php:86—getParameter(HiddenAliasesPass::PARAMETER)is unguarded, and the container is loaded fromconfig/cache/container.phpwhenever that file exists, with no staleness check (seecontainer()below). A checkout with a container cached before this change — e.g. a developer switching to this branch without re-runningcomposer installormake clean— now fails with an uncaughtParameterNotFoundExceptionon every command, instead of the previous behaviour of quietly missing the aliases.hasParameter()with an empty-array fallback would degrade instead of crashing.
Verification
- No hidden alias collides with any
#[AsCommand]name or visible alias — I extracted all 23 hidden aliases and compared them against every command's name and aliases. find()'s new branch resolves an exact hidden alias to the canonical name via the loader, and still honours--helpbecauseget()applieswantHelps.- Excluding hidden aliases from
getNames()plus stripping them inaddCommand()keeps them out ofgetNamespaces(), sofindDescribableNamespace('snapshot')returns null. - The Go
Commandstruct's newhidden_aliasesfield is re-encoded byJSONListFormatter, which marshalsListdirectly, so the key survives the wrapper. - Both descriptors unwrap
LazyCommandbefore reading aliases, so stripping aliases from the lazy wrapper does not change help or list output.
New PHPUnit cases (testHiddenAliasesDoNotDefineNamespaces, testHiddenAliasesAreNotAbbreviated) run in the legacy-php job and the extended Go TestHiddenAliases runs in the integration-test job; gh pr checks 178 shows both passing. Nothing covers the config-disabled-command path (isCommandEnabled) that the new addCommand() override now bypasses, nor a stale container cache.
Review 2 of 10 for this pull request · View the full run
Symfony 7.4 registers commands via addCommand(). Overriding both add() and addCommand() made it skip add() for default commands too, so the disabled_commands check never ran. The check now lives in addCommand(), and add() is no longer overridden. Hidden aliases are resolved in get() instead of find(), so has() and get() agree for them. HiddenAliasesPass fails the build when a hidden alias clashes with a command name, an alias, or another hidden alias. The application falls back to no hidden aliases when a stale container cache lacks the parameter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
📋 PR Summary This PR fixes hidden command aliases such as Changes
|
Aliases set with
setHiddenAliases(), such assnapshots,logs,user:roleandenvironment:sql, failed withCommand "..." is not defined.Commands are lazy-loaded via the DI container. Symfony's
AddConsoleCommandPassonly maps names and aliases from theAsCommandattribute, and itssetAliases()method call runs after the constructor, overwriting aliases set inconfigure(). Visible aliases were unaffected: they are all inAsCommand.#[HiddenAliases([...])]attribute.HiddenAliasesPassadds them as extraconsole.commandtags beforeAddConsoleCommandPassruns, so they are in the lazy command map.CommandBase::getHiddenAliases()reads the attribute; help, lists, list column widths and command name completion exclude them.hidden_aliasesfield, kept by the Golistmodel, so a command index built fromlist --format=json(fix: resolve abbreviations of native commands #176) can include them.Command::getAliases()asstring[], removing several baseline entries.Hidden aliases only work in full:
HiddenAliasesCommandLoaderleaves them out ofgetNames()(used for abbreviations and suggestions), andApplication::addCommand()removes them from each command's aliases so they do not define namespaces such assnapshot.Application::get()resolves them by exact name.HiddenAliasesPassfails the build if a hidden alias clashes with another name.Application::add()is replaced by anaddCommand()override, which Symfony 7.4 uses for all registrations; thedisabled_commandscheck now applies to default commands too.Tests: a PHPUnit test (checks all declared hidden aliases resolve, and help/JSON output) and a Go integration test.
🤖 Generated with Claude Code