Unable to forcefully unregister commands in bukkit namespace

[Reminant]

CommandAPI version: 5.12

Server version: Paper version git-Paper-762 (MC: 1.16.5) (Implementing API version 1.16.5-R0.1-SNAPSHOT)

What I did:

CommandAPI running as plugin on the server.

class Main extends JavaPlugin {
    @Override
    public void onEnable() {
        CommandAPI.unregister("help", true);
        CommandAPI.unregister("me", true);
    }
}

Actual result:

• "help" command not unregistered, neither was "bukkit:help".
• "me" command was unregistered and so was "minecraft:me"
• No error messages

Relevant console output:

[11:44:07 INFO]: [CommandAPI] Loading CommandAPI v5.12
[11:44:07 INFO]: [CommandAPI] Hooked into Spigot successfully for Chat/ChatComponents
[11:44:07 INFO]: [CommandAPI] Enabling CommandAPI v5.12
[11:44:07 INFO]: [CommandAPI] Linked 27 Bukkit permissions to commands
[11:44:07 INFO]: [CommandAPI] Reloading datapacks...
[11:44:08 INFO]: [CommandAPI] Finished reloading datapacks

[JorelAli]

Yup. Can confirm that the CommandAPI doesn't unregister stuff from the Bukkit namespace, because the code literally doesn't do that - it only unregisters Minecraft commands from the Minecraft namespace because it was designed to unregister Vanilla commands because the CommandAPI registers Vanilla commands.

I will look into trying to improving the unregistration system at a later date after 6.0.0's release.

[JorelAli]

@Reminant So it turns out, Bukkit commands are fairly complicated!

You can't unregister bukkit:help. Bukkit commands are stored in a CommandMap and the main server's command map contains a mapping of the command name and its corresponding command object. This map doesn't actually store the bukkit:help command!

I think the best way to implement this is to use a separate method (e.g. CommandAPI.unregisterBukkit()) instead of trying to merge it all into the CommandAPI.unregister() method.

I have two questions:

Command prefixes

• Commands can be prefixed (typically with a plugin name or otherwise). Would you expect unregistering to require a prefix, or unregister all instances of a command regardless of prefix?

Say I have my own plugin MyPlugin which registers a command plugins. If I ran this method:

CommandAPI.unregisterBukkit("plugins");

Should it unregister the following:

• /bukkit:plugins
• /myplugin:plugins
• /plugins

Or should it unregister the following:

• /plugins

Alternatively, should the CommandAPI enforce that a prefix is present? If so, what about unprefixed commands? (Both /plugins and /bukkit:plugins is present in the list of known commands)

Aliases

• Commands have aliases. Would you expect unregistering a command to unregister all subsequent aliases?

Say I have this method:

CommandAPI.unregisterBukkit("version");

Should it unregister the following:

• /about
• /bukkit:about
• /bukkit:ver
• /bukkit:version
• /ver
• /version

Or should it unregister the following:

• /bukkit:version
• /version

[DerEchtePilz]

@JorelAli
I obviously didn‘t open the issue but want to give answers to the questions:

1. If you have the method call CommandAPI.unregisterBukkit(“plugins“) it should unregister every instance of the command, including prefixes like bukkit:. But I think it would also be an alternative to have the unregisterBukkit() method and then does an action based on what you passed into the method. So if you pass in plugins it would unregister every instance like myplugin:plugins, bukkit:plugins and plugins. When you pass in bukkit:plugins it would only unregister bukkit:plugins

2. I think you could add a boolean into the unregisterBukkit() method. If it is set to true all aliases would be unregistered, otherwise only the commands that match the command name that was passed into the method.

[DerEchtePilz]

For this issue, I noticed something surprising, but it is totally possible I messed something up.
I used this code, Minecraft 1.20.1, CommandAPI 9.2.0:

Bukkit.getScheduler().runTaskLater(this, () -> {
            CommandAPIBukkit.unregister("help", true, true);
            CommandAPI.unregister("me", true);
        }, 0);

As expected, help and bukkit:help are being unregistered. me and minecraft:me, however, are not.
Is there something I am missing?

[willkroboth]

@DerEchtePilz Hm, for me it does work. All of the commands in question do not show up for tab-complete, and they can not be run. Specifically, I have this:

@Override
public void onEnable() {
    CommandAPI.onEnable();

    Bukkit.getScheduler().runTaskLater(this, () -> {
        CommandAPIBukkit.unregister("help", true, true);
        CommandAPI.unregister("me", true);
    }, 0);
}

