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

Pivot events do not fire #8452

Closed
tpavlek opened this issue Apr 16, 2015 · 9 comments
Closed

Pivot events do not fire #8452

tpavlek opened this issue Apr 16, 2015 · 9 comments

Comments

@tpavlek
Copy link
Contributor

tpavlek commented Apr 16, 2015

When creating (or performing any other action on a Pivot table), no model events are fired.

There is a proof of concept repository here: https://github.com/tpavlek/pivot-events-test

WelcomeController@index creates a new user and a new role, and attaches the role to the user. in AppServiceProvider we register this in boot()

    Pivot::creating(function($event) {
        \Log::info("Creating pivot...");
    });

Even after running the function in WelcomeController no data is logged, meaning no Pivot creating event was fired.

@morilog
Copy link

morilog commented Apr 18, 2015

has your user_role table an eloquent model?

@jarektkaczyk
Copy link
Contributor

@tpavlek There are no pivot events at all. Your model is not used by the BelongsToMany relation when attaching/syncing related models.

It would require changing the way this relation works, or adding such events to be fired. The latter was rejected by Taylor long time ago, can't remember why.

@GrahamCampbell
Copy link
Member

Closing due to inactivity.

@tpavlek
Copy link
Contributor Author

tpavlek commented Jun 1, 2015

@GrahamCampbell I don't really understand the justification for closing? Is this the same as labeling this a wontfix?

@ReallyPhil
Copy link

@GrahamCampbell Is it possible to re-open/consider this?

There are a number of other issues/PRs/feature-requests all relating to the same thing;
#10435 #10545 #10770 #4022 #2303 #1703

Some have workaround in app-level code but it seems widely expected that the framework should fire events on pivot table changes.

I think the limitation is that BelongsToMany doesn't use the Pivot model to interface with the DB but uses Query Builder directly; so doesn't benefit from the events Model provides.

Should BelongsToMany be updated to use the Pivot model - or - should BelongsToMany just fire it's own events? Or some other mechanism? It'd be good to get some feedback as to what a good approach here would be before coming up with a PR.

@zmorris
Copy link

zmorris commented Aug 20, 2016

I just want to point out that without Pivot model events or a way to override BelongsToMany::attach(), it forces us to break the don't repeat yourself (DRY) principle throughout our apps. Currently there is no solution to this issue:

http://stackoverflow.com/questions/37569142/overriding-attach-method-from-belongstomany-relationship

This is a real problem when trying to implement concepts from graph databases, since instead of being able to say $node->nodesWithSomeEdgeType('my_edge_type')->save($myNode), we have to create various convenience functions like $node->saveNodeWithSomeEdgeType('my_edge_type', $myNode).

I am considering extending the BelongsToMany class and overriding attach() or createAttachRecords() so that I can set additional columns on the pivot table row. Unfortunately, Model::belongsToMany() is too complex of a function to override in my child model class in order to return an ExtendedBelongsToMany, and there is no clear clone() or copy constructor for BelongsToMany that the extended class could use.

I feel this level of obfuscation is hurting developer productivity for more advanced use cases. We already have BelongsToMany::withPivot() and BelongsToMany::withTimestamps(), I don't see any reason not to have:

BelongsToMany::withPivot(['column' => function () {
    // return column value to be inserted into attach($model, $attributes) array
}])

or something to that effect for setting these columns upon attach(). I also don't see any reason not to provide a setPivotAttributes() accessor function of some kind on BelongsToMany that could be used within a model relationship method to prevent breaking the DRY principal. I hope these ideas are considered for the next version of Laravel. Thanks for all your hard work until then!

@fico7489
Copy link

fico7489 commented Nov 1, 2017

take a look at my package : https://github.com/fico7489/LaravelPivot

@Abhi0725
Copy link

This should be fired by framework itself.
@GrahamCampbell can we open this issue?

@shehi
Copy link
Contributor

shehi commented Jan 1, 2020

Why can't you use Model observers to handle real model events, also including relationship events in your flow as well?

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

No branches or pull requests

9 participants