Add command plugin compatability into GeneralCommands

[codeHusky]

CommandBook compat within WorldGuard was gutted some time ago, but the GeneralCommands don't register when CommandBook is present. This functionality is not identical for similar plugins such as Essentials, which have their commands unexpectedly overridden by WorldGuard.

It's worth noting that all functionality within GeneralCommands is not required for the continued functionality of the plugin. CommandBook support for the CommandBook god mode and detecting it is entirely disabled in the current version of WorldGuard, only existing in references to the plugin in the documentation and commented-out code. If this functionality is desired, it should be implemented in a more relevant PR or elsewhere.

This PR makes this functionally identical when using the similar-featured and popular plugin Essentials. Functionality could be added to support other Essentials-like plugins via changing the wording to "Third Party" rather than Essentials and some sort of list, but most complaints seem to be from folks using Essentials.

This PR also adds "GeneralCompatibleCommands", which is basically a wrapper for GeneralCommands with different aliases. I'm not personally familiar with the WorldEdit command system and it seemed the "cleanest" way to handle the alias removal situation would be to just have the wrapper class.

Functionality of this change was verified on Paper 1.20.2 git-Paper-280

Fixes issue #2013

[ThePotatoDev]

@wizjany 🙏

[Joo200]

I don't think it's a good idea to add Essentials as a new soft-depend. Maybe there is a better way to detect registered commands in existing plugin.yml from other plugins.

At some point we should remove the soft-depend to CommandBook and handle the commands only from worldguard. The command aliases prefixed by /wg are fine but the implementation is terrible.

The paper logic in SimpleCommandMap has the following comment when registering a command: // If the command exists but is an alias we overwrite it, otherwise we return We should be able to use that by using the /wg-prefixed command labels as first command in the annotation.

[codeHusky]

