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

add column id of pivot record in relations #619

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gpibarra
Copy link

@gpibarra gpibarra commented Feb 9, 2023

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
Copy link
Owner

JosephSilber commented Feb 10, 2023

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
Copy link
Author

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
Copy link
Author

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

@JosephSilber
Copy link
Owner

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

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

Successfully merging this pull request may close these issues.

None yet

2 participants