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

[8.x] Fire pivot detach events when using custom class #36300

Closed
wants to merge 2 commits into from

Conversation

taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Feb 17, 2021

Fixes #36291

This adjusts the detach logic when using custom classes such that the deleting and deleted events are fired for the pivot models. This also adjusts new pivot queries to wrap their conditions in a logical group for a lesser chance of unexpected query results.

The events are fired by retrieving the pivot models and then maintaining the same logic where we rehydrate new empty models. I tried to keep the logic as close as possible to the original implementation.

The decision to not fire these events was done here: #27997 - but I believe that was more of a "quick fix" to stop a breakage when pivot events were added. The original implementation did not carry over any additional where clauses when using a custom pivot class and thus caused the bug where all pivot records were deleted. However, this PR fixes that by carrying over the where clauses.

Also, in the meantime since the original PR, @themsaid fixed an issue with updating existing pivots (#28416) ... this also carries over where clauses. So, I don't think we need to check for empty where clauses before using updateExistingPivotUsingCustomClass.

@taylorotwell
Copy link
Member Author

One reason we may not want to merge this is that is essentially a mass delete operation and we never fire events for those types of situations for the very reason that we have to take the performance hit of retrieving all of the models.

@GrahamCampbell GrahamCampbell changed the title Fire pivot detach events when using custom class [8.x] Fire pivot detach events when using custom class Feb 17, 2021
@taylorotwell
Copy link
Member Author

I don't think we will end up merging this. In general, mass update / delete operations do not fire events because of the performance implications of having to retrieve every model matched by the query. If you need individual model events for these types of operations you should retrieve them yourself and delete them individually.

@driesvints driesvints deleted the pivot-detach-events-with-custom-class branch February 18, 2021 08:58
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

Successfully merging this pull request may close these issues.

When use ->sync() with condition wherePivot() deleted or deleting events dont firing
1 participant