Skip to content

feat(app): allow removing installed-but-disabled apps; improve help & logging #56720

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
joshtrichards wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(app): allow removing installed-but-disabled apps; improve help & logging #56720

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 54 additions & 35 deletions core/Command/App/Remove.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand All @@ -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;
}

/**
Expand All @@ -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 [];
Expand Down
Loading