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

Fixed hard user deletion #785

Closed
wants to merge 2 commits into from
Closed

Fixed hard user deletion #785

wants to merge 2 commits into from

Conversation

splitt3r
Copy link
Contributor

This https://github.com/laravel/framework/blob/5.4/src/Illuminate/Database/Eloquent/SoftDeletes.php#L33 method was causing an infinite loop on hard user deletion.

@@ -180,10 +180,10 @@ public function delete($hardDelete = false)
// TODO: remove any persistences

// Delete the user
$result = parent::forceDelete();
Copy link
Contributor Author

@splitt3r splitt3r Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing from delete to deleteUser breaks the model API - are you sure we need to change the name of the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise the traits forceDelete Method calls the this method. See: https://github.com/laravel/framework/blob/5.4/src/Illuminate/Database/Eloquent/SoftDeletes.php#L33

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, got it. It sounds like maybe overriding User::delete to support hard/soft deletes was a bad idea in the first place. But do we need a deleteUser method either? It sounds like maybe we should use model events to handle deleting/modifying related entities instead. Choosing to do a hard/soft delete could be handled in the controller method.

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.

None yet

2 participants