-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Allow configurable Role and Permission models #21
Conversation
I just noticed DraperStudio also submitted a PR (#20) concerning these features while I wrote this PR. Quite cool if you compare the similarities in approach and naming convention, 'cause the PR list was empty when I started working on it :-) Anyhow, an objective summary of both pull requests: This PR only exposes the Eloquent model classes to use for The other PR explicitly asks for table names and injects it dynamically into the supplied migration table. The provided contracts however do not enforce the additional behavior currently present in traits (e.g. revokeRole etc.). Docblocks aren't updated to use the contracts. |
I can see how it would be beneficial to have Contracts for the Traits and also split up the migrations since a User can have permissions without roles so I might add that later. Though, there is one issue I see with your migrations. If people want to use roles without permissions and they just run the [Illuminate\Database\QueryException]
SQLSTATE[HY000]: General error: 1215 Cannot add foreign key constraint (SQL: alter table `role_has_permissions` add constraint role_has_pe
rmissions_permission_id_foreign foreign key (`permission_id`) references `permissions` (`id`) on delete cascade) I guess you could solve this two ways.
|
Hi @DraperStudio! Thanks for joining the discussion. Just a quick comment: Table names: I felt that adding a config for them would be overkill since the described behavior in #5 is quite niche and the desired functionality is simply achieved using a regular Eloquent model specifying its table explicitly. Migrations: this is a matter of, let's say, preference. If I as a developer implement a custom Role model, I might also add some additional fields to the pivot table ( Anyway, this isn't a competition so I would advise the maintainer to review both PRs and take best of both worlds. |
Fellow artisans, thank you for your contributions. I'll soon review both your PR's and pick the changes I like. |
I've decided to go for @DraperStudio 's. But I still greatly appreciate the work you have put into this. |
See #18
I suggest to roll this in a major release since it was necessary to change migration publishing behavior.