Repository: pmmp/PocketMine-MP (language: php)

Issue:
Disallow non-shutdown plugin disabling
## Description
Following #2823, it is now possible to further regularize the plugin loading lifecycle.

This issue proposes three alternatives:
- Plugins must not request to disable **other** plugins, but they can suicide anyway
- Plugins must not be disabled at all, except the natural disabling on shutdown or during startup crash
- Plugins must not be disabled even during startup crash

I personally tend to implement the first alternative, but the other two are also open for discussion.

## Justification
Why plugins should not disable other plugins:
- This may cause instability of other plugins. It should be more convenient to disable
- Disabling plugins should be a concept local to the plugin itself and be handled by itself. PocketMine should not be so authoritarian and force plugin disabling into their API. Plugins tend to have complex multifold mechanisms, and it is usually more desirable to only disable certain components, e.g. a player using a world edit plugin might want to turn off world edit mode for themselves only, but not to disable it for the whole server; or the owner just wants to pause a plugin from running for a while; these should be explicitly supported by plugins, not brutally deafening their event handlers.

Why plugins should not disable themselves between startup and shutdown:
- Give me a reason why they should. I have actually never seen a plugin doing this. Disabling a plugin actually just deafens their listeners, hide their commands and kill their scheduler tasks. It is not even conceptually "disabled", but just isolated from the PocketMine API.
- Because they can't crash on startup; they fetched startup data asynchronously that did not arrive immediately.
  - Let's promisify server startup, allowing plugins to hold back startup. This would be more useful even beyond the scope of this issue.

Why plugins should not disable themselves on startup:
- If they crash due to uncoverable invalid configuration etc., just throw an exception instead of explicitly calling `disablePlugin`. Let PocketMine decide whether to just disable the plugin or crash the server.
- If some config value asked the plugin to disable... Why do you even need this? If you don't want to enable a plugin, simply move it to another folder or change its file extension. You don't need to make a config value to ask to disable your own plugin!

But here comes the question: I only raised why they aren't needed (rebutting the advantages), but didn't provide a motivation to remove them (suggesting the disadvantages). Indeed, there is no motivation to remove except "make the code cleaner and easier". The first alternative has a positive impact on plugin developers, while the second and third only improve the simplicity of PocketMine itself, so I would personally support just restricting disabling other plugins.

But how to detect if it is the plugin disabling itself? Simply add an API that only expects the plugin itself to call; API design is not security design, and we don't need to defend it. But we can still make it clear that our API design does not expect killing other plugins.


Discussion / hints:
Let plugins disable themselves for example PluginX requires a special internet connection(for example it should be connected over Tor socks proxy) and without that its useless why should be enabled any more?
@naja82 in that instance, pluginX should check for that connection onLoad instead of onEnable. Then it can prevent itself from being enabled in the first place.
I usually disable some plugins on production. For example I used devtools on development and disable it on production coz it's not useful and I'm lazy to reupload it if I will use it again.
@SonsaYT04 https://github.com/pmmp/PocketMine-MP/pull/2783 (not sure if it was backported to stable)

Alternatively, you could just rename plugin.yml like to plugin.ym then it wont load
@SonsaYT04 the standard suggestion for your case is to move the plugin to a directory like plugins/unused/. Enabling the plugin then disabling it could cause unintended side effects 
On the other hand, if a plugin has an error, it should throw a PluginException during startup so that the *server* disables it or even stops the server. After all, you might not want your server to be online if one of the plugins is not working.
yeah, the plugin graylist has not been backported to stable @lukeeey although there has been a lot of demand for it. Nonetheless, this issue is targeting 4.0 so it doesn't matter. With that said, this probably won't make it to the 4.0 release at this hour.

Fix patch:
diff --git a/src/plugin/PluginManager.php b/src/plugin/PluginManager.php
index 67ca8cc374e..9c6958d4deb 100644
--- a/src/plugin/PluginManager.php
+++ b/src/plugin/PluginManager.php
@@ -486,6 +486,7 @@ public function enablePlugin(Plugin $plugin) : bool{
 		return true; //TODO: maybe this should be an error?
 	}
 
+	/** @internal */
 	public function disablePlugins() : void{
 		while(count($this->enabledPlugins) > 0){
 			foreach($this->enabledPlugins as $plugin){
@@ -503,7 +504,7 @@ public function disablePlugins() : void{
 		}
 	}
 
-	public function disablePlugin(Plugin $plugin) : void{
+	private function disablePlugin(Plugin $plugin) : void{
 		if($plugin->isEnabled()){
 			$this->server->getLogger()->info($this->server->getLanguage()->translate(KnownTranslationFactory::pocketmine_plugin_disable($plugin->getDescription()->getFullName())));
 			(new PluginDisableEvent($plugin))->call();
@@ -532,13 +533,6 @@ public function tickSchedulers(int $currentTick) : void{
 		}
 	}
 
-	public function clearPlugins() : void{
-		$this->disablePlugins();
-		$this->plugins = [];
-		$this->enabledPlugins = [];
-		$this->fileAssociations = [];
-	}
-
 	/**
 	 * Returns whether the given ReflectionMethod could be used as an event handler. Used to filter methods on Listeners
 	 * when registering.


Test patch: