[5.3] Fire BelongsToMany relation events for attach/detach/sync/toggle #14988

jarektkaczyk · 2016-08-24T02:26:53Z

BelongsToMany relations is the only missing part in the events system in eloquent.

It has been requested for a long time and has many applications, eg.
#10435 #10545 #10770 #4022 #2303 #1703 #8452

This PR is BC however it also brings change in the public interface, that is all the methods (attach/detach/sync/toggle) get additional false return value in case *ing event halts the execution. As is it doesn't break BC however there might be edge case:

1. one relies on array return value from sync or toggle
2. then adds syncing/toggling event handler that can halt execution in some circumstances
3. the return value under those circumstances is no longer array but false

It introduces 8 new events:

eloquent.attaching.[RELATION]: Parent\Model
eloquent.attached.[RELATION]: Parent\Model
eloquent.detaching._RELATION_: Parent\Model
eloquent.detached._RELATION_: Parent\Model
eloquent.syncing._RELATION_: Parent\Model
eloquent.synced._RELATION_: Parent\Model
eloquent.toggling._RELATION_: Parent\Model
eloquent.toggled._RELATION_: Parent\Model

and appropriate methods on the Model to register these events (due to different nature, they have additional parameter - relation name), eg:

// Model::attaching($relation, function ($parent, Support\Collection $pivotData) { ... })
User::attaching('roles', function ($user, $pivotData) {
    // payload contains parent of the relation AND pivot data being inserted to the table
    $pivotData == Collection([
        ['user_id' => $user->id, 'role_id' => ATTACHED_ID, 'some_pivot_field' => 'some pivot value'],
        ...
    ]);
})

// manually
$dispatcher->listen('eloquent.attaching.roles: App\User', function ($user, $pivotData) { ... })

Since the $pivotData payload may vary depending on the event, here's complete list of its content:

attaching - like in the example above - Collection with raw data to be inserted to the pivot table
attached - same as above
detaching - Collection of ids being detached OR empty Collection
detaching - Collection of detached ids OR empty Collection
syncing - Collection of id => [ ..pivot data.. ] entries to be inserted/updated
synced - Collection of changes made (the same as return value of sync only wrapped in Collection)
toggling - Collection of id => [ ..pivot data.. ] entries to be inserted/updated
toggled - Collection of changes made (the same as return value of sync only wrapped in Collection)

and an example of all events being fired when syncing:

>>> $user->roles()->sync([1, 2 => ['extra' => 'provided pivot value']])

user 1 syncing roles: {"1":[],"2":{"extra":"provided pivot value"}}
user 1 detaching roles: ["6"]
user 1 detached roles: ["6"]
user 1 attaching roles: [{"user_id":1,"role_id":1}]
user 1 attached roles: [{"user_id":1,"role_id":1,}]
user 1 attaching roles: [{"user_id":1,"role_id":2,"extra":"provided pivot value"}]
user 1 attached roles: [{"user_id":1,"role_id":2,"extra":"provided pivot value"}]
user 1 synced roles: {"attached":[1,2],"detached":[6],"updated":[]}

All of the *ing events are, just like existing ones, capable of halting the action if your listener returns false:

// Bill can't be an admin:
User::attaching('roles', function ($user, $pivotData) use ($adminRole) {
   if ($user->name == 'Bill'
      && $pivotData->contains(function ($record) use ($adminRole) {
           return $record['role_id'] == $adminRole->id;
      })
   ) {
       return false;
   }
})

You can also adjust pivot data in the callback, just like you'd do with exiting events:

// We need UUID for the pivot records (well... yeah):
User::attaching('roles', function ($user, $pivotData) {
   foreach ($pivotData as $key => $record) {
      $record['uuid'] = data_get($record, 'uuid', Uuid::generate()); // generate if not provided already
      $pivodData->put($key, $record);
   } 
})


>>> $user->roles()->attach(5)
>>> $user->roles()->find(5)
Role {
  'name' => 'editor',
  ...
  'pivot' => Pivot {
    'user_id' => 1,
    'role_id' => 5,
    'uuid' => 'a-nice-auto-generated-uuid'
  }
}

