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

[11.x] Update detachUsingCustomClass to trigger delete event with full model record #51776

Conversation

abetwothree
Copy link

@abetwothree abetwothree commented Jun 11, 2024

TLDR: This update allows the any listener to the delete event on a pivot model record to receive the full set of columns (including the PK) instead of just the foreign keys.

Fully compatible with 10.x branch as well.

Long Description: I am working on a feature to audit pivot column changes. Attach and detach work fine to create audits. However, detach (the delete event) failed to create an audit record because my audits table does not allow null values for auditable primary key value. The issue is that the detach section was creating a new pivot record only with the foreign keys leaving out all other columns from the delete event.

While debugging I dug into the history of Laravel supporting pivot records triggering events and this change brings the framework more in line with how the updateExistingPivot method works when using a pivot model in the updateExistingPivotUsingCustomClass method.

image

I also made the delete null safe in case the record has already been deleted by another process it doesn't throw a "call to member function delete() on null" as was fixed on this pull request for the pivot update section -> #29362

Relevant links to other PRs or commits related to this change:

Happy to add necessary test but generally speaking this change should match all existing tests.

Thank you for your consideration!

Abraham

@abetwothree abetwothree changed the title Update detachUsingCustomClass to trigger delete event with full model record [11.x] Update detachUsingCustomClass to trigger delete event with full model record Jun 11, 2024
@taylorotwell
Copy link
Member

I don't think we can change this on a patch release.

@abetwothree
Copy link
Author

@taylorotwell This change wasn't breaking any functionality but I can make a PR to target the master branch if that's a better route for this change.

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.

2 participants