Open konovalov-nk opened 7 years ago
@konovalov-nk sure
@kodeine ok, thanks. I've also fixed some issues as well.
Permission slugs.
Roles and permissions relations from actual model instances.
When I've tried to use this package on existing database models like that:
$user_admin = \App\User::first();
$user_admin->assignRole(ROLE_ADMIN);
// $user_admin->getRoles is empty!
$this->assertEquals($user_admin->getRoles(), [
1 => ROLE_ADMIN
]);
// $user_admin->getPermissions is empty!
$this->assertEquals($user_admin->getPermissions(), $admin_role->getPermissions());
It wouldn't work for me. However, new created user works:
$user_admin = new User();
$user_admin->firstname = 'Role';
$user_admin->lastname = 'test';
$user_admin->newsletter = 0;
$user_admin->email = 'role@test.com';
$user_admin->password = 'RoleTest';
$user_admin->save();
$user_admin->assignRole(ROLE_ADMIN);
// Test passes.
$this->assertEquals($user_admin->getRoles(), [
1 => ROLE_ADMIN
]);
// Test passes.
$this->assertEquals($user_admin->getPermissions(), $admin_role->getPermissions());
I had to modify core code to fix that behavior (see this commit). Wonder why no one encountered this on Laravel 5.3
@konovalov-nk
sounds good, please post the PR, also for slug size to 1024 characters
we can mention in the wiki as well.
@kodeine
There is something I'm not sure about, though. I have modified assignRole()
and hasRole()
methods, but haven't touched the can()
method and the code is actually breaking DRY principle because I didn't have enough time to implement it in the right way, so I've added able()
method just to make sure I'm not breaking anything. Some features like syncRoles are not working at all with models/ids because I only needed assignRole()
, hasRole()
and able()
.
So, I guess it would be better if I spend some time to merge able()
method into can()
and write additional unit tests to cover new feature. I think I need a week or two to get this done.
Should I make pull request now or it's better to fix everything and send proper pull request later? ;) I'll make pull requests for slug size and getRoles(), getPermissions() now, though.
@konovalov-nk please make a proper PR when you have time :) no rush. You can submit the PR's which you think are good enough.
@kodeine Sure, thank you for help!
@kodeine I have one more question. This project uses semantic versioning. I've made breaking changes to API. For example:
From:
public function can($permission, $operator = null, $mergePermissions = [])
To:
public function can($permission, $model = '', $reference_id = 0, $operator = null, $mergePermissions = [])
Possible outcomes:
2.0
?able()
for backward-compatibility?master
is still version 0.x
and 1.0@dev
is not completed yet?able()
version (for backward-compatibility):
public function can($permission, $operator = null, $mergePermissions = [])
{
$model = '';
$reference_id = 0;
return $this->able($permission, $model, $reference_id, $operator, $mergePermissions);
}
@kodeine I've decided to make 3rd outcome since there are a lot of things changed. See pull request.
Basically, I've already implemented the feature, see this fork.
Here how it works:
I'm using it like this:
This allows me to assign roles to user per entities. For example, users could give other users permissions to edit their posts, etc. Much like groups in a facebook.
Should I clean up my code a little bit and make a pull request?