Notes on the commits:

the code in BelongsToMany has been a bit refactored in order to use Collection where events require that
new tests added + some brittle tests refactored

…toggle) + refactored brittle tests

styleCI

axyr · 2016-08-27T08:14:18Z

I really like this.

Should $model->belongsToRelation()->associate($otherModel)

not also have an event?

jarektkaczyk · 2016-08-27T10:17:47Z

1 this is only for belongsToMany, that is operations on pivot table, which
can't be tracked in any way in eloquent directly, unlike belongsTo
2 associate doesn't run the query, so it's not real association until you
call save

Jarek Tkaczyk

taylorotwell · 2016-09-02T14:03:13Z

tests/Database/DatabaseEloquentBelongsToManyTest.php


-        // we expect to call $relation->wherePivot()
-        $relation->getQuery()->shouldReceive('where')->once()->andReturn($relation);
+        // This is our test! The wherePivot() params also need to be called


What does "This is our test!" mean?

We should ask @GrahamCampbell :)
I guess it refers to the wherePivot line below. There are bunch of tests in this class that are mocking CUT and require refactoring, but I didn't want to do that, so PR doesn't get even bigger and harder to review.

jarektkaczyk · 2016-09-08T03:13:09Z

ping @taylorotwell

GrahamCampbell · 2016-09-08T08:01:02Z

ping @taylorotwell

Pending you removing "This is our test!" @jarektkaczyk.

jarektkaczyk · 2016-09-08T08:27:27Z

WAT?

That's an old test, out of scope of this PR, and btw your test Graham (or at least git blames you) ;)

GrahamCampbell · 2016-09-08T08:29:25Z

jarektkaczyk · 2016-09-08T08:56:51Z

well, as irrelevant as it is, here's the time travel:

framework/tests/Database/DatabaseEloquentBelongsToManyTest.php

Line 373 in d70921d

// This is our test! The wherePivot() params also need to be called

so git blamed your tabs to spaces as latest.

Now, back to the feature being proposed.

GrahamCampbell · 2016-09-08T10:02:47Z

I'd appreciate you not taking the piss. Please just remove it. I don't care who added it.

taylorotwell · 2016-09-09T15:21:06Z

I really don't think this is ever a good solution to a problem to be honest. If you want something to happen before or after you attach / detach / toggle / whatever, you should make a repository class that actually does those things before calling ->attach. That saves us having to maintain all of this complicated event code throughout Eloquent.

andrewkamm · 2016-12-16T23:04:23Z

@jarektkaczyk Did you find a clean way to implement this since this PR was rejected? I agree that this would be really useful to have in Laravel core.

jarektkaczyk · 2016-12-17T02:47:39Z

I didn't work on it anymore since then. Jarek Tkaczyk softonsofa :: development with pleasure

…

On Dec 17, 2016 07:04, "Andrew Kamm" ***@***.***> wrote: @jarektkaczyk <https://github.com/jarektkaczyk> Did you find a clean way to implement this since this PR was rejected? I agree that this would be really useful to have in Laravel core. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14988 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGm5sl25ZBTbmPJUUeXmchY4wI05gKSNks5rIxkCgaJpZM4Jrk2W> .

lukepolo · 2016-12-17T02:49:26Z

Just manually fire the events , is what I did On Fri, Dec 16, 2016 at 9:47 PM Jarek Tkaczyk <[email protected]> wrote:

…

I didn't work on it anymore since then. Jarek Tkaczyk softonsofa :: development with pleasure On Dec 17, 2016 07:04, "Andrew Kamm" ***@***.***> wrote: > @jarektkaczyk <https://github.com/jarektkaczyk> Did you find a clean way > to implement this since this PR was rejected? I agree that this would be > really useful to have in Laravel core. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#14988 (comment) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AGm5sl25ZBTbmPJUUeXmchY4wI05gKSNks5rIxkCgaJpZM4Jrk2W > > . > — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#14988 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB-I7CgF9brA4hZLWHHPk7s5ciA5X97Xks5rI01XgaJpZM4Jrk2W> .

