Print warning if people try to call JavaPlugin#getCommand with paper plugins

[Leguan16]

This PR adds a simple warning if a commands section has been found in the paper-plugin.yml file.

I am taking suggestions for the warning message since I don't know if I should include how to register commands with Paper Plugins.
I also did not know which logger I should use since I don't think a plugin logger has already been created at the time where the warning is printed.

Which brings me to the next point:
Currently the warning is printed before the server is started:

Loading libraries, please wait...
2023-09-04 17:30:38,271 main WARN Advanced terminal features are not available in this environment
[17:30:38 WARN]: [Paper-Test-Plugin] Defining commands in paper-plugin.yml is not supported! <-------
[17:30:41 INFO]: Environment: authHost='https://authserver.mojang.com', accountsHost='https://api.mojang.com', sessionHost='https://sessionserver.mojang.com', servicesHost='https://api.minecraftservices.com', name='PROD'
[17:30:42 INFO]: Loaded 7 recipes
[17:30:42 INFO]: Starting minecraft server version 1.20.1

I don't think there is a way of printing the warning during the plugin load period (unless we add a boolean or something) since I don't think it is not possible to get the node after the PaperPluginMeta#create() method.

Closes #9669

[Warriorrrr]

You could probably link to the docs page (https://docs.papermc.io/paper/dev/getting-started/paper-plugins#commands) that explains what to do instead.

[Owen1212055]

Tbh, It's a bit worrying that people are using these plugins like this, to the point where we need to warn if they aren't even reading the documentation.

That format is still very much subject to change, and in all honesty, I would rather have some kind of warning if a paper plugin is being loaded... because those are very much still experimental and are waiting for future API.

And if a developer is actively handing out Paper plugins, /shrug what fun.

[Leguan16]

That is a good point. But I think the one thing has little to do with the other.
I think many people are creating a Paper plugin with the MinecraftDev IJ plugin and don't even realize that its different to Bukkit Plugins. So what you suggested should probably be handled over there by adding a little warning next to the paper manifest checkbox.

You can see it in the Discord that people are even too lazy to read JavaDocs so we cannot assume that they read the docs. The best thing we can do is give them a hint that they did something wrong on our side.

[A248]

And if a developer is actively handing out Paper plugins, /shrug what fun.

There are already plugins published on Modrinth using the paper-plugin.yml. I recall viewing this myself.

With all due respect, one should not publish an API to a popular project without expecting its use. The adoption of Paper plugins began as soon as a newfangled API for declaring plugin metadata was released. These shifts are not necessarily misguided, as the original impetus for Paper plugins outlined several use-cases which remain relevant: #7955

[lynxplay]

With all due respect, one should not publish an API to a popular project without expecting its use.

We do expect usage of the API, hence we published it. To collect feedback on it as we further improve it.
I don't think anyone minds people publishing paper plugins, IF they are aware of the contracts that come along with the usage of paper plugins, specifically changes to format, abi, api without backwards compatibility promises, as explicitly defined via the docs page and the experimental annotation.

Developers that publish such plugins need to be aware of those restrictions and risks and what those might mean for their users, hence we mostly expected this API to be used in private plugin development as updating to changes to paper plugins becomes a lot easier.

Tho given how common the question became regarding command definition in paper-plugin.yml, this is clearly not the case as it seems that a lot of people have started using paper plugins without the required insights into the unstable API and its limitations. Which is why I 100% agree with your Issue about actively warning to further raise the fact, that they are trying to use unstable API incorrectly.

If we have to go as far as printing a warning for paper plugins in general, I am unsure of.
Again, either developers are already aware of the risks they accept when using paper plugins and use it correctly (aware of breaking changes), not much need for a warning there, or people have ignored every warning thrown at them by us, at which point, another warning in their server logs isn't going to help much.

[4drian3d]

There are already plugins published on Modrinth using the paper-plugin.yml. I recall viewing this myself.

Personally, I have more than 10 public plugins made based on a paper-plugin.yml, I am fully aware that there will be changes in the structure of this type of plugins in the future and I have no problem to update my projects based on the new changes.
What I don't agree is to add a warning in console about a paper plugin being loaded, because there would be users asking about the warning and what to do to remove it and the only solution would be to lose exclusivity of all my plugins to be loaded only in Paper, which I want to avoid doing at all cost.

[Leguan16]

another warning in their server logs isn't going to help much.

In my opinion its generally wrong to say that "it wont help much". If we have the opportunity to make it more user-friendly we should use that opportunity I dont think a warning doesn't hurt anybody.

Another idea that had already been mentioned is that we could throw an exception instead of just printing a warning. That way 90% would actually read the message.

But I am generally against this approach since it would be kind of weird to ONLY throw it if a commands section has been found.

In my opinion a much better approach would be a InvalidConfigurationException which is thrown if any unwanted section has been found in the config, and not just for commands.
That's why i decided to only print a warning (could make that an error actually).

Maybe it would also be an idea to modify the exception message if someone tries to call getCommand and give further hints that it is not the correct way to define commands with Paper plugins.

[lynxplay]

With my comment about a warning I was only referring to a general warning comment for loading paper plugins, sorry if that wasn't clear.

I am 100% on board with a warning/exception for an invalid yaml format in the paper-plugin.yml.

[MiniDigger]

cant we have a general yaml format and throw if the format doesnt match? special casing this feels strange

[Leguan16]

special casing this feels strange

usually it isn't really a problem to have random tags in the config.
This is rather meant as a hint that defining commands has changed.

[Leguan16]

cant we have a general yaml format and throw if the format doesnt match?

A generic exception like "invalid section found in paper-plugin.yml" would just again bring the people to the discord asking what they should use instead.
The warning is supposed to minimize this.

[Leguan16]

What if, instead of checking the paper-plugin.yml for a commands section, we print a warning if JavaPlugin#getCommand() is called by a paper plugin?

@Nullable
public PluginCommand getCommand(@NotNull String name) {
    // Paper start - print warning if paper plugin
    if (getResource("paper-plugin.yml") != null) {
        this.logger.warning("---------------------------------WARNING--------------------------------");
        this.logger.warning("         Defining commands in paper-plugin.yml is unsupported!          ");
        this.logger.warning("      Please refer to the docs for proper command initialization!       ");
        this.logger.warning("https://docs.papermc.io/paper/dev/getting-started/paper-plugins#commands");
        this.logger.warning("                    Command will not be initialized!                    ");
        this.logger.warning("---------------------------------WARNING--------------------------------");
        return null;
    }
    // Paper end
    ...
}

[MiniDigger]

that seems the nicer way for that particular usecase
not sure tho if checking for the file is the best way, idk much about the infra behind paper plugins, but there gotta be a nice instanceof check we could do

[Leguan16]

but there gotta be a nice instanceof check we could do

i changed it to

if (!(pluginMeta instanceof PluginDescriptionFile)) {
    this.logger.warning("---------------------------------WARNING--------------------------------");
    this.logger.warning("         Defining commands in paper-plugin.yml is unsupported!          ");
    this.logger.warning("      Please refer to the docs for proper command initialization!       ");
    this.logger.warning("https://docs.papermc.io/paper/dev/getting-started/paper-plugins#commands");
    this.logger.warning("                    Command will not be initialized!                    ");
    this.logger.warning("---------------------------------WARNING--------------------------------");
    return null;
}

I am not sure if I should throw an exception instead of returning null.

[Leguan16]

Updated it to reflect discussed changes.
I also decided to throw a RuntimeException instead of returning null.

The only downside is that it doesn't respond to
getServer().getPluginCommand() yet.

[electronicboy]

I really dislike the notion of this; There are still some Qs over what we want to do long term here, ideally we wanna move away from the broken bukkit command system, however, the expectation of there still being an easy to use command system still exists, etc, etc; it's not 100% decided that such a commands block will never exist, I think that this issue would generally be better off solved with documentation and such

Not to mention, this would generally cause issues with plugins querying other plugins for commands, etc

[Leguan16]

Generally I agree with you but good luck getting new people to read the docs. As I have mentioned before this is solely for the purpose of hinting new devs that they are doing something wrong. This can always be removed in the future but to be honest, there are too much plugin tutorials and people will use those tutorials to develope Paper Plugins in the future. So getting people away from the old system will never really work until we remove the methods entirely. And until then I think its a good idea to print such a warning.

This is also not final but in my opinion its important and more user-friendly to add this (or in another form).

[electronicboy]

Except this ends up leaking warnings to other plugin invocations of this stuff, which imho makes this a no go on this solution

[Leguan16]

I think I am closing this in favor of minecraft-dev/MinecraftDev#2122 since it got merged recently. If you ignore this warning there is really nothing else we can do.

If someone still thinks this is something worth adding (in another form than how it works at the moment) just let me know :)