spatie / laravel-medialibrary

Associate files with Eloquent models
https://spatie.be/docs/laravel-medialibrary
MIT License
5.77k stars 1.07k forks source link

clearMediaCollection() doesn't able to force delete if using softDeletes. #1474

Closed develsanket closed 5 years ago

develsanket commented 5 years ago

clearMediaCollection() doesn't able to force delete the database record and hence the files also not get removed from the storage. I'm using Laravel's softDeletes trait and while using the clearMediaCollection() method the record in the database gets soft deleted instead of forceDelete(). I think you should change the trait a little bit like what I do:

if (in_array(SoftDeletes::class, class_uses_recursive($entity))) { $this->getMedia($collectionName) ->each->forceDelete(); } else { $this->getMedia($collectionName) ->each->delete(); }

riasvdv commented 5 years ago

We'd accept a PR for this with tests

gundamew commented 5 years ago

I have quickly tried it on Laravel 5.8. It looks like all we need here is forceDelete.

// It works and no matter if App\User has SoftDeletes trait or not.
App\User::all()->each->forceDelete()

So maybe we could change the code like this:

// before
$this->getMedia($collectionName)->each->delete()

// after
$this->getMedia($collectionName)->each->forceDelete()
joshuadegier commented 5 years ago

Shouldn't this be debated first? I suppose when you make a model to implement SoftDelete it is not a given for everyone that the files should be deleted anyway. I can think of numerous of situations where I would want the record to be SoftDeleted but the files to be persisted (as it works now) for later use (or restoring the record including the media).

So, in my eyes, the solution wouldn't involve deleting the files without having a per-model setting which triggers forceDelete(). If I may suggest I'd feel more for a $forceDeleteMedia variable (defaults to false) on the HasMediaTrait which in turn uses forceDelete() over delete().

This change would be backward compatible with the way Medialibrary works now.

joshuadegier commented 5 years ago

I've made a draft for this: https://github.com/spatie/laravel-medialibrary/pull/1534

This PR only adds a single feature: whenever the model you are deleting uses SoftDeletes the media is deleted anyway when $shouldDeleteMediaWhenSoftDeleting is set to true.

freekmurze commented 5 years ago

Personally I'm not a fan of soft deletes in general and I don't want to go too far in supporting this. If you are using soft deletes and want to really delete media files, use $this->getMedia($collectionName)->each->forceDelete().