fix(list): list a namespace's commands the same way everywhere - #137
fix(list): list a namespace's commands the same way everywhere#137vrobert78 wants to merge 3 commits into
Conversation
Naming a namespace listed different commands than "list <namespace>", and
listed commands that are meant to be hidden:
$ upsun project $ upsun list project
project:clear-build-cache project:clear-build-cache
project:create [create] project:create (create)
project:curl <-- hidden
project:delete project:delete
project:get [get] project:get (get)
project:info project:info
project:list [projects|pro] project:list (projects, pro)
project:set-remote project:set-remote (set-remote)
project:variable:delete <-- hidden, deprecated
project:variable:get <-- hidden, deprecated
project:variable:set <-- hidden, deprecated
Asking for help on a namespace did not list it at all: "upsun help project"
reported that the name was ambiguous, and the commands it suggested included
the hidden ones. "upsun project --help" printed the generic application help.
All of it comes from one cause: Symfony reads isHidden() from the LazyCommand
wrapper, which reports the AsCommand attribute it was built from, while this
CLI decides on the command itself — CommandBase::isHidden() combines the
configured hidden_commands, the command's stability and its own flag, and
deliberately ignores that attribute. The CLI's own list command is right
because DescriptorUtils::describeNamespaces() resolves the wrapper first.
Resolve lazily-loaded commands in Application::all(), so the parent sees each
command's own state wherever it enumerates them. That covers describing a
namespace, completing a command name, and collecting the namespaces
themselves: "upsun proj" used to be ambiguous only because the hidden
project:variable:* commands made "project:variable" a second namespace. It
costs about 50ms per invocation of anything that lists commands.
Then run our own list command for a namespace, from Application::doRun() and
from the help command, so the formatting matches too, along with the DEPRECATED
markers and the commands a vendor distribution disables through
disabled_commands — the default descriptors never checked isEnabled(), so those
were listed even though running them fails with "The command ... is not
enabled."
As before, the bare namespace writes to the error output and reports that no
command was run; help writes to stdout and succeeds, since it was asked for.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR makes namespace invocations (e.g., upsun project, upsun project --help, and upsun help project) list commands consistently with upsun list <namespace>, ensuring hidden and config-disabled commands are not advertised due to Symfony LazyCommand wrapper behavior.
Changes:
- Resolve
LazyCommandinstances in the legacy SymfonyApplication::all()so enumeration uses each command’s realisHidden()/isEnabled()logic. - Route “namespace-as-command” execution (
upsun <namespace>) through the CLI’s customlistcommand to unify formatting and filtering. - Enhance the legacy
helpcommand sohelp <namespace>lists the namespace vialist, including--format/--rawsupport; add integration tests covering all listing paths, completion, and disabled commands.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
legacy/src/Command/HelpCommand.php |
Detects when help is requested for a namespace and delegates output to list to match CLI formatting/filtering. |
legacy/src/Application.php |
Resolves lazy-loaded commands during enumeration and uses list output for namespace invocations to avoid listing hidden/disabled commands. |
integration-tests/list_namespace_test.go |
Adds integration coverage for consistent namespace listings, hidden-command suppression, completion suggestions, and vendor-disabled commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CI failed on RouteListTest: the route list came back in its human-readable form, ignoring the --format, --columns and --no-header options the test passes. findNamespace() collects the namespaces from every command, which now resolves every command, and it was called for any name that is not an exact command name — including an ordinary abbreviation such as "app:config" or "env:url". So a normal invocation constructed every command in the application. That is what broke the test. Services are shared, and InputInterface is synthetic, set per run: a command constructed while another command's input is current keeps that input, and its Table then reports the wrong format. The tests share one Application across runs, so the commands constructed early by one test were still in place, with a stale input, for a later one. Try find() first, as the parent does, and only look for a namespace when the name is not a command at all. That keeps command resolution on the paths that genuinely enumerate commands, and takes ~50ms back off every other invocation: 8 runs of "app:config --help" go from 2.5s to 2.1s, which is where main is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
📋 PR Summary This incremental update refactors the namespace-listing logic so the shared resolution and list-running code lives on the Changes
|
Review feedback on the duplication: the help command re-implemented both the find()-then-findNamespace() resolution and the ArrayInput that runs the list command, so the two copies had to stay in step — including the ordering that keeps full command resolution off the ordinary path. Both now live on Application, as findDescribableNamespace() and listNamespace(), and the help command calls them. listNamespace() also only passes on the options that are set, as SubCommandRunner::forwardStandardOptions() does, rather than passing false for --raw when the flag is absent. The vendored Symfony accepts that value, so this is a convention rather than a fix, but it keeps the input free of options the caller never gave. Covers "help <namespace>" with --raw and with --format=json, which pass the help command's options through to the listing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Problem
Naming a namespace lists different commands than
list <namespace>does, and includes commands that are meant to be hidden:Asking for help on a namespace did not list it at all:
And a command a vendor distribution disables through
disabled_commandswas still advertised. With the Akeneo configuration, which disablesproject:create:Cause
Symfony reads
isHidden()from theLazyCommandwrapper, which reports theAsCommandattribute it was built from. This CLI decides hidden-ness on the command itself:CommandBase::isHidden()combines the configuredhidden_commands, the command's stability and its own flag, and deliberately ignores that attribute. The CLI's own list command is correct becauseDescriptorUtils::describeNamespaces()resolves the wrapper before asking.Three places read the wrapper:
Application::doRun(), when the name is a namespace, describes it with aDescriptorHelperit builds itself — default descriptors, which also never checkisEnabled(), hence the disabled commands.Application::complete()skips$command->isHidden()when suggesting command names.Application::getNamespaces()skips hidden commands when collecting namespaces.Fix
Application::all()now resolves lazily-loaded commands, so the parent sees each command's own state wherever it enumerates them. ThenApplication::doRun()and the help command run the CLI's own list command for a namespace, so the formatting, theDEPRECATEDmarkers and the disabled-command filtering match as well.Streams and exit codes are unchanged: the bare namespace writes to the error output and reports that no command was run;
help <namespace>writes to stdout and succeeds, since it was asked for.help <namespace>also honours--format, so it can produce the same JSON aslist <namespace>.Effect, measured
upsun project/project --help/help projectlist projectproject:curl,api:curl,project:variable:*upsun projCommand "proj" is ambiguousprojectnamespaceproject:create, any listing path_completelatency, 5-run averageupsun projimproves becausegetNamespaces()was countingproject:variable— a namespace that exists only because of the hidden deprecated commands — so any abbreviation matching both was ambiguous.The ~50 ms is the cost of resolving every command, and it lands on anything that enumerates them, including each Tab press. Verified by driving real Tab presses in a Debian container:
upsun project:<TAB>lists the seven visible commands and no longer offerscurlorvariable:*.Verification
integration-tests/list_namespace_test.gocovers the three listing paths, the abbreviation, the completion suggestions and a vendor configuration that disables a command. All three tests fail against unmodifiedmainand pass here. Full integration suite green; golangci-lint, php-cs-fixer and PHPStan (level 8, PHP 8.4) clean;legacyPHPUnit at its existing baseline.Known remaining difference
upsun list projectincludesproject:init, which the namespace listings do not: that command is implemented in Go, and the Golistcommand adds it to the output the legacy CLI produces. The legacy CLI cannot know about it, so closing that gap would mean the Go layer intercepting namespace invocations, which needs the command list up front and would cost an extra PHP start on every pass-through. The test encodes this explicitly rather than hiding it.🤖 Generated with Claude Code