Do you call CommandAPI.onEnable() before or after scheduling the unregister task? If you call it after, I think the problem is that when the unregister task runs, it doesn't know that the server is enabled, because the CommandAPI's delayed init tasks aren't done yet. Since the unregister task doesn't think the server is enabled, it doesn't bother removing the commands from Bukkit's CommandMap.

Also, are you able to run the me command, or does it only appear in the tab-complete?

[DerEchtePilz]

Yeah, I am calling CommandAPI.onEnable() after I unregister these commands. But that should not matter. If I know what to do before the server is running, I should be able to do it before the server is running.

As for the /me command, I am fully able to run it.

[willkroboth]

I think the placement of CommandAPI.onEnable() is important here. Here's the code that runs when the CommandAPI is enabled on Bukkit:

CommandAPI/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/CommandAPIBukkit.java

Lines 198 to 217 in e48dc6d

	@Override
	public void onEnable() {
	JavaPlugin plugin = config.getPlugin();
	// Prevent command registration after server has loaded
	new Schedulers(paper).scheduleSyncDelayed(plugin, () -> {
	CommandAPI.stopCommandRegistration();
	// Sort out permissions after the server has finished registering them all
	fixPermissions();
	if (paper.isFoliaPresent()) {
	CommandAPI.logNormal("Skipping initial datapack reloading because Folia was detected");
	} else {
	reloadDataPacks();
	}
	updateHelpForCommands(CommandAPI.getRegisteredCommands());
	}, 0L);
	paper.registerReloadHandler(plugin);
	}

Note that the CommandAPI schedules its own task to take care of a couple of things after the server is loaded. One of those is calling CommandAPI.stopCommandRegistration():

CommandAPI/commandapi-core/src/main/java/dev/jorel/commandapi/CommandAPI.java

Lines 44 to 59 in c78c686

	/**
	* Flag that commands should no longer be registered. After running this,
	* {@link CommandAPI#canRegister()} will return false.
	*/
	public static void stopCommandRegistration() {
	CommandAPI.canRegister = false;
	}
	/**
	* Determines whether command registration is permitted via the CommandAPI
	*
	* @return true if commands can still be registered
	*/
	public static boolean canRegister() {
	return canRegister;
	}

This is important because CommandAPIBukkit#unregister relies on the value of CommandAPI#canRegister (See lines 575 and 591)

CommandAPI/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/CommandAPIBukkit.java

Lines 562 to 609 in e48dc6d

	private void unregisterInternal(String commandName, boolean unregisterNamespaces, boolean unregisterBukkit) {
	CommandAPI.logInfo("Unregistering command /" + commandName);
	if(!unregisterBukkit) {
	// Remove nodes from the Vanilla dispatcher
	// This dispatcher doesn't usually have namespaced version of commands (those are created when commands
	// are transferred to Bukkit's CommandMap), but if they ask, we'll do it
	removeBrigadierCommands(getBrigadierDispatcher(), commandName, unregisterNamespaces, c -> true);
	// Update the dispatcher file
	CommandAPIHandler.getInstance().writeDispatcherToFile();
	}
	if(unregisterBukkit || !CommandAPI.canRegister()) {
	// We need to remove commands from Bukkit's CommandMap if we're unregistering a Bukkit command, or
	// if we're unregistering after the server is enabled, because `CraftServer#setVanillaCommands` will have
	// moved the Vanilla command into the CommandMap
	Map<String, Command> knownCommands = commandMapKnownCommands.get((SimpleCommandMap) paper.getCommandMap());
	// If we are unregistering a Bukkit command, DO NOT unregister VanillaCommandWrappers
	// If we are unregistering a Vanilla command, ONLY unregister VanillaCommandWrappers
	boolean isMainVanilla = isVanillaCommandWrapper(knownCommands.get(commandName));
	if(unregisterBukkit ^ isMainVanilla) knownCommands.remove(commandName);
	if(unregisterNamespaces) {
	removeCommandNamespace(knownCommands, commandName, c -> unregisterBukkit ^ isVanillaCommandWrapper(c));
	}
	}
	if(!CommandAPI.canRegister()) {
	// If the server is enabled, we have extra cleanup to do
	// Remove commands from the resources dispatcher
	// If we are unregistering a Bukkit command, ONLY unregister BukkitCommandWrappers
	// If we are unregistering a Vanilla command, DO NOT unregister BukkitCommandWrappers
	removeBrigadierCommands(getResourcesDispatcher(), commandName, unregisterNamespaces,
	c -> !unregisterBukkit ^ isBukkitCommandWrapper(c));
	// Help topics (from Bukkit and CommandAPI) are only setup after plugins enable, so we only need to worry
	// about removing them once the server is loaded.
	getHelpMap().remove("/" + commandName);
	// Notify players
	for (Player p : Bukkit.getOnlinePlayers()) {
	p.updateCommands();
	}
	}
	}

If CommandAPI.onEnable() is called after scheduling the unregister task, the unregister task will run before CommandAPI.stopCommandRegistration() is called. Even though the server has moved the Vanilla commands into the Bukkit CommandMap, CommandAPI#unregister does not know about this. So, the me command isn't removed from the Bukkit CommandMap and it can still be run.

Have you tried calling CommandAPI.onEnable() before scheduling the unregister task to see if that works? I could be missing some other difference, but putting CommandAPI.onEnable() before scheduling the unregister task makes it work for me.

[DerEchtePilz]

I mean, yeah, it does work when calling CommandAPI.onEnable() first. But it really shouldn't only work this way. To me, it seems really unintuitive and it should be considered to update this method again.

According to the order of operations you could probably argue that this is correct this way.
But I also think that delaying this stuff should be a task of the CommandAPI. We could probably easily put this complete method in a task that runs once the server is loaded. We could then require that unregistrations are made in the onEnable method of a plugin (since you can't register a plugin task if the server is still loading), but by doing this we would remove this (seemingly) arbitrary restriction of only being able to unregister commands once the server has started.

[willkroboth]

Oh, got it. I think I see what you're saying. It is not obvious that this would be a problem just looking at it from the outside, so yeah, something needs to be improved.

Note, however, that in most cases, unregistration does work. You can usually unregister commands before and after the server has started (see the docs; all of those cases are valid and tested). The bug here is that command unregistration does not work after the server has started, but before the CommandAPI's delayed initialization task has run.

I see 3 ways to fix this:

1. Add a note to the docs. If a developer is shading and wants to unregister commands in a delayed init task, then they must call CommandAPI.onEnable() before scheduling their unregister task. This would ensure that CommandAPI.stopCommandRegistration() is called before the unregisteration task is run, which I assumed would be the case when rewriting CommandAPIBukkit#unregister.
2. Rewrite CommandAPIBukkit#unregister to assume that CommandAPI.canRegister() is always false. It should be fine to assume the server is always enabled. The extra tasks that get run to ensure proper cleanup should work anyway even when the server is not yet enabled. Of course, testing is necessary to validate that statement. This would just mean that unregistration would do a bunch of extra stuff when it doesn't need to.
3. Find a better way to call CommandAPI.stopCommandRegistration(). Ideally, CommandAPI.stopCommandRegistration() would be called as soon as the server is finished enabling, before it runs any delayed init tasks. This would be more accurate to what it's actually supposed to represent. I'm just not sure how to do this. Maybe there's a Bukkit.isServerEnabled() method or ServerEnableEvent the CommandAPI can hook into.

I rank these ideas 3 best, then 1, with 2 worst. Other parts of the CommandAPI depend on the value of CommandAPI#canRegister (For example, CommandAPIBukkit#postCommandRegistration), so it would be best if it accurately represented when Spigot is done helping us with normal command registration. 1 is fine; it works, but it just puts the burden on developers and could be easily missed, causing confusion. I think 2 is kinda just ignoring the problem.

[willkroboth]

Ah, I found exactly what I was looking for to implement idea 3. The ServerLoadEvent is called after all the plugins are enabled, just before the server starts its main loop.

I implemented this (ee9077a), and tested it for this application. It looks like it works. With this code:

@Override
public void onEnable() {
    Bukkit.getScheduler().runTaskLater(this, () -> {
        CommandAPIBukkit.unregister("help", true, true);
        CommandAPI.unregister("me", true);
    }, 0);

    CommandAPI.onEnable();
}

me and minecraft:me cannot be tab-completed or executed.

[DerEchtePilz]

I also did my own testing and I am also able to confirm that this seems to work great!

[Reminant]

it's been 3 years mate.

…

On Sun, 8 Oct 2023 at 02:34, DerEchtePilz ***@***.***> wrote: I also did my own testing and I am also able to confirm that this seems to work great! — Reply to this email directly, view it on GitHub <#210 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAY4FFEB7FD62JCYENMZXILX6FK4RAVCNFSM46IWNS5KU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCNZVGE3TCNBVGMYA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[JorelAli]

Implemented in CommandAPI 9.3.0.