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

Allow configurable Role and Permission models #21

Closed
wants to merge 2 commits into from
Closed

Allow configurable Role and Permission models #21

wants to merge 2 commits into from

Conversation

Propaganistas
Copy link

See #18

I suggest to roll this in a major release since it was necessary to change migration publishing behavior.

@Propaganistas
Copy link
Author

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 Role and Permission. It hereby deducts its table names from the model class as regular Eloquent models would do. If custom models are provided, custom migrations need to be created as well. Everything related to this package is extracted into a Contract, resulting in 4 Contracts in total.

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.

@ghost
Copy link

ghost commented Oct 27, 2015

  1. The table names are configurable to solve this issue Config file for setting table names #5
  2. The DocBlocks didn't needed to be updated because I named the contracts the same as the Models, so it was just a matter of replacing the use statements.
  3. The Provided contracts are explicitly for the Models that should be bound to the IoC. I did it that way because if a Model is compliant with a Contract which would enforce compliance with the Trait doesn't really matter just for the binding into the IoC.

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 create_roles_table migration Laravel should throw an exception because the table permissions won't exist when role_has_permissions gets created so the foreign key creation should fail for permission_id.


[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.

  1. Keep the role_has_permissions schema were it is and check if the permissions table exists and only then you run the migration.
  2. Move the role_has_permissions migration into the create_permissions_table migration.

@Propaganistas
Copy link
Author

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 (role_has_permissions). That's why I put it into the roles migration. It is therefore assumed that if a custom Role model and table is used, the corresponding relation tables are also set up manually.

Anyway, this isn't a competition so I would advise the maintainer to review both PRs and take best of both worlds.

@freekmurze
Copy link
Member

Fellow artisans,

thank you for your contributions. I'll soon review both your PR's and pick the changes I like.

@freekmurze
Copy link
Member

I've decided to go for @DraperStudio 's. But I still greatly appreciate the work you have put into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants