This is intended to break API in order to jerk the rug out from underneath plugin developers who have been misusing this without noticing the side effects.
Use of this by plugins will produce a lot of undefined behaviour, such as event handlers not being unregistered, scheduled tasks not being removed, and registered permissions causing memory leaks.
This has the triple bonus effect of a) making a lot of code easier to read, b) reducing Server::getInstance() usages, and c) removing a whole bunch of Server dependencies.
The network and block namespaces are untouched by this commit due to potential for merge conflicts. These should be dealt with separately on master.
This is dependent on the changes made in b1e0f82cbf2f585ed729245a6883d713effd1793. This now makes it possible to call events without fetching a Server reference, allowing to eliminate a vast array of Server dependencies.
The basic principle here is "if you're not expecting it, don't catch it".
Event handlers are **never** supposed to throw exceptions. If they do throw exceptions, it's usually going to one of two things;
1. Broken code producing an error
2. Code triggering (and not catching) a runtime error
Both 1) and 2) boil down to defective code on the part of the event handler, and thus should not be caught by the caller, but instead allowed to crash the server and produce a crashdump.
It's also undesirable to catch unexpected errors here for a few other reasons
- It leaves the owner of the event handler in an unknown, potentially unstable state
- It allows broken code to cause event handlers to spam the logger in events that happen frequently (for example movement handlers)
- It allows the process to continue down a train of further undefined behaviour, which may lead to more errors or ultimately a crash, so it makes no sense to hold off the inevitable.
This has a few advantages that are not merely inverted disadvantages:
- Crash dumps will now be created and automatically submitted for defective event handlers, allowing quicker issue location, debugging and fixing in plugins without manual user interaction
- Event calling now isn't dependent on Server to work.
This came to light after observing cfb6856634f91930f6e013e7b98edb638dea15d9 in a fresh light. I noticed that this fix should not have been necessary because clearPermissions() should have dealt with it. Unfortunately, permissions can be set without being set in PermissibleBase->permissions, so this misses things.
* Added a new PermissionManager, remove ridiculous cyclic dependencies of Permissions on Server
Aside from all the other ridiculous design problems with the permission system, the biggest problems are its API. This is, once again, a result of poor API design copied from Bukkit.
This pull request removes all permission-related functionality from `PluginManager` and moves it to the `pocketmine\permission\PermissionManager` class.
As can be observed from the removed code in the diff, the permissions system was previously entirely dependent on the Server, because it needed to get the PluginManager for registering permissions. This is utterly ridiculous. This refactor isolates _most_ permission-related functionality within the `permission` namespace.
As mentioned above, this stupid API is a direct result of copying from Bukkit. If you look at the API documentation for Bukkit for `PluginManager` you will see that the methods I'm deprecating here are also in there.
## Changes
- Added a new `PermissionManager` class. This can be accessed via its singleton `getInstance()` static method.
- Deprecated the following `PluginManager` methods - these will be removed no later than 4.0.0:
- `getPermission()`
- `addPermission()`
- `removePermission()`
- `getDefaultPermissions()`
- `recalculatePermissionDefaults()`
- `subscribeToPermission()`
- `unsubscribeFromPermission()`
- `getPermissionSubscriptions()`
- `subscribeToDefaultPerms()`
- `unsubscribeFromDefaultPerms()`
- `getDefaultPermSubscriptions()`
- `getPermissions()`
I wasn't sure whether this would be considered a bug fix or a feature. Nonetheless, it's a behavioural change, so it belongs in 3.1 if anywhere.
Prior to this, plugins would be required to call saveDefaultConfig() before calling getConfig() or anything else. Calling getConfig() without saveDefaultConfig() first would generate an empty configuration file. Instead, it now saves the default config before loading it.
This is quite an interesting bug. If you have
```php
class A{
public function onMove(PlayerMoveEvent $event){} //shouldn't be a handler because this class isn't a Listener
}
class B extends A implements Listener{}
```
then
```php
registerEvents(new B, $plugin);
```
then `A::onMove()` will be registered as an event handler even though `A` is not an instanceof `Listener`.
This was observed by noting that plugins which do something like `extends PluginBase implements Listener` causes `registerEvents()` to try and register `PluginBase` methods as event handlers, which could lead to astonishing behaviour.
then A::onMove() will be registered as an event handler even though A is not an instanceof Listener.
This was observed by noting that plugins which do something like "extends PluginBase implements Listener" causes registerEvents() to try and register PluginBase methods as event handlers, which could lead to astonishing behaviour.
I considered making this instead save the default config instead of creating an empty config file, but that would be (albeit minor) a behavioural change which therefore belongs in 3.1.
- remove onLoad(), onEnable(), onDisable()
- remove Config related methods
- remove getResource(), saveResource(), getResources()
did I troll any readers so far?
On a more serious note, these methods do not need to be declared in this interface because they are either hooks (`onLoad()`, `onEnable()`, `onDisable()`) or methods only used from within `PluginBase` and its children. They are not intended to be public API, and for this reason they don't need to be exposed in the interface.
PhpStorm can't see these or understand how they are being called, which is very annoying for bug hunting. Additionally, we already have the CodeOptimizer for this.
