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 <[email protected]>

Author: joshtrichards

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>Error: ' . $e->getMessage() . '</error>');
-				$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>Error: ' . $e->getMessage() . '</error>');
-			$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 [];