Pocketmine-mp: /reload, re-imagined

[lukeeey]

Description

A few things that /reload did were not very useful and misleading, like reloading plugins. This doesn't mean that it was entirely useless, though.

For example, it could be used to reload config files (plugin configs / language files, ops list, ban list, server.properties etc). I think we can all agree that quickly editing these files and reloading is a useful and needed feature (especially during public plugin development).

Of course we can implement commands into our plugins to reload our configs, however this means a lot of extra commands and still does not solve the issue that we cannot reload server config files.

Proposal

1) Re-implement the /reload command, with different arguments.

/reload - reload all config files and plugins
/reload config <enum: [ops, bans, ip-bans, server]> - reload specified config
/reload plugin <string: name> - reload specified plugin

2) Add an onReload() method to Plugin and PluginBase that is called when the /reload command is executed

This method should not be called by plugins at all

Justification

I feel like this removal was not completely thought out, and i also feel that the reload functionality was not implemented very well in the first place. This proposal re-introduces the /reload command with the useful features it once had, while leaving out the not-so-useful features that gave the feature a bad reputation.

This is targeted at 4.0 as the reload command has already been removed there and it would be a BC break to implement it in 3.x.

---

p.s. whitelist reloading is already handled in the /whitelist command and is not covered by this proposal, however it may be useful to add it to the config enum.

[SOF3]

• Some plugins load config.yml into array during startup
• Some plugins don't use getConfig at all
• The ability to adjust settings during server execution involves a higher maintenance cost. For example, reducing the number of async workers during runtime could be a bit complex.
• What plugins do in onReload is unclear and subject to those plugins' own specification. It is misleading to execute it in general. In particular, plugins could implement their own config reloading commands, and I see no point to merge all these reload commands into one.

I am voting against this proposal due to the inconsistency and unexpectable and undefinable behaviour caused by this command.

[dktapps]

I agree with @SOF3's judgement. A few additional points:

• It is impractical to reload configuration in many cases where the configurations are used to initialize the server runtime.
• I think that the idea of "reload plugin" introduces the exact same misleading ambiguity of functionality that we had to deal with with /reload to begin with. It doesn't really make any sense.

[lukeeey]

• Plugins would obviously have to change the behaviour to account for reloading (and implementing onReload() is completely optional, so existing plugins would continue to work
• Why would it matter if they use getConfig()? They should implement their own logic to reload the configs or whatever other data should be reloaded
• Thats a good point, so i suggest only allowing reloading server.properties, as pretty much all those settings (except port) can be changed while the server is running
• That is a good point, however by having it in a single command it actually would provide consistency - to the user, at least. As with onDisable(), we should hope the plugin developer does nothing stupid in it that could break anything.

[lukeeey]

@dktapps Actually i do agree with that second point but idk what else could be used instead

[lukeeey]

As for the first point, i guess the developer should choose which values should be changed once they have reloaded the config file

[SOF3]

• This is completely optional, but users don't know which plugins would reload when they run the command, which means, the command is ambiguous and some expected and some unexpected stuff happen randomly.
• And they won't. Plugins would rarely remember to implement the non critical reload function. If they would spend that effort, they could as well register a specific command. Or even better, register a file watcher on the config and reload automatically.
• As you said, except port. Which means inconsistency. Users don't know which properties are reloadable and which ones aren't, so again, it just does something unexpected and didn't do something expected by chance. Users can't determine which.
• It is inconsistency, because it is unclear how much it does. As I said, registering a file watcher would be a better approach.

[SOF3]

As for the first point, i guess the developer should choose which values should be changed once they have reloaded the config file

And the user doesn't know which values the developer magically decided to choose, so it is just a random command that works by pure chance.

[SOF3]

The way you're putting it is that reload is just a random miscellaneous feature that plugins might support. So for every verb in the dictionary, I can create an onVerb function, create a /verb command and expect some plugins to implement it (and some not). This is a slippery slope.

[SOF3]

So for every verb in the dictionary, I can create an onVerb function, create a /verb command and expect some plugins to implement it (and some not).

I just revisited this issue and noticed that this is actually not unreasonable. While putting every verb into the Plugin interface is unnecessary, it is possible to explicitly register actions that plugin developers tend to use the same verb to describe. If at least one plugin requires such an action, the server can automatically register a command that does this and execute all such implementations.

This can already be implemented by an API plugin, but it also makes sense arguably to put it in the core.

(Disclaimer: This has nothing to do with /reload in particular, and /reload isn't gonna come back)