Skip to content

Added PermissibleBase getters and setters#8463

Closed
Badbird5907 wants to merge 5 commits into
PaperMC:masterfrom
Badbird5907:master
Closed

Added PermissibleBase getters and setters#8463
Badbird5907 wants to merge 5 commits into
PaperMC:masterfrom
Badbird5907:master

Conversation

@Badbird5907
Copy link
Copy Markdown
Contributor

@Badbird5907 Badbird5907 commented Oct 12, 2022

Adding this API will remove the need to use reflections in plugins to get and set PermissibleBase on CraftHumanEntity to override permissions logic

@Badbird5907 Badbird5907 requested a review from a team as a code owner October 12, 2022 15:36
@Owen1212055
Copy link
Copy Markdown
Member

Owen1212055 commented Oct 12, 2022

Note: #8108

Do note that this expands the whole permission overriding and allows you to override the permission logic in the SimplePluginManager.

@Badbird5907
Copy link
Copy Markdown
Contributor Author

Added the missing annotations causing tests to fail

Comment thread patches/api/0400-Added-PermissibleBase-getter-and-setter.patch Outdated
Comment thread patches/api/0400-Added-PermissibleBase-getter-and-setter.patch Outdated
@Badbird5907
Copy link
Copy Markdown
Contributor Author

Did the requested changes
idea didn't like the indents

@Badbird5907 Badbird5907 requested a review from kennytv October 13, 2022 17:42
Comment thread patches/server/0926-Added-PermissibleBase-getter-and-setter.patch Outdated
Comment thread patches/api/0400-Added-PermissibleBase-getter-and-setter.patch Outdated
@Badbird5907 Badbird5907 requested review from Lulu13022002 and removed request for kennytv October 13, 2022 18:31
@zml2008
Copy link
Copy Markdown
Member

zml2008 commented Oct 13, 2022

this feels like the kind of thing that exposes internal details of api impl which makes evolving the permissions api harder, and makes it easier for plugin developers to make mistakes without any traceability.

you'd be better off reworking how permissions are set and computed in the first place to remove the numerous footguns developers encounter when customizing permissions api logic - things like state tracking across the various join/quit/kick events, the permissions impact of recalculating permissions, and all the logic with default permissions

@electronicboy
Copy link
Copy Markdown
Member

The thing with evolving the perm API is that it's generally one of the many things which has been left to rot, hence why any perm plugin of note will literally just rip the thing out in order to add basic features, such as verbose mode, wildcards, etc

I'm much open towards expanding this API, but, I'd much rather that be an effort kicked off through perm plugin devs whom know their needs here so that we can actually deliver towards it, but, at this point, people reflect into this mess anyways that I don't have a strong urge to deny this personally, but, did wanna ask people like Luck if they had any thoughts or considerations; It's not like exposing this in the API is a huge priority for us and I'd much rather see perm plugin devs discuss this sorta thing

@Owen1212055
Copy link
Copy Markdown
Member

Closing, as in general as specified above it's best to instead expand this API instead of trying to move around it like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants