Vault rework

[WalshyDev]

Hey,

We all know how Vault has aged over time, I mean hell it still only accepts player's name and OfflinePlayer. Would any of the project members object to a PR being done re-working it all?

This would be a breaking change (obviously) and should be branded as such in the version numbers too (2.0). I think it'd be best to rip out all of the player name and OfflinePlayer stuff and just replace them with UUID methods. All up-to-date libs implementing Vault use UUIDs anyway as far as I know so adapting to this new version would not take long at all.

If people want older libs they can just simply use an older version of Vault but at this point everything should use UUIDs no matter what.

So, would any of you members object to a PR being opened doing this? VaultAPI and Vault would be updated and then plugins such as LuckPerms, Essentials etc can all update from there.

Out with the old, in with the new.

[Sleaker]

One of the problems with this is getting implementations to update. It's not just a matter of getting downstream plugin authors to update for the new API, I'm less concerned about that, because it's a matter of pushing a new version and then they have to conform. The issue is that upstream providers for econ/perms have to also implement it. If there's enough traction to get the major providers (like essentials) to write a new provider with an updated API then it wouldn't be a bad idea, but if they aren't willing to do that, then redoing the API wont really do anything, Vault doesn't supply updated Providers anymore to try and limit it's maintenance surface.

This is also the same problem as other requests to support BigDecimal precision.. etc.

[WalshyDev]

Understandable, however, it should very much benefit them too and assumingly wouldn't take too long. As I said initially they all store as UUIDs. Just looking at LuckPerms' code they have a bit of a wrapper in order to have UUIDs in their proper implemenation.

I could look into opening a PR to some of the big guys like Essentials and LuckPerms after the implemenation is done here.

[Sleaker]

I don't know if issue generation is the proper way to do it. It's more of getting them involved in a discussion about 'would you support this'. Issue creation is primarily for like 'hey please do this'

[MeFisto94]

I was going to open an issue around the same topic, but with a slightly different approach: The only thing that bugged me as a "custom backend writer" is that the methods declared abstract are exactly those which only have Strings for playerNames.

This means I have to override every potential method resulting in a lot of boilerplate code.

My proposal would be to declare the variants accepting OfflinePlayers and Worlds abstract and either re-routing the string accepting abstract methods (Bukkit.getOfflinePlayer) or to delete them. This would only propose a small impact upstream but gets rid of all the manual work required when you don't want to be resolving player names to uuids all the time (every fancy method goes through the deprecated but abstract has(String, String))

[Sleaker]

I was going to open an issue around the same topic, but with a slightly different approach: The only thing that bugged me as a "custom backend writer" is that the methods declared abstract are exactly those which only have Strings for playerNames.

This means I have to override every potential method resulting in a lot of boilerplate code.

That's the point of an API. It's forcing you to follow the consistent conventions to keep your plugin compatible with all ways the API has been used in the past for maximum compatibility.

My proposal would be to declare the variants accepting OfflinePlayers and Worlds abstract and either re-routing the string accepting abstract methods (Bukkit.getOfflinePlayer) or to delete them. This would only propose a small impact upstream but gets rid of all the manual work required when you don't want to be resolving player names to uuids all the time (every fancy method goes through the deprecated but abstract has(String, String))

There's still a lot of deprecated methods in the API due to legacy permission/economy systems, if you don't want to have those methods work you can just stub them out since they're deprecated in the API. That will make it not work with any downstream plugins that still use the deprecated APIs, but if you don't want to support them because it's too much work or incompatible with how you want to handle things.. then don't support it.

The AbstractEconomy is an internal reroute for the built-in Vault legacy implementations which didn't support offline player, and was provided for others to potentially use for the same situations. There's little point in using it as a new implementation. A rework of the API would have no Abstract anything since all of the implementation would go back to being implemented at your level.. so I think there's a misunderstanding on your part as to what the purpose of the Abstract classes are used for.

[blablubbabc]

This seems to relate to https://github.com/MilkBowl/VaultAPI/issues/37

I am currently in a similar situation where I am trying to decide between using OfflinePlayer and UUID for representing players in a plugin's API. Bukkit itself uses OfflinePlayer whenever it needs to refer to a player (even if that player reference is actually stored as UUID in memory and on disk).

I am wondering, what's the issue with using OfflinePlayer inside the API? Plugins can easily retrieve an OfflinePlayer via Bukkit#getOfflinePlayer(UUID). The overhead of this should be minimal.

[Sleaker]

@blablubbabc this is why Vault was initially updated to allow OfflinePlayer objects.