andy-berry-dev · 2016-12-18T11:53:04Z

We needed attach/detach events at Agency Core for sending things like User notifications when new tasks were assigned.

I've created a Gist with the extensions we added to make it work - https://gist.github.com/andyberry88/be3c45380568fc359cb61e00c4249704. It's basically the changes suggested by @jarektkaczyk above but taken out into app classes. I've had to extract it from other extensions we've added so hopefully it works. It should at least give an idea of the changes required.

andrewkamm · 2016-12-19T15:26:32Z

@andyberry88 Thanks!

lukepolo · 2017-01-04T01:32:40Z

@jarektkaczyk maybe we could work together to make it a package ? Not sure if theres a ton of demand for it but it would save me time in some other applications.

jarektkaczyk · 2017-01-04T02:11:28Z

@lukepolo I can help you out with the package, just drop me an email with what assistance you need 🏄‍♂️

lephleg · 2017-05-11T13:53:58Z

I can also assist any attempt to make this formal.
I am really having a hard time to find a proper workaround for this, as I need to track relationship changes for logging purposes in L4.

lukepolo · 2017-05-11T13:58:38Z

I sadly do not have the time todo this as previously thought, you can see if @jarektkaczyk still wants to help out

neylsongularte · 2017-05-11T14:43:00Z

Experimental package:

https://github.com/neylsongularte/eloquent-extra-events

fico7489 · 2017-12-06T09:41:49Z

I created a well documented and well-tested package for this problem https://github.com/fico7489/laravel-pivot.
You can do much more with my package that with this one neylsongularte/eloquent-extra-events.
You can get parent model, changed pivot ids, and relation name on which is updating performed.

jehoshua02 · 2018-12-18T23:54:43Z

@taylorotwell If we create a repository class, is there a way to get Nova to utilize it?

We are doing some caching and want to invalidate/recalculate the cache when many-to-many relationships change.

Do you have any recommendations for how we can do this? For now I will see if we can use laravel-pivot.

vpillinger · 2019-04-18T20:15:43Z

@taylorotwell Sorry to necro this comment on you; however, perhaps there is a better solution here than creating new events or leaving the current functionality as-is?

If you want something to happen before or after you attach / detach / toggle / whatever, you should make a repository class that actually does those things before calling ->attach.

While I generally agree with this statement regarding business logic like sending notifications, there are good use-cases for wanting events when updating pivots. For example, a common use-case is to implement auditing and having some sort of events fire allows one to setup auditing events in a general way. For example, in an auditing package.

My understanding is that sync() does not trigger the normal eloquent events even if we create a custom pivot model. Therefore, currently one can audit on a belongsTo relationship by listening to the model being updated, but there is not a way to listen to a belongsToMany relationship as the information is contained in an unrelated model that isn't triggering events.

So maybe the appropriate solution is to just make sync() and related methods trigger these default events if a custom Pivot model exists?

Currently one can just ignore these relationship functions and update the pivots manually with a save() statement to get eloquent events. So these convenient functions just become a newbie trap instead of something that one should really use.

fire eloquent events on BelongsToMany operations (sync/attach/detach/…

cdd6f21

…toggle) + refactored brittle tests

jarektkaczyk changed the title ~~5.3 Fire BelongsToMany relation events for attach/detach/sync/toggle~~ [5.3] Fire BelongsToMany relation events for attach/detach/sync/toggle Aug 24, 2016

Added model registrars for BelongsToMany events

8530bcb

styleCI

jarektkaczyk force-pushed the 5.3-fire-BelongsToMany-relation-events branch from 7fa262e to 8530bcb Compare August 24, 2016 02:33

taylorotwell reviewed Sep 2, 2016
View reviewed changes

taylorotwell closed this Sep 9, 2016

neylsongularte mentioned this pull request May 11, 2017

Not logging Affected Relations? owen-it/laravel-auditing#58

Closed

