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

Cascading restore #75

Closed
wants to merge 2 commits into from
Closed

Cascading restore #75

wants to merge 2 commits into from

Conversation

marksalmon
Copy link
Contributor

Hi Michael,

Thanks for the very useful package.

I note in the docs its says:

Note: It's important to know that when you cascade your soft deleted child records, there is no way to know which were deleted by the cascading operation, and which were deleted prior to that. This means that when you restore the blog post, the associated comments will not be.

Given that deleted_at is a timestamp, surely we can determine relations that were deleted as part of the cascaded soft delete if their deleted_at value is equal to (or greater than, just in case the operation takes a second) the parent model's deleted_at value.

This is how I implemented it in my app so I thought it might be worth a PR.

@michaeldyrynda
Copy link
Owner

Made a few small tweaks to the tests and merged in - thanks @marksalmon

@michaeldyrynda
Copy link
Owner

I've reverted this as it's not going to behave consistently or, ultimately, correctly.

The way that the deletes are executed via the deleting event, the Post will be the last record to be deleted, meaning that its timestamp is going to always be ahead of any children deleted as part of the cascade i.e. calling $post->restore() won't always restore all children reliably (maybe there are lots and even a second's difference will throw it out).

CleanShot 2025-01-07 at 09 22 05

In the test scenario as written, this works because the cascade is effectively instant. However, if we elongate time (using Sleep::for(1)->second() between each delete, the test ends up failing.

CleanShot 2025-01-07 at 09 22 24

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