wintercms / winter

Free, open-source, self-hosted CMS platform based on the Laravel PHP Framework.
https://wintercms.com
MIT License
1.35k stars 192 forks source link

Allow forced deletion of soft-delete models #1212

Open lex0r opened 5 days ago

lex0r commented 5 days ago

Package targeted

Winter CMS

Description

By definition, soft-delete enabled models are not expected to be permanently deleted. There's an exception to this rule, when developers rely on soft-delete as a sort of "inactive" attribute. Having such an attribute implies a possibility to "activate" at some point later (i.e. restore). Another possible operation for an "inactive" content may be actually a permanent deletion for the sake of good data housekeeping.

Of course the above can be achieved using an extra field like "status" or similar. This comes with some extra work needed to implement this, including a custom scope to hide inactive records. The amount of work needed is probably 95% repeats what is already available with soft deletion feature. So it comes down to the question - can soft delete cover the 5% change or that would be a form of featuritis?

It makes sense to extend the existing handling of soft-delete models to allow for a forceDelete() call on the model being deleted. This could be done with minimum modifications using an extra data parameter on the delete request (like post('permanentDelete')) inside FormController's respective method.

Will this change be backwards-compatible?

No response

mjauvin commented 5 days ago

There is already a forceDelete() method: https://github.com/wintercms/storm/blob/develop/src/Database/Traits/SoftDelete.php#L83-L90

mjauvin commented 5 days ago

Also see the documentation: https://wintercms.com/docs/v1.2/docs/database/traits#soft-deleting

lex0r commented 5 days ago

@mjauvin I'm sorry I wasn't clear enough about what I would like to see as a feature in Winter CMS.

You are right about the existence of forceDelete(), which I did mention in the issue.

My request is about the gap that exists between forceDelete() and backend UI backed by FormController. Current FormController implementation doesn't care about whether the model which data it handles is a soft delete or non-soft delete one. As the result, it's impossible for a developer to easily (for example by adding a relevant button parameter) ensure permanent deletion of soft-delete content. One has to either come up with a solution based on model.afterDelete event to actually force delete the model, or override update_onDelete() implementation of FormController to do the same (or maybe use formAfterDelete()).

There's an easy win to bridge this gap out of the box by allowing to forceDelete() using declarative approach of defining an extra data-request-data on the deletion button.

FYI @bennothommo @LukeTowers

mjauvin commented 5 days ago

You can easily add a button to your controller's update.php view like below:

    <button
        class="btn btn-default oc-icon-refresh"
        data-request="onPurgeDeleted"
    >REALLY DELETE
    </button>   

And just add a onPurgeDeleted() method to your controller.

public function onPurgeDeleted($id)
{
    $this->formFindModelObject($id)->forceDelete();
}

Am I missing something here?

lex0r commented 5 days ago

Am I missing something here?

FormController's implementation of deletion is a bit longer and I'm afraid developers risk breaking compatibility with it unless they duplicate update_onDelete() and add own custom handling like above. Which is of course an option but I thought this could be extended a little mainly because it's a rather typical use case (SoftDelete trait has forceDelete() for a reason).

It's about developer productivity mostly. Imagine having to do the above in 5+ controllers, or instead have it out of the box accessible with:

data-request-data="forceDeleting: 1"

LukeTowers commented 5 days ago

@lex0r you could always use a trait to add it to multiple controllers

lex0r commented 4 days ago

@lex0r you could always use a trait to add it to multiple controllers

Well yes, just that it becomes a little clumsy because I will have to override two methods from FormController (the action handler and getLang() which is protected), and keep an eye on both in the unlikely case they change...

lex0r commented 4 days ago

I thought would be nice for FormController to become aware of soft-delete models. They are not uncommon after all.

mjauvin commented 4 days ago

Feel free to submit a PR if you think this can be generic enough to be helpful to the masses

bennothommo commented 3 days ago

I'm not opposed to us adding in a check for a forceDelete parameter that could be defined with the data request and interpreted by the onDelete action in the FormController behaviour to run forceDelete as opposed to delete. I think that is what @lex0r is suggesting.

LukeTowers commented 3 days ago

@bennothommo any ideas for how to protect it from misuse? I wouldn't want to add it and allow it to be triggered by users without the developers realizing by simply including an extra parameter in the delete requests.

bennothommo commented 3 days ago

The alternative would be to allow it to be configurable in the form config, if you didn't want it to be triggered by a parameter in the request.

LukeTowers commented 3 days ago

Can anyone provide some sample use cases here for us to consider?

bennothommo commented 3 days ago

A potential use case would be for things that could be deleted (softly) elsewhere, but you wanted the Backend to be a place where an admin could permanently delete it, and instead of having to create a custom delete action or override the Form Controller's method, you just define that parameter in the form config.

For example, I create a Comments plugin to leave comments on a page. I could allow a "delete" action for users to delete their own comments on the front-end, leaving a "deleted message" placeholder in its place, but I could also have a Backend page to review all comments (including deleted ones) with the ability to completely delete the comment if I wish.