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 4 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 Makes every path that lists a namespace's commands in the legacy PHP CLI go through the CLI's own list command, so Changes
Flow Diagramflowchart TD
A[CLI invoked with a name] --> B{names a command?}
B -- yes --> C[normal dispatch]
B -- no --> D{names a namespace?}
D -- no --> E[parent doRun: usual error handling]
D -- yes --> F[Application::listNamespace -#gt; list command]
F --> G[bare namespace: stderr, exit 1]
H[help <namespace>] --> D
D -- yes --> I[listNamespace with --format/--raw: stdout, exit 0]
|
![upsun-dispatch[bot]](https://avatars.githubusercontent.com/in/4153575?s=60&v=4){kind=small}
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>
doRun() looks the name up before the parent does, and the help command looks it up again for the same name, so keep the answer. Let anything unexpected fall through to the parent, which reports it through the error event and the usual exception rendering, rather than escaping doRun(). Also record which callers pay for resolving every command, and why the namespace listing does not dispatch ConsoleEvents::ERROR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YRnasDG9njauyLG5BGfkKz
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning · 🔵 3 minor points
🔍 Full review · 3 files reviewed
🔍 What this review checked
- HelpCommand only enters the namespace branch when
$this->commandis null, so<command> --help(which sets it via wantHelps) still describes the command. listNamespace()only adds--format/--rawwhen set, and ListCommand's definition declares both options plus thenamespaceargument, so the ArrayInput validates.DescriptorUtils::describeNamespaces()already unwrapped LazyCommand and filtered on isHidden()/isEnabled(), solist <ns>output is unaffected by the newall()override.getDescribableNamespace()returns null for--version/-Vand for a null command name, so bareupsunstill runs thewelcomedefault command.lookUpDescribableNamespace()triesfind()beforefindNamespace(), andfind()short-circuits onhas($name), so exact command dispatch does not resolve every lazy command.- No command in legacy/src/Command uses
AsCommand(hidden: true), so unwrapping LazyCommand cannot un-hide a command that CommandBase::isHidden() considers visible.
Verification. The diff adds three integration tests (integration-tests/list_namespace_test.go) covering the three listing paths, completion and a disabled_commands override; CI runs them in the integration-test job (make integration-test) and lints the PHP in the php job (make lint-phpstan, make lint-php-cs-fixer, ./scripts/test/unit.sh). No test covers list <hidden-only-namespace> --all (api, blue-green, session, version, project:variable), <namespace> --help, or the exit status/stream of the bare-namespace path.
Review details
- Commit: 2315faa
- Model: claude-opus-5
Review 3 of 10 for this pull request · View the full run
| $commands = parent::all($namespace); | ||
| foreach ($commands as $name => $command) { | ||
| if ($command instanceof LazyCommand) { | ||
| $commands[$name] = $command->getCommand(); |
There was a problem hiding this comment.
🟡 Warning — --all exists to list hidden commands, and it now errors for whole namespaces.
Unwrapping LazyCommand in all() makes Application::getNamespaces() (which skips $command->isHidden()) drop every namespace whose commands are all hidden: api, blue-green, session, version and project:variable (verified — ApiCurlCommand, SessionSwitchCommand set $hiddenInList = true; the BlueGreen/Version commands are ALPHA; the ProjectVariable commands are hidden+deprecated; no visible command or alias supplies those prefixes). Symfony\...\ApplicationDescription::inspectApplication() and CustomTextDescriptor::describeApplication() (line 111) both call $application->findNamespace($describedNamespace), which now throws NamespaceNotFoundException. So upsun list api --all (and the same for the other four namespaces) fails with 'There are no commands defined in the "api" namespace.' instead of listing the hidden commands — through the Go wrapper this surfaces as a non-zero legacy exit and exitWithError. The same lookup in Application::find() ($this->findNamespace(substr($name, 0, $pos))) turns a typo like api:crl into that message instead of 'Command "api:crl" is not defined. Did you mean api:curl?'.
| */ | ||
| public function doRun(InputInterface $input, OutputInterface $output): int | ||
| { | ||
| if (($namespace = $this->getDescribableNamespace($input)) !== null) { |
There was a problem hiding this comment.
🔵 Minor — An explicit --help request exits 1 with empty stdout, breaking scripted help.
The namespace check runs before parent::doRun() handles --help/-h, so upsun project --help and upsun project -h now take this branch: the listing goes to the error output and the method returns 1. An explicit help request therefore writes nothing to stdout and exits non-zero, while upsun help project writes to stdout and exits 0 — the PR's own table presents the two as equivalent. upsun project --help > out.txt yields an empty file and a failing exit status.
| $listInput = new ArrayInput($args); | ||
| $listInput->setInteractive(false); | ||
|
|
||
| return $this->get('list')->run($listInput, $output); |
There was a problem hiding this comment.
🔵 Minor — A vendor config that disables list turns every namespace listing into an error.
listNamespace() resolves the list command with $this->get('list'). Application::add() returns null for a command in application.disabled_commands, so with list disabled it is never registered and get('list') throws CommandNotFoundException. upsun project and upsun help project then render 'Command "list" is not defined.' instead of the namespace listing the parent used to produce with its own DescriptorHelper. A vendor distribution disabling list — the very configuration mechanism this PR targets — hits this on every namespace invocation.
| require.NoError(t, err) | ||
|
|
||
| // A namespace is not a command, so the CLI reports a failure, as before. | ||
| viaNamespace, viaNamespaceErr, _ := f.RunCombinedOutput("project") |
There was a problem hiding this comment.
🔵 Minor — The behaviour the change promises to keep is not actually asserted.
The exit status of the bare-namespace run is discarded (_), here and at line 110 for the abbreviation, and the split between stdout and stderr is erased by concatenating both into namespaceListing. The PR states that streams and exit codes are unchanged (stderr, non-zero), but nothing in the test would fail if doRun() returned 0 or wrote the listing to stdout. Asserting err != nil and that viaNamespace (stdout) is empty would pin the contract the change claims to preserve.
Superseded: the latest Upsun Dispatch review no longer requests changes.
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