Always use Permission objects instead of string
#6638
Conversation
dktapps
added
Category: API
There was a problem hiding this comment.
I'm on the way of @dktapps for the real value of this. As Permission doesn't guarantee that the object itself is registered, I don't necessarily see the need to manipulate objects. After that, if you manage to find a system that permit it, why not, but I don't see any solution for the moment.
| /** | ||
| * @method static Permission BROADCAST_ADMIN() | ||
| * @method static Permission BROADCAST_USER() | ||
| * @method static Permission COMMAND_BAN_IP() | ||
| * @method static Permission COMMAND_BAN_LIST() | ||
| * @method static Permission COMMAND_BAN_PLAYER() |
There was a problem hiding this comment.
This does not appear to be self-generating
I thought at 2 solutions:
|
|
The best solution would be the PermissionManager returning the Permission object and you need to create it via PermissionManager to use them. But idk how this could be applied in a clean way |
This wouldn't prevent people from doing |
| @@ -1007,8 +1007,6 @@ public function __construct( | |||
| ))); | |||
| $this->logger->info($this->language->translate(KnownTranslationFactory::pocketmine_server_license($this->getName()))); | |||
|
|
|||
| DefaultPermissions::registerCorePermissions(); | |||
|
|
|||
There was a problem hiding this comment.
Only just caught this, but this could be problematic if plugins register permissions that conflict with the built-in ones. RegistryTrait will only register the permissions when the first permission is accessed. Sure, technically SimpleCommandMap should trigger it but I don't feel happy about relying on that.
There was a problem hiding this comment.
Protecting the namespace pocketmine. for registering permission could be a solution. I don't really like it but I don't see another way to do it atm
There was a problem hiding this comment.
Not sure how that would work without pre-registering the permissions somehow. Maybe PermissionManager::make() should do it similar to how we setup states for RuntimeBlockStateRegistry.
| public function __construct(array $basePermissions){ | ||
| $this->permissibleBase = new PermissibleInternal($basePermissions); | ||
| public function __construct(){ | ||
| $this->permissibleBase = new PermissibleInternal(); |
There was a problem hiding this comment.
I'm not sure of this change. What's the utility of permissibleBase ?
There was a problem hiding this comment.
It's for getting rid of cyclic references. Not related to the PR topic.
Related issues & PRs
Fix #6536
Changes
API changes
Permissioninstead ofPermission|stringDefaultPermissionNames(useDefaultPermissions)DefaultPermissionsuseRegisteryTrait-> contains all permissions added by pocketmineBehavioural changes
Backwards compatibility
BC Break (see API changes)
Follow-up
Tests
Test Ingame with vanilla commands but without commands added by plugins