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

Add support for forceDelete() #7

Merged

Conversation

adriandmitroca
Copy link
Contributor

@adriandmitroca adriandmitroca commented Jun 8, 2016

Hi Michael,

I have found an issue where package couldn't handle situation, when you want to perform an actual DELETE statement to your model.

That's because you want to delete record that is associated with records from other tables with foreign keys so you get a SQL error like this:

SQLSTATE[23000]: Integrity constraint violation: 1451 Cannot delete or update a parent row: a foreign key constraint fails (`dbname`.`comments`, CONSTRAINT `comments_review_id_foreign` FOREIGN KEY (`review_id`) REFERENCES `reviews` (`id`)) (SQL: delete from `reviews` where `id` = 1)

This little change handle makes this possible now.

@michaeldyrynda
Copy link
Owner

This is for when you're using the soft deletes in combination with database-level foreign key constraints, right?

Would it be too much trouble to get you to add an integration test along with this change, obviously there's no regression, but want to test for the new behaviour, too.

@adriandmitroca
Copy link
Contributor Author

This isn't generally about soft deletes here. It allows to cascade delete relations using forceDelete() method which behind the scenes it's just regular DELETE statement.

$model->delete() // UPDATE, SET deleted_at = Carbon::now()
$model->forceDelete() // DELETE

I will provide example unit test by tomorrow, that's definitely good idea.

@michaeldyrynda
Copy link
Owner

Sure, but what I was getting at was the fact that the error you encountered was because you were using both soft deletes at the application level and foreign key constraints at the database level.

Without that, the error wouldn't have surfaced. The error occurring is definitely a good thing as it means that we can handle the behaviour better.

@adriandmitroca
Copy link
Contributor Author

The unit test was added.

Yes, I was using foreign key constraints at the database level - I think of that like design requirement. But still, only thing that was added here is handling Eloquent's forceDelete() method properly.

@michaeldyrynda michaeldyrynda merged commit d7c30c4 into michaeldyrynda:master Jun 9, 2016
@michaeldyrynda
Copy link
Owner

Thanks for this @adriandmitroca!

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