jarektkaczyk deleted the 5.3-fire-BelongsToMany-relation-events branch July 12, 2018 08:24

staudenmeir mentioned this pull request Dec 17, 2018

Custom Intermediate Table Models #26864

Closed

PrafullaKumarSahu mentioned this pull request Apr 15, 2019

Laravel 5.6 pivot table auditing using laravel auditing 8.0 owen-it/laravel-auditing#507

Closed

genesiscz mentioned this pull request Apr 14, 2020

Control over created models/attached fields laravel/nova-issues#2506

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5.3] Fire BelongsToMany relation events for attach/detach/sync/toggle #14988

[5.3] Fire BelongsToMany relation events for attach/detach/sync/toggle #14988

jarektkaczyk commented Aug 24, 2016

axyr commented Aug 27, 2016

jarektkaczyk commented Aug 27, 2016 •

edited

Loading

taylorotwell Sep 2, 2016

jarektkaczyk Sep 2, 2016

jarektkaczyk commented Sep 8, 2016

GrahamCampbell commented Sep 8, 2016 •

edited

Loading

jarektkaczyk commented Sep 8, 2016

GrahamCampbell commented Sep 8, 2016

jarektkaczyk commented Sep 8, 2016 •

edited

Loading

GrahamCampbell commented Sep 8, 2016 •

edited

Loading

taylorotwell commented Sep 9, 2016

andrewkamm commented Dec 16, 2016

jarektkaczyk commented Dec 17, 2016 via email

lukepolo commented Dec 17, 2016 via email

andy-berry-dev commented Dec 18, 2016

andrewkamm commented Dec 19, 2016

lukepolo commented Jan 4, 2017

jarektkaczyk commented Jan 4, 2017 •

edited

Loading

lephleg commented May 11, 2017

lukepolo commented May 11, 2017

neylsongularte commented May 11, 2017

fico7489 commented Dec 6, 2017 •

edited

Loading

jehoshua02 commented Dec 18, 2018

vpillinger commented Apr 18, 2019 •

edited

Loading

[5.3] Fire BelongsToMany relation events for attach/detach/sync/toggle #14988

[5.3] Fire BelongsToMany relation events for attach/detach/sync/toggle #14988

Conversation

jarektkaczyk commented Aug 24, 2016

Notes on the commits:

axyr commented Aug 27, 2016

jarektkaczyk commented Aug 27, 2016 • edited Loading

taylorotwell Sep 2, 2016

Choose a reason for hiding this comment

jarektkaczyk Sep 2, 2016

Choose a reason for hiding this comment

jarektkaczyk commented Sep 8, 2016

GrahamCampbell commented Sep 8, 2016 • edited Loading

jarektkaczyk commented Sep 8, 2016

GrahamCampbell commented Sep 8, 2016

jarektkaczyk commented Sep 8, 2016 • edited Loading

GrahamCampbell commented Sep 8, 2016 • edited Loading

taylorotwell commented Sep 9, 2016

andrewkamm commented Dec 16, 2016

jarektkaczyk commented Dec 17, 2016 via email

lukepolo commented Dec 17, 2016 via email

andy-berry-dev commented Dec 18, 2016

andrewkamm commented Dec 19, 2016

lukepolo commented Jan 4, 2017

jarektkaczyk commented Jan 4, 2017 • edited Loading

lephleg commented May 11, 2017

lukepolo commented May 11, 2017

neylsongularte commented May 11, 2017

fico7489 commented Dec 6, 2017 • edited Loading

jehoshua02 commented Dec 18, 2018

vpillinger commented Apr 18, 2019 • edited Loading

jarektkaczyk commented Aug 27, 2016 •

edited

Loading

GrahamCampbell commented Sep 8, 2016 •

edited

Loading

jarektkaczyk commented Sep 8, 2016 •

edited

Loading

GrahamCampbell commented Sep 8, 2016 •

edited

Loading

jarektkaczyk commented Jan 4, 2017 •

edited

Loading

fico7489 commented Dec 6, 2017 •

edited

Loading

vpillinger commented Apr 18, 2019 •

edited

Loading