@Joo200 I attempted this but it doesn't appear Essentials overrides the commands in that situation. The commands are still handled by WorldGuard

 @Command(aliases = {"wggod", "god"}, usage = "[player]",
            desc = "Enable godmode on a player", flags = "s", max = 1)
    public void god(CommandContext args, Actor sender) throws CommandException, AuthorizationException {
        Iterable<? extends LocalPlayer> targets = null;

Essentials is trying to not override other plugins with its command registrations deliberately, as most of the time that's what people want. Some changes could be made to how these commands are created in the first place (using the non-annotation way of defining commands from WorldEdit) but we still need to ensure WorldGuard loads last so we can, at minimum, check the command map to see if "heal", etc are registered

[Joo200]

No, we don't change the way worldguard registers commands by dropping the annotations or other things.

I'm fine with prefixing the worldguard commands here with /wg to allow other plugins to overwrite worldguard's aliases. This is handled by bukkit and we don't need to change much in WorldGuard. If Essentials doesn't register the command the way Bukkit's API is designed (either by calling getCommand("...").setExecutor(...); or by injecting the CommandMap from Bukkit/Paper it's their issue. We don't add work arounds for that.

[codeHusky]

Realistically then, these commands should be disabled by default. They override other plugins commands and provide functionality that isn't core and essential to WorldGuard's function. Keep them enabled for existing installs, but new configs should probably start with them disabled. As I mention in #2042, "Everything is off by default" is one of the key features of WorldGuard and this behavior is rather contradictory.

Also, I'm not certain, but Essentials might pre-date the Bukkit command API. Not sure when that was added but I feel like I can recall onCommand being the "correct" way to handle commands back in the day.

[wizjany]

yea this is somewhat philosophically misguided from the onset. the reason we tested specifically for commandbook is because we integrated with it. testing for the presence of essentials is useless - if we just want to avoid command conflicts, we need to be testing if the command is registered, not if some specific plugin is loaded (never mind enabled and registering the command itself). an essentials version of the GodMode session handler can be added easily by a third party plugin, but we shouldn't pretend to depend on essentials if we're not actually using their api to do things properly (and i'd like to avoid third party apis anyway).

also you're entirely mis-reading "off by default". that refers to changing default game behavior. using a command to turn on god mode is opting in to changed behavior. this is a nonsense argument.

re: onCommand, no that's very much part of the api, and it still requires command registration. plugin#onCommand is called for the owner(plugin) of the command if it doesn't have an executor set. i.e. it's a fallback/catchall for plugins that don't use individual executors, such as those using their own handling logic (e.g. most command frameworks)

edit: would also like to add that i think /wg-prefixed aliases are ugly, if we're going there we should just throw it as nested commands under the /wg command instead. bukkit already does namespacing here, no need for /worldguard:wggod. again, this goes back to the correct solution being commands.yml, because directing conflicting non-namespaced commands to the plugin that the server owner, not the plugin, chooses is the correct behavior here. but someone will have to take that up with spigot/paper.

[codeHusky]

Given how many issues have been made about these commands being on by default being a "bug" of some kind, I can't help but think that the interpretation of "off by default" you have conflicts with what people are taking it as. I don't see a point in having these commands even strictly included on by default with WorldGuard is in the first place other than "it's always been there." They don't serve most users needs very well in the first place, and the "locate" command especially replaces something very useful for SMP admins with something intrusive and unhelpful.

If long-time users want to use them, they should be able to use them, but many of them override high-use existing features due to their current aliases. The bulk of Bukkit/Spigot/Paper servers use Essentials, and many modern servers want to use vanilla /locate, so it seems a bit pointless to try and keep these commands for new installs. It's not like a fresh config having them off by default will exactly break compat with anything either.

Essentials itself goes the correct route in this situation - commands that get replaced by other plugins have a fallback /e<cmd> alias for each. Yes, the /wg<cmd> alias is ugly but the current functionality is rather gross as-is. I'd love it if the Bukkit namespacing was a perfect solution here but it's not, and choosing to simply not support one of the most popular plugins on Bukkit servers because it's not something you feel like you should have to deal with is a bit silly. WorldGuard is very like Essentials here - it's a core plugin for many servers that serves a specific purpose. That purpose isn't to provide these helper commands, even if the functionality is "already there" and works with a flag. If anything, compatibility with Essentials godmode should have existed ages ago for this reason.

If you don't want a softdep then I guess configuring the time the plugin tries to load itself in the plugin.yml to be late vs essentials and other plugins would make the most sense. Regardless, not having any sort of compatibility measure just seems pointlessly aggressive for a plugin that's primary function is not to provide helpful commands to users. Players hate that kind of stuff - same reason many people don't use certain "Anti Chat Reporting" plugins that replace the messaging commands, etc. It's unexpected features that interfere with things they have deliberately added to their server for a certain purpose. Let's not make server admin more of a headache for people than necessary - it's hard enough to figure out what plugin is providing these pink-text god/ungod commands in the first place.

EDIT: Also, yeah, if onCommand is part of the API (I don't recall, I use lucko/helper's cmd system most of the time myself) then Essentials isn't strictly doing anything wrong.

[Joo200]

I attempted this but it doesn't appear Essentials overrides the commands in that situation.

This is configured by the Essentials config. By adding some prefix to the worldguard gods command and registering the command "god" as alias I was able to configure Essentials with the overridden-commands config to use the essentials god command. Imo this should be the preferred solution for now. Unfortunatly it's not possible to hide them by default as a /wg subcommand.

Indeed the onCommand method is part of Bukkti API, I didn't see Essentials has some workaround for other plugins.

[codeHusky]

I'd still, personally, prefer a config option to just nuke the commands if this particular PR isn't merged. I'd rather not have to fudge with the massive Essentials config to fix an issue that's ultimately on WG's side, especially with commands I'll never use.

[Joo200]

I'm not gonna argue about which plugin is reasonable for this issue. It's not only WG's side. If you don't want to modify Essentials config that's your problem.

Realistically then, these commands should be disabled by default. They override other plugins commands and provide functionality that isn't core and essential to WorldGuard's function. Keep them enabled for existing installs, but new configs should probably start with them disabled.

It's not possible to create a new config value which is disabled by default but enabled for existing installations. I don't mind a new config option to disable those commands but the commands should be enabled by default.

[codeHusky]

It's not possible to create a new config value which is disabled by default but enabled for existing installations.

I mean, that's a bit of an exaggeration - see this commit. Works perfectly in my testing.

[wizjany]

we will not be breaking default behavior.

[codeHusky]

I'm not sure what you mean, the commit does not break any default behavior. It's a new behavior that disables legacy features in fresh configs by default to fit more in line with what players seem to expect. This doesn't affect existing installs, either, so it doesn't break anything. If you mean that you don't want to disable the commands by default on new installs, I'm curious for reasoning as to why. Are you aware of them being used a lot on new servers?