Deleted events are not fired when synchronizing roles

[FrancisMawn]

Describe the bug
Using $model->syncRoles($roles) will not trigger any "deleted" events from pivot models, only "created" ones.

Versions

• spatie/laravel-permission package version: 4.2
• illuminate/framework package: 8.61

PHP version: 7.4.3

Database version: MySQL 8

Expected behavior
When synchronizing roles, corresponding events should fire just like they would when using the sync() method on an eloquent relationship.

Additional context
The problem originate from the fact that when calling $model->syncRoles(), you are first calling $this->roles()->detach() which does a mass detach. Doing it this way will never trigger any eloquent model events for deletion.

For now, I've overridden the syncRoles() method on my model to:

public function syncRoles(...$roles)
{
    $this->roles()->detach($this->roles);

    return $this->assignRole($roles);
}

It works (somewhat) but I think this is not ideal and should be looked at. For example, when using events to audit changes made on models, this will trigger a "deleted" event for all current roles assigned to the model, event though these role could still be applied. Whereas using $model->roles()->sync($roles) would only trigger a "deleted" event for those that were actually removed.

It seems like sharing the logic across the assignRole() and syncRoles() methods might not be the best way to go in this case...?

Environment (please complete the following information, because it helps us investigate better):

• OS: Ubuntu
• Version 20.04

[erikn69]

That is not a bug, it's the laravel's expected behavior, mass detaches won't contain any id's, doing a mass detach is faster and requires less resources, not everyone needs that event, override is the best option on this cases

[FrancisMawn]

Thanks for your response.

I was curious so I've done some db logging while using sync without mass detach and it seems to use around the same amount of queries.

The performance gain seems pretty negligible while you lose the ability to properly use model events... but that's totally up to you to decide how you want to handle this.

[PaolaRuby]

Laravel will not change that behavior laravel/framework#38988 (comment)

[FrancisMawn]

@PaolaRuby I know, the issue you linked to was mine.

I think we can all agree that the mass detach will not trigger the deleted events and this is the intended behavior as far as Laravel is concerned.

The change I suggested was to eliminate the mass detach and instead use the sync() method on the roles relationship which in return does trigger the appropriate deleted/created events.

[PaolaRuby]

So, also

public function syncPermissions(...$permissions)
{
    $this->permissions()->detach($this->permissions);
    return $this->givePermissionTo($permissions);
}

[FrancisMawn]

That's correct. The problem is identical for the permissions.

Although it's still not ideal to do it this way since this will trigger deleted events for all the permissions associated with a role even though some might still be applied. That's why I said the code would need to be changed to use sync().

Here is what I ended up doing in my application (which is fine for my needs).

public function syncRoles(...$roles)
{
    // Override syncRoles() behavior to trigger deleted events.
    // This is needed to properly audit changes made to user roles.
    $roles = collect($roles)->flatten()->all();
    $this->roles()->sync($roles);
    $this->forgetCachedPermissions();

    return $this;
}

I wanted to limit the amount of changes so I decided to simply override the syncRoles method but this would have to tweaked to be retrofitted in the current code base as the roles or permissions might not only be ids but also model instances or names. I think that logic should be abstracted to another method that could be shared between syncRoles() and assignRole().