Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always use Permission objects instead of string #6536

Open
dktapps opened this issue Nov 25, 2024 · 0 comments
Open

Always use Permission objects instead of string #6536

dktapps opened this issue Nov 25, 2024 · 0 comments
Labels
BC break Breaks API compatibility Category: API Related to the plugin API Easy task Probably really easy to do, good task for first-time contributors Type: Enhancement Contributes features or other improvements to PocketMine-MP
Milestone

Comments

@dktapps
Copy link
Member

dktapps commented Nov 25, 2024

Problem description

There are various places where strings have to be looked up as permissions before using them:

$perm = $permManager->getPermission($name);

$perm = $permManager->getPermission($name);

$opRoot = $permManager->getPermission(DefaultPermissions::ROOT_OPERATOR);

if($permissionManager->getPermission($perm) === null){

This is problematic because it requires redundant checks for null when working with always-defined core permissions, and also allows passing in strings that can't be checked statically, allowing typos that only crash when the code is run.

Proposed solution

In all of these places, we should just use Permission objects directly. Using strings presents the possibility that the permission may not exist or may not have been registered. (To be fair, using Permission objects also doesn't guarantee registration. We should probably fix that too.)

Referencing permissions by their string IDs should only be done when loading stuff from configs, like in a permission management plugin. The IDs are intended for user use, not code use. Ergo, PermissionManager should only be interacted with to look up a permission by its ID.

DefaultPermissions should turn into a RegistryTrait user (or perhaps an enum with metadata) to remove the need for DefaultPermissionNames (which should never have been needed to begin with).

Alternative solutions that don't require API changes

@dktapps dktapps added BC break Breaks API compatibility Category: API Related to the plugin API Type: Enhancement Contributes features or other improvements to PocketMine-MP labels Nov 25, 2024
@dktapps dktapps added this to the 6.0 milestone Nov 25, 2024
@dktapps dktapps added the Easy task Probably really easy to do, good task for first-time contributors label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC break Breaks API compatibility Category: API Related to the plugin API Easy task Probably really easy to do, good task for first-time contributors Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Development

No branches or pull requests

1 participant