Allow bukkit commands in /execute command

[Machine-Maker]

Oh what a beautiful command system bukkit has crammed on top of brigadier...

If there's a better way to fix this, let me know. Main issue is that when the /execute command, and its children are registered, the root node from the dispatcher it's using gets thrown away because a new instance of the dispatcher is created later on (in CraftServer#syncCommands). So the root node it's using is "outdated". This DelegatingRootCommandNode always calls for the latest one. Further changes were needed to make sure the player's are sent the correct redirect node for /execute instead of the "outdated" root node.

[NoahvdAa]

I feel like there should be a config option for this, this looks like it may have a chance of breaking maps designed for vanilla environments.

[electronicboy]

I think that the ultimate end goal is that we replace the SimpleCommandMap with something which actually basically backs the internal command dispatcher, the current command system setup is basically a hacked together clusterfuck of hell

[Machine-Maker]

I feel like there should be a config option for this, this looks like it may have a chance of breaking maps designed for vanilla environments.

How exactly would it break? You can still run all the same commands, there's just more of them.

I think that the ultimate end goal is that we replace the SimpleCommandMap with something which actually basically backs the internal command dispatcher, the current command system setup is basically a hacked together clusterfuck of hell

Yeah absolutely. The whole CommandMap thing is really annoying.

[NoahvdAa]

How exactly would it break? You can still run all the same commands, there's just more of them.

Some servers have plugins like Essentials installed, which override commands such as /tp. /execute is probably the most used command for checking if certain conditions are met. I'd imagine that servers running command block/function based maps would break if they also had a plugin installed that overrode a command that was working in /execute before this change.

While trying to test for this, I tried to put the following command in a command block: /execute as NoahvdAa at @s run tp @s ~ ~1 ~. After applying this patch, the server would shut down whenever I clicked on the command block with this command in it. There isn't any error message, not even in debug mode, so I'm not entirely sure which part of the patch is causing this. The only plugin installed on the server was EssentialsX (2.19.1-dev+4-fde6524).

[Phoenix616]

How exactly would it break? You can still run all the same commands, there's just more of them.

Some servers have plugins like Essentials installed, which override commands such as /tp. /execute is probably the most used command for checking if certain conditions are met. I'd imagine that servers running command block/function based maps would break if they also had a plugin installed that overrode a command that was working in /execute before this change.

While trying to test for this, I tried to put the following command in a command block: /execute as NoahvdAa at @s run tp @s ~ ~1 ~. After applying this patch, the server would shut down whenever I clicked on the command block with this command in it. There isn't any error message, not even in debug mode, so I'm not entirely sure which part of the patch is causing this. The only plugin installed on the server was EssentialsX (2.19.1-dev+4-fde6524).

Tbh. this isn't really a main concern in my eyes, especially seeing the major benefit of having plugin commands working again.

Before the internal command changes plugin commands worked just fine in command blocks and similar issues arose hence the replace-commands config option in the spigot.yml and the command-block-overrides in the commands.yml (these would ideally need to be supported by this PR again) and the ability to use namespaces to manually fix it/properly account for that on building it.

[Machine-Maker]

This… might be a bigger concern when my PR that fixes plugin commands in datapack functions is merged. Because then, any server with a datapack that uses /tp for example and has essentials, will break.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Machine-Maker]

Rebased for 1.18.1

[Machine-Maker]

Rebased for 1.18.2

[Owen1212055]

https://github.com/PaperMC/Paper/pull/8235 Will resolve this underlying issue, do we still want to push for this individual fix?

[Owen1212055]

Closing in favor of https://github.com/PaperMC/Paper/pull/8235