JosephSilber / bouncer

Laravel Eloquent roles and abilities.
MIT License
3.43k stars 330 forks source link

add column id of pivot record in relations #619

Open gpibarra opened 1 year ago

gpibarra commented 1 year ago

I am working with this package and I wanted to have audit processes, I need to know the id of the records created in the assigned_roles and permissions pivot tables. Particularly I am using the package https://github.com/spatie/laravel-activitylog, but it is not important since the pivot relationships are not with models which cannot be listened for events.

That's why I was trying to perform manual audit trails when attaching and detaching users to roles for example. For that I need the ids of the mentioned tables. These tables do not have assigned models and in case you want to retrieve this information you should make a query using the "DB" facade (this is how these records are registered, for example in Conductors\AssignsRoles in $this->newPivotTableQuery()->insert().

Another alternative to using the DB is to use the relationships of the Role and Ability models, but in these morphToMany information about the id of the pivot table does not come. We can see this quickly with tinker:

$role = \Silber\Bouncer\Database\Role::query()->whereHas('users')->with(['users'])->first();
$role->users->first()->pivot;

The change adds the id columns in the pivot relationships of the Role and Ability models as well as the traits to apply to the User model.

JosephSilber commented 1 year ago

Hmmm. I'm afraid this is a breaking change. The id wasn't always there in the migration, and was added as a non-breaking change. But querying for it means that users who had started using Bouncer, and run the initial migration before we added the ids to the pivot table, will now get runtime errors in production.

The only reason I'm maybe still considering it is that version 1.0 had the id in the migration.

On the other hand, we never told users that they must add those columns, so people that had been running Bouncer prior to 1.0 are now using 1.0 as intended, but will have it break in production. That's kinda unacceptable.

🤔

gpibarra commented 1 year ago

Thanks for the reply. I close this PR, if at some point there is a version 2.0 then you could evaluate whether to add it or not.

gpibarra commented 5 months ago

@JosephSilber With the update to Laravel11, would it be a good time for this?

JosephSilber commented 5 months ago

The Laravel 11 update is not a breaking change. See the release notes: https://github.com/JosephSilber/bouncer/releases/tag/v1.0.2