PaperMC / Paper

The most widely used, high performance Minecraft server that aims to fix gameplay and mechanics inconsistencies
https://papermc.io/
Other
9.98k stars 2.32k forks source link

Print helpful warning if 'commands' section is found in paper-plugin.yml #9669

Closed A248 closed 6 months ago

A248 commented 1 year ago

Is your feature request related to a problem?

Some developers expect to use the commands section in the paper-plugin.yml as they do in the plugin.yml. This is unsupported, which causes calls such as getCommand("myCommand").setExecutor(someExecutor) to fail with a NPE.

A NPE does not immediately explain the cause of the bug, namely that the developer expects to use the commands section but the commands section is no longer a feature. So the developer revisits the paper-plugin.yml, checks for typos, checks the code again, recompiles and reruns, and it still doesn't work. Then they maybe read the documentation on Paper plugins one more time, or join the PaperMC discord (as has happened recently) to debug.

Describe the solution you'd like.

If the commands section is found, print a warning message informing developers it is not available using paper-plugin.yml and programmatic registration is preferred.

It may be worth even throwing an exception and refusing to load the plugin. I can see no other reason to use the commands section if paper-plugin.yml is not in use.

Describe alternatives you've considered.

Do nothing, and continue to sustain inquiries and support requests on this.

The documentation is already sufficiently prominent, so I see nothing to change there.

Other

No response

Leguan16 commented 1 year ago

I don't think it should refuse to load the plugin. Either it throws an exception or a warning.

For me it makes more sense to show a warning than an exception because there is not really a "downside" of the command section existing in paper-plugin.yml.

MiniDigger commented 1 year ago

I would argue in favor of an error, since the file doesn't fit the schema and its a clear developer error

Leguan16 commented 1 year ago

but it doesn't break the plugin nor does it have it any notable performance downsides.

456dev commented 1 year ago

i would say warning in dev env, in case for backwards compatible schema changes (like when folia was added)

Leguan16 commented 1 year ago

Ok so I have looked into this for a while and it seems that its not really a possibility to just check-and-print if there is a command section in the paper-plugin.yml. It seems that a methods get a BufferedReader which is then converted to a PaperPluginMeta instance in a builder. Then its already too late to check if there is a command section because it just doesn't parse it.

public static PaperPluginMeta create(BufferedReader reader) throws ConfigurateException {
    YamlConfigurationLoader loader = YamlConfigurationLoader.builder()
        .indent(2)
        .nodeStyle(NodeStyle.BLOCK)
        .headerMode(HeaderMode.NONE)
        .source(() -> reader)
        .defaultOptions((options) -> {

            return options.serializers((serializers) -> {
                serializers
                    .register(new EnumValueSerializer())
                    .register(PermissionConfiguration.class, PermissionConfigurationSerializer.SERIALIZER)
                    .register(new ComponentSerializer())
                    .registerAnnotatedObjects(
                        ObjectMapper.factoryBuilder()
                            .addConstraint(Constraint.class, new Constraint.Factory())
                            .addConstraint(PluginConfigConstraints.PluginName.class, String.class, new PluginConfigConstraints.PluginName.Factory())
                            .addConstraint(PluginConfigConstraints.PluginVersion.class, String.class, new PluginConfigConstraints.PluginVersion.Factory())
                            .addConstraint(PluginConfigConstraints.PluginNameSpace.class, String.class, new PluginConfigConstraints.PluginNameSpace.Factory())
                            .addNodeResolver(new FlattenedResolver.Factory())
                            .build()
                    );

            });
        })
        .build();
    CommentedConfigurationNode node = loader.load();
}

Debuging node at this point prints: image

456dev commented 1 year ago

Does node.isVirtual() work? Or similar

Leguan16 commented 1 year ago

Ok so I did a mistake. I only defined a commands tag with no actual commands in it. I tested it with a command defined and now it works with virtual()!

The warning is printed very early (Before the Environment log).

System Info: Java 17 (OpenJDK 64-Bit Server VM 17.0.8+7-Debian-1deb12u1) Host: Linux 5.15.90.1-microsoft-standard-WSL2 (amd64)
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