laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.55k stars 11.03k forks source link

You can softdelete over and over a model that is already softdeleted #30541

Closed musapinar closed 5 years ago

musapinar commented 5 years ago

Description:

having a Post model which uses the SoftDeletes trait, the Eloquent Model does not throw any error if you softdelete it over and over :

$post->delete();
$post->delete();
$post->delete();
$post->delete();

which will also trigger the model observer event deleting() over and over. How to avoid this? If it's not a bug, isn't this a lack of consistency? I mean you cannot do the following on a softdeleted model :

Post::find('id');

You have to do this instead :

Post::withTrashed()->find('id');

Why not the same requirement for the delete() method? By the way, this applies to the restore() method as well (you can restore something already restored, over and over)

This makes the restored() event observer unusable

Steps To Reproduce:

Create a model that uses the softdelete trait. Retrieve the model and delete it 99 times. It won't throw any error and fire the deleted event 99 times.

Possible fix : ugly if statements and check like this https://i.imgur.com/4sucZMR.png

devcircus commented 5 years ago

Just my opinion but I think this should be handled at the app level.

One reason of many would be that the date that is added into the deleted_at column is likely different each time I call delete. This value could be important in some apps. I wouldn't want the framework deciding to skip the action of updating that value just because there's already a value present.

When you're performing an action on a model that could change its state, you should confirm that the model is in the state it needs to be before it is acted on.

antonkomarev commented 5 years ago

If you are doing delete only once in controller - on the next call model will be deleted after retrieval. Same for the restore.

musapinar commented 5 years ago

@devcircus The deleted_at column is indeed updated each time you call delete(). You said it yourself, with your own word, which is basically "updating the value of the deleted_at column". So you are updating. Why not use the update() method? You are not deleting anything (it is already soft deleted). You are just updating a column of a soft deleted record. Again, you are not deleting it. I do not expect a method called "delete" to update a record (which it technically does here, of course, for the very purpose of soft deleting the record), but to delete a record. What about semantics?

@antonkomarev I do not understand.

My controllers are fine, I handle it at the "app level" like you said. I just figured that "issue" during my tests and that bothered me.

public function test_it_restores_and_increments_tags_once_taggable_is_restored()
{
    $post1 = factory(Post::class)->create();
    $post2 = factory(Post::class)->create();
    $post1->saveTags('alpha');
    $post2->saveTags('beta');
    $this->assertEquals(2, Tag::count());

    $post1->delete();
    $post1->delete(); // trying to soft delete a taggable that is already soft deleted
    $post1->delete();
    $post1->delete();
    $post1->delete();
    $post1->delete();
    $post1->delete();

    $tags = Tag::get();
    $this->assertEquals(2, Tag::count());
    $this->assertEquals(0, $tags->where('name', 'alpha')->first()->count); // tests will fail starting from here
    $this->assertEquals(1, $tags->where('name', 'alpha')->first()->trashed_count);
    $this->assertEquals(1, $tags->where('name', 'beta')->first()->count);
    $this->assertEquals(0, $tags->where('name', 'beta')->first()->trashed_count);

    //$post1->restore();
    //$post1->restore(); // trying to restore a taggable that is already restored
    // ...
}
driesvints commented 5 years ago

Hi there,

Thanks for reporting but it looks like this is a question which can be asked on a support channel. Please only use this issue tracker for reporting bugs with the library itself. If you have a question on how to use functionality provided by this repo you can try one of the following channels:

However, this issue will not be locked and everyone is still free to discuss solutions to your problem!

Thanks.