From 62e3aea186eed5c1f803104291bca29c3711b63f Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 26 Nov 2025 12:08:45 -0500 Subject: [PATCH] feat(app): allow removing installed-but-disabled apps; improve help & logging - Allow removal of apps that are installed but currently disabled - Add explicit installed check (getAppPath) with clear message when not installed - Expand help text to document uninstall behavior and --keep-data - Mirror console messages to logs for key events and consolidate exception logging - miscellaneous code tidying Signed-off-by: Josh --- core/Command/App/Remove.php | 89 ++++++++++++++++++++++--------------- 1 file changed, 54 insertions(+), 35 deletions(-) diff --git a/core/Command/App/Remove.php b/core/Command/App/Remove.php index d43bfa96ccc59..13e2990d850c6 100644 --- a/core/Command/App/Remove.php +++ b/core/Command/App/Remove.php @@ -9,6 +9,7 @@ namespace OC\Core\Command\App; use OC\Installer; +use OCP\App\AppPathNotFoundException; use OCP\App\IAppManager; use Psr\Log\LoggerInterface; use Stecman\Component\Symfony\Console\BashCompletion\Completion\CompletionAwareInterface; @@ -32,7 +33,14 @@ public function __construct( protected function configure(): void { $this ->setName('app:remove') - ->setDescription('remove an app') + ->setDescription('Remove an app from this Nextcloud instance') + ->setHelp( + "Removes the specified app and, if present, runs the app's uninstall steps.\n" . + "\n" . + "By default, this command runs the app's uninstall steps (which may delete data) and then removes the app files.\n" . + "Use `--keep-data` to skip uninstall steps and preserve app data (database tables, configuration, and stored files).\n" . + "Note: Some apps may still preserve data either way, depending on their uninstall implementation.\n" + ) ->addArgument( 'app-id', InputArgument::REQUIRED, @@ -42,63 +50,73 @@ protected function configure(): void { 'keep-data', null, InputOption::VALUE_NONE, - 'keep app data and do not remove them' + 'Do not run uninstall tasks; preserve app data and configuration' ); } protected function execute(InputInterface $input, OutputInterface $output): int { - $appId = $input->getArgument('app-id'); + $appId = (string) $input->getArgument('app-id'); + $keepData = (bool) $input->getOption('keep-data'); - // Check if the app is enabled - if (!$this->manager->isEnabledForAnyone($appId)) { - $output->writeln($appId . ' is not enabled'); - return 1; + // Prevent removal of shipped/core apps + if ($this->manager->isShipped($appId)) { + $output->writeln("App '$appId' is a shipped/core app and cannot be removed."); + return self::FAILURE; } - // Removing shipped apps is not possible, therefore we pre-check that - // before trying to remove it - if ($this->manager->isShipped($appId)) { - $output->writeln($appId . ' could not be removed as it is a shipped app'); - return 1; + // Prevent removal of apps that aren't even installed (note: don't use isInstalled(); it's a misnomer) + try { + $this->manager->getAppPath($appId); + } catch (AppPathNotFoundException $e) { + $output->writeln("App '$appId' is not installed. Nothing to remove."); + return self::FAILURE; // one could argue this a no-op and should be considered a success (?) } - // If we want to keep the data of the app, we simply don't disable it here. - // App uninstall tasks are being executed when disabled. More info: PR #11627. - if (!$input->getOption('keep-data')) { + $appVersion = $this->manager->getAppVersion($appId); + + // Do not run the specified app's uninstall tasks -- preserving app data/config -- if requested + if ($keepData) { + $message = "Removing app '$appId' but keeping app data (uninstall hooks skipped)."; + $output->writeln($message); + $this->logger->info($message, [ 'app' => 'CLI', ]); + } else { + // Disable the app before removing to trigger uninstall steps try { $this->manager->disableApp($appId); - $output->writeln($appId . ' disabled'); + $message = "Disabled app '$appId' (uninstall steps executed)."; + $output->writeln($message); + $this->logger->info($message, [ 'app' => 'CLI', ]); } catch (Throwable $e) { + $message = "Failed to disable app '$appId' (version $appVersion) - app removal skipped."; $output->writeln('Error: ' . $e->getMessage() . ''); - $this->logger->error($e->getMessage(), [ - 'app' => 'CLI', - 'exception' => $e, - ]); - return 1; + $output->writeln("\n" . $message); + $this->logger->error($message, [ 'app' => 'CLI', 'exception' => $e, ]); + return self::FAILURE; } } - // Let's try to remove the app... + // Remove the specified app try { - $result = $this->installer->removeApp($appId); + $removeSuccess = $this->installer->removeApp($appId); } catch (Throwable $e) { + $removeSuccess = false; $output->writeln('Error: ' . $e->getMessage() . ''); - $this->logger->error($e->getMessage(), [ - 'app' => 'CLI', - 'exception' => $e, - ]); - return 1; + $this->logger->error("Failed to remove app '$appId': " . $e->getMessage(), [ 'app' => 'CLI', 'exception' => $e, ]); } - if ($result === false) { - $output->writeln($appId . ' could not be removed'); - return 1; + // Something went wrong during removeApp(); probably no removal took place or incomplete + if (!$removeSuccess) { + $message = "\nFailed to remove app '$appId' (version $appVersion) - app files/registration were not removed."; + $output->writeln($message); + $this->logger->error($message, [ 'app' => 'CLI', ]); + return self::FAILURE; } + + $message = "Removed app '$appId' (version $appVersion)."; + $output->writeln($message); + $this->logger->info($message, [ 'app' => 'CLI', ]); - $appVersion = $this->manager->getAppVersion($appId); - $output->writeln($appId . ' ' . $appVersion . ' removed'); - - return 0; + return self::SUCCESS; } /** @@ -117,6 +135,7 @@ public function completeOptionValues($optionName, CompletionContext $context): a */ public function completeArgumentValues($argumentName, CompletionContext $context): array { if ($argumentName === 'app-id') { + // TODO: Include disabled apps too return $this->manager->getEnabledApps(); } return [];