ScreamingSandals / ScreamingLib

Cross-platform library for Minecraft development.
Apache License 2.0
29 stars 7 forks source link

getPaperConfig() might throw NotSupportedYetException #20

Closed boiscljo closed 2 years ago

boiscljo commented 2 years ago

https://github.com/ScreamingSandals/ScreamingLib/blob/b0574244b670643f8955ca658b510d54a3ea8359/core/bukkit/src/main/java/org/screamingsandals/lib/bukkit/BukkitServer.java#L146

This line Throws on server like Mohist, it's probably better to just catch and return no proxy. Unsupported server would at least get the no proxy value and be able to function at their own risks. Same if Paper decided to remove it in a later version.

zlataovce commented 2 years ago

most plugins won't work properly on mohist anyways, it's very bad. I'm against sanitizing any bukkit method calls for usage with spigot-forge hybrids.

boiscljo commented 2 years ago

It might not be just mohist that implement the method and throw, we already check with reflection if it exists, having a try and catch would fix those problems.

Misat11 commented 2 years ago

NotSupportedYetException is not something regular Spigot or Paper would throw. What version of Mohist did you use btw?

boiscljo commented 2 years ago

I've just seen it in the log of Ticket 971 while trying to help. https://discord.com/channels/582271436845219842/957703001554968696

Technically the NPC might work, but it's the check we added for bungee cord that doesn't work on it. I mean it might throw for another reason than NotSupportedYetException. A detection of the proxy should not throw error and should just return Unknown or None if an error occurred.

boiscljo commented 2 years ago

My intention isn't to make it work on Mohist(1.16.5), it's to make sure we're not causing a problem with any of the legitimate Paper or spigot fork that might return null or throw an exception in the .getConfig or .getSpigotConfig, etc... The check should never throw anything and just fallback to NONE if there was an error in the config (Can be caused by a deleted file or an eventual sandbox that prevent reading the config for privacy reason(possible legitimate fork of spigot) )

zlataovce commented 2 years ago

if the config key is missing, it will fallback to NONE. the Player.Spigot#getPaperConfig method is in the normal Paper API (not an implementation detail), so if a fork decides to limit access to it, that's not a problem of SLib.

zlataovce commented 2 years ago

we'd have to try-catch every Bukkit API call, if we truly wanted to sanitize wrong implementations

boiscljo commented 2 years ago

Actually not, only catch them on functions that are unavoidable. Like static constructor. The plugin dev(ex: SBA) can go around any other function but cannot go around the static constructors. Isn't it a good standard things in java that Static constructor or constructors should never throw no matter what.

iamceph commented 2 years ago

If in future these changes will cause problems, we will fix them.