Add occ app:remove CLI command

[patschi]

While PR #11053 recently added the possibility for updating apps and PR #5644 to install apps, basically just removing apps is missing... This PR adds the command to be able to remove apps using CLI.

I've decided to use the term remove because of two reasons:

1. In OC\Installer the function is also called removeApp()
2. To prevent any confusion when using term uninstall with command install

Usage is simple as that:

# sudo -u www-data php occ app:install news
news installed
news enabled
# sudo -u www-data php occ app:remove news
news removed
# sudo -u www-data php occ app:remove news
news is not installed

Now we have all important CLI commands to be able to manage apps using the command line, perfectly for automation tasks.

[skjnldsv]

Code looks 👍

[ChristophWurst]

Should we have a mechanism to allow apps to delete their data when they are removed? I think this command gives the impression that it would leave the instance if as the app was never installed, while database and app data entries remains.

[skjnldsv]

Should we have a mechanism to allow apps to delete their data when they are removed? I think this command gives the impression that it would leave the instance if as the app was never installed, while database and app data entries remains.

The mechanism is in the appSettings management indeed, but not in the Installer class:

server/settings/Controller/AppSettingsController.php

Lines 510 to 513 in 9057621

	if($result !== false) {
	$this->appManager->clearAppsCache();
	return new JSONResponse(['data' => ['appid' => $appId]]);
	}

[patschi]

@skjnldsv Based on the comment in following lines, the clearAppsCache() does also not clear the data of the deleted app, just the cached list of apps:

server/lib/public/App/IAppManager.php

Lines 143 to 147 in 2e36069

	/**
	* Clear the cached list of apps when enabling/disabling an app
	* @since 8.1.0
	*/
	public function clearAppsCache();

On some further research, apparently uninstall steps are executed when disabling the app:

server/lib/private/App/AppManager.php

Lines 286 to 303 in 2e36069

	public function disableApp($appId) {
	if ($this->isAlwaysEnabled($appId)) {
	throw new \Exception("$appId can't be disabled.");
	}
	unset($this->installedAppsCache[$appId]);
	$this->appConfig->setValue($appId, 'enabled', 'no');
	// run uninstall steps
	$appData = $this->getAppInfo($appId);
	if (!is_null($appData)) {
	\OC_App::executeRepairSteps($appId, $appData['repair-steps']['uninstall']);
	}
	$this->dispatcher->dispatch(ManagerEvent::EVENT_APP_DISABLE, new ManagerEvent(
	ManagerEvent::EVENT_APP_DISABLE, $appId
	));
	$this->clearAppsCache();
	}

You're right, @ChristophWurst. The default behavior seems to be to perform app uninstall steps when disabling the app, so same should happen when using the occ app:remove command as well.

However: Would it make sense to provide a additional parameter to allow to keep the data of the app when removing? Something like: ./occ app:remove news --keep-data

[skjnldsv]

However: Would it make sense to provide a additional parameter to allow to keep the data of the app when removing? Something like: ./occ app:remove news --keep-data

Oh that's a really great idea!! 🎉 👍

[nickvergessen]

so why would you remove but keep data, instead of just disabling the app?

[skjnldsv]

so why would you remove but keep data, instead of just disabling the app?

Downgrade ? I just did it with contacts ;)
I wanted to keep the data but replace the app 😝

[nickvergessen]

Downgrade is not a supported usecase.

But well, fine by me.

[patschi]

When removing an app I would basically do following:

1. $appManager->disableApp($appId);
2. $installer->remove($appId);

I'd use disableApp() here, because of two reasons:

1. To re-use the existing code, which also performs the uninstall steps when disabling and is firing the disableApp-event (in case any other code would like to react on it)
2. When removing an app through the web UI, basically the exact same steps happen. So this way we have similar behavior in both cases when using UI and CLI.

Any suggestions?

[ChristophWurst]

Minor thing. Apart from that it's ready to be merged IMO 👍 Nice work!

[patschi]

When <error>MESSAGE</error> should be used? Only for Exceptions and critical errors? Just asking so that we can keep that behavior consistent across other commands. I'd use the error formatting for e.g. thrown exceptions.

[ChristophWurst]

When <error>MESSAGE</error> should be used? Only for Exceptions and critical errors? Just asking so that we can keep that behavior consistent across other commands. I'd use the error formatting for e.g. thrown exceptions.

I doubt we have any clear rules for it. Use it where it makes sense, I'd say 😉

FYI: there's also the option to make text bold, which I now use to emphasize some important sections of the output like at https://github.com/ChristophWurst/twofactor_admin/blob/d9cac99b10f3b2bf56427aa2dd64cabadd5713ad/lib/Command/Generate.php#L70

[patschi]

Okay! Thanks for the information! I've added the <error> styling to the exceptions, which should be quite rare in normal cases anyway :)

[patschi]

Is this probably something which is worth being backported to stable14 (NC 14.0.3)? Or just NC 15+?

[ChristophWurst]

Is this probably something which is worth being backported to stable14 (NC 14.0.3)? Or just NC 15+?

It's a feature and we only backport fixes. This will be 15+ only.

resolved

[ChristophWurst]

@patschi for the sake of having atomic commits and a clean git history, I'd appreciate if you could squash these commits into a single one 😉

@nickvergessen final review? I'd like to have your feedback on this because you know the app management code a lot better.

[skjnldsv]

What is the status here?

[MorrisJobke]

What is the status here?

If the button is green then click it ;) I'm not the only one that is allowed to do so. 😉