fix(occ): don't let one broken command make occ unusable (#33895)

[DeepDiver1975]

Summary

Fixes #33895 — a single broken console command could make all of occ unusable.

OC\Console\Application::loadCommandsFromInfoXml() loaded each app-provided command and let any failure propagate out of occ. The existing try/catch only handled QueryException (falling back to new $command() or throwing for an unknown class), so:

• a command class that no longer exists, or
• a command whose constructor throws — e.g. an ArgumentCountError or any other PHP \Error/\Throwable that is not a QueryException

bubbled all the way up and aborted the entire command loader. The practical impact: one broken app makes occ unable to run any command — including the maintenance:* / upgrade / repair commands an administrator needs to recover from that very situation.

Change

Wrap each per-command load in an outer try/catch (\Throwable):

• A broken command is now logged via ILogger::warning (with the command name and error message) and skipped, and the loop continues loading the remaining commands.
• The existing query → new $command() → unknown-class logic is preserved byte-for-byte as the inner block, so the normal/success path registers exactly the same commands as before. The outer catch only changes behavior when a command actually fails.
• \Throwable is used deliberately so it covers both \Exception and \Error (the ArgumentCountError-style constructor failures this issue is about).

Why this is safe

• No new code-execution path is introduced — new $command() was already present; this only broadens error tolerance around it.
• Failures are not swallowed silently — each is logged with enough context (command name + message) for an admin to find and fix the offending app.
• Identical behavior when nothing is broken.

Tests

Adds tests/lib/Console/ApplicationTest.php (testBrokenCommandDoesNotAbortLoading): given a command list containing a broken (unknown-class) command followed by a valid one, loadCommandsFromInfoXml() must not throw and the valid command must still be registered.

Tagged @group DB because it relies on the ownCloud test bootstrap (\OC::$server for the logger); it does not need an actual DB connection. php -l clean; the full PHPUnit suite was not run in the preparation environment.

Note: owncloud/core is in maintenance mode; this targets installations still on classic ownCloud 10.x. Labelled junior job / sev3-medium on the issue.

---

🤖 This PR was prepared by the Claude Code review agent from the analysis of #33895. Please review carefully before merging.

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[DeepDiver1975]

🤖 Automated code review by Claude Code review agent

Overview

Wraps each per-command load in OC\Console\Application::loadCommandsFromInfoXml() in an outer try/catch (\Throwable) so a single broken app command is logged and skipped instead of aborting the whole occ loader. Adds tests/lib/Console/ApplicationTest.php. The change is small, well-scoped, and directly addresses #33895. I verified the fix against the local clone (Symfony Console v7.4.7, PHP >= 8.3).

Correctness

• \Throwable covers \Error: ✅ Correct. \Throwable is the common ancestor of both \Exception and \Error (PHP 7+), so it catches ArgumentCountError, TypeError, etc. thrown from a command constructor — exactly the failure class the issue describes.
• Success path unchanged: ✅ The original query → new $command() → unknown-class block is preserved byte-for-byte as the inner try/catch. On the happy path the inner block never throws, the outer catch is never entered, and the same commands register as before.
• addCommand is the real method: ✅ Verified lib/composer/symfony/console/Application.php (installed v7.4.7) defines public function addCommand(callable|Command $command). Production code already used $this->application->addCommand($c) and the PR did not change that line, so this is not a regression risk. The test's RecordingApplication::addCommand($command) matches the production call site.
• Logger signature valid: ✅ ILogger::warning($message, array $context = []) — the call passes a message string and a context array (command, message, app). {command}/{message} PSR-3-style placeholders match the context keys. Fine.
• Failures not swallowed silently: ✅ Each skip is logged with command name + error message.

Tests

I traced the new testBrokenCommandDoesNotAbortLoading end-to-end against the actual container/test infra:

• Test\TestCase::invokePrivate() ✅ exists (static, tests/lib/TestCase.php:176), and correctly handles both the private-property set ('application') and the private-method invoke ('loadCommandsFromInfoXml').
• newInstanceWithoutConstructor() ✅ appropriate — avoids needing OC_Defaults / a real SymfonyApplication; the test then injects RecordingApplication into the private $application property.
• Broken command OC\This\Command\Does\Not\Exist: query() → resolve() → ReflectionException → wrapped QueryException → class_exists() false → throw new \Exception(...) → caught by outer \Throwable → logged + skipped. ✅
• Valid command ValidStubCommand::class: Command::__construct(?string $name = null) is fully optional, so the container autowires it (or the new $command() fallback runs); either way $c instanceof ValidStubCommand. ✅
• Assertions: assertCount(1, registered) and assertInstanceOf(ValidStubCommand::class, registered[0]). ✅ Both hold — exactly one command survives. No sort-order/ordering assumption issues (single survivor).
• Namespace/PSR-4: Test\ → tests/lib/ (composer autoload-dev), so Test\Console\ApplicationTest → tests/lib/Console/ApplicationTest.php. ✅ Matches the existing Test\Files\ subdir convention.
• @group DB: justified — the test reaches \OC::$server->getLogger() and the DI container, which need the OC bootstrap. ✅
• php -l on the production file: clean.

The test is correct and should pass in CI.

Minor

• Three classes in one file (ValidStubCommand, RecordingApplication, ApplicationTest). Common and acceptable for test fixtures; PHPUnit loads by path so PSR-4 single-class-per-file is not required here. No action needed.
• RecordingApplication is a structural duck-type, not a real SymfonyApplication — fine because loadCommandsFromInfoXml only calls addCommand(). If the loader ever calls another method on $this->application, the test fixture would need updating; a short comment to that effect already exists.
• Optional: the warning context sets 'app' => 'core', but the failing command belongs to an app, not core. Harmless (it identifies the logging subsystem), but the offending app id is not captured. Not blocking.

Verdict

approve-with-nits. The fix is correct, minimal, and safe; the happy path is provably unchanged; and the new test is sound and should pass in CI. No blocking issues. The only suggestions are cosmetic (logging the originating app id). Good defensive fix for a real admin-recovery footgun.

[phil-davis]

This looks good. For each PR like this we should have a changelog entry.
Our convention is that the changelog entry has a filename that is the PR number. In practice that is difficult. For real people, like me, I "guess" the PR number when doing my code and changelog, and hope that when I push a few minutes later, that I get that next number.

Claude is going to really struggle with that convention. Is there some other changelog naming convention that we can be happy to use, and then could Claude also write a suggested changelog entry with each PR?