search

PyvesB / advanced-achievements

:fireworks: Popular plugin that adds unique and challenging achievements to Minecraft servers.
https://www.spigotmc.org/resources/83466
GNU General Public License v3.0
200 stars 214 forks source link

Add SunLight Essential to soft dependence #1012

Closed 4o3F closed 3 years ago

4o3F commented 3 years ago

Add SunLight Essential to soft depend to fix the load order problem

PyvesB commented 3 years ago

Hello @ExusiaiMoe ! 👋🏻

Could you please clarify in what way Advanced Achievements depends on SunLight?

4o3F commented 3 years ago

Hello @ExusiaiMoe ! 👋🏻

Could you please clarify in what way Advanced Achievements depends on SunLight?

SunLight is an alternative for essential, it has economy built in, so Advanced Achievement need it to load before to init economy functions

PyvesB commented 3 years ago

I'm not sure I understand. Advanced Achievements only integrates with economy plugins supported via the Vault API and has a soft depency on Vault as one would expect. Vault is an abstraction layer that should avoid these kind of problems and that ensures that other plugins don't need to care about the underlying details of the economy plugin actually being used. Adding a soft depency on Sunlight should not be needed if it integrates with Vault.

Do you have a link to specific SunLight bug reports that describe problems with Advanced Achievements in more details?

4o3F commented 3 years ago

Advanced Achievement will throw this error if SunLight isn't loaded before https://pastebin.com/G30C2syn

PyvesB commented 3 years ago

The stack trace suggests that the bug is in SunLight itself. Throwing obscure NullPointerExceptions rather than loading on the fly or providing a sensible default value is not sane behaviour. This should be fixed in Sunlight.

On the more general topic of plugin loading, there is one and one only correct load order: SungLight, then Vault and finally Advanced Achievements. Advanced Achievements' existing code guarantees the end of the ordering chain: Advanced Achievements needs Vault, and will always load after it by virtue of how its softdependence setup. Your pull request only mandates that Advanced Achievements is loaded after SunLight, but does not guarantee that Vault is loaded after SunLight. This means that any other plugin that is not Advanced Achievements and that needs the Vault API when it is enabled will bump into the exact same problem.

Imagine if every economy, chat or permissions plugin loaded after Vault and threw NullPointerExceptions as SunLight does: every single Bukkit/Spigot plugin that needs to call the Vault API when enabled would need to depend on every single one of these economy/chat/permissions plugins and would need to be updated whenever a new economy/chat/permissions is released. Vault is an abstraction layer that precisely allows plugins not to worry about what the underlying economy/chat/permissions plugin actually is. This pull request defeats the spirit and purpose of Vault.

The correct way to enforce the right load ordering is for you to add loadbefore: [Vault] to SunLight. Many other popular Vault implementations for economy, chat or permissions do this this way, for example Gringotts or LuckPerms.

If ever you absolutely need SunLight to call the Vault API once Vault has loaded, you can simply add a listener for PluginEnableEvent.

Hopefully the above makes sense and gives you a few pointers to things to improve in SunLight. No further action should be taken in Advanced Achievements at this point, so I'll go ahead and close this pull request. 😉

4o3F commented 3 years ago

Thanks for the info, seems I'm still new at this, I'll contact SunLight Developer :-) Thank you for your time

PyvesB commented 3 years ago

No worries, thank you for raising this anyway and having a useful discussion!