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
[4.x] Allow roles and groups to be database driven #5686
[4.x] Allow roles and groups to be database driven #5686
Conversation
Yep, absolutely a good idea. However it should be an option, not a requirement. |
That’s fair, should I put it behind a config flag? So if the roles/groups tables are falsey then it reverts to how it was before? |
Yeah I think that'll work. Just gotta be aware of how it works if the keys are missing from the 'tables' => [
'users' => 'users',
'role_user' => 'role_user',
'roles' => 'roles', // wont be there at all
'group_user' => 'group_user',
'groups' => 'groups', // wont be there at all
], |
I've pushed a couple of updates that make it opt-in based on the config value. Do you mind checking them over to make sure this approach works for you? I'll have a go at the tests tomorrow. |
This allows tests to generate migrations using the commands
This prevents composer autoload errors
Ok I've added some tests here to cover adding groups/roles to users and removing them again. I had to make some changes to the existing eloquent user test, and the auth migration. So you'll see I've added a new param to the auth migration ( Then I've made the eloquent user test (and groups/roles) run the migration as there was a todo mentioning that needed changed. I also modified the down() migration to support sqlite.
Can you take a look and see what you think of these changes? edit: I added |
If we use this PR, will the roles.yaml file gets updated when we change the role permissions? |
@abhishekvijayan-zessta no. They will be stored in the database. |
You need to specify tables for your roles/groups here: Lines 109 to 111 in 22ab699
|
@shawinigan can you share the full stack trace please? |
Here it is
|
@shawinigan I'm not seeing that on a clean install. We've already made it possible to query by string/handle - see: |
The issue seems to be with the getUserIds function in the UserGroup file. We also don't have the error on an empty DB but when we add our users and associate them to groups, we've got the issue |
@shawinigan are you definitely using the migrations provided by this PR - the group_id is a varchar, not a bigint. |
I changed it because I was having another issue when importing my groups through SCIM. If I change it back to varchar, I don't get the error but the Users column in the list is displaying all zeros instead of the right number. I do get the right number when I create a function id in the UserGroup.php file returning the id instead of the handle. |
Is it possible the issue is happening only with Postgres ? is_int return false in the find function. If I change it to is_numeric, it return true. |
@shawinigan I don't mind changing it to |
It's working greaat with the is_numeric instead of the is_int but the users count still stay at 0 on the group list. The id() function return an handle instead of an id. We can't seems to find what the problem is |
@shawinigan i get a count ok |
Are you using the ids in the group table or you are using uuids ? We didn't changed anything in the code. I twill probably be the reason we will have to forget using statamic unfortunately. The tables are ok, we don't understant why we can't get a count right and why you are getting it right |
@shawinigan I'm using this migrations this PR provides, so groups use bigint ids but group_user and role_user are linked by the handle not the id, as Statamic expects. If you are sure you havent changed anything, and have a Statamic license, you should email Statamic support and give them access to your site so they can investigate further. |
# Conflicts: # tests/Auth/Eloquent/EloquentUserTest.php
This PR follows on from the eloquent all models PR by allowing the user roles and groups to be stored in the database.
I appreciate that not every install would want this as they may be using Spatie permissions or some other roles library, but it feels like the option should be there at least for the roles and groups to be eloquent driven on a vanilla Statamic install.
If you'd be happy with the approach I've taken and open to merging it in principle then I'll start into writing the tests required to ensure coverage.
Closes #5986