Skip to content
This repository has been archived by the owner. It is now read-only.

Deleting an item should always delete an item #789

Closed
danielbachhuber opened this Issue Jan 19, 2015 · 29 comments

Comments

Projects
None yet
@danielbachhuber
Copy link
Member

commented Jan 19, 2015

Core has this weird pattern where you can pass force = true when deleting a post or comment. However, force = false can produce this behavior:

  • Post isn't yet trashed => Post is marked as "trash", but not deleted.
  • Post is already trashed => Post is permanently deleted.

This behavior is inconsistent, and an idiosyncrasy of core internals.

In the API, DELETE <id> should always delete the item. To trash an item, clients should be expected to PUT <id> status=trash, just like any other status.

Related #618.

@danielbachhuber danielbachhuber added the Bug label Jan 19, 2015

@danielbachhuber danielbachhuber added this to the 2.0 milestone Jan 19, 2015

@rmccue

This comment has been minimized.

Copy link
Member

commented Feb 11, 2015

One issue with always deleting is that it means that the trash will get ignored for the most part, since clients will just send a DELETE without caring about the internal intricacies of WP's trashing system. This is confusing for users, who see the action of deleting to mean "move to trash".

That said, I do not like the current behaviour of force = false causing a permanent delete the second time around. DELETE is idempotent, so doing the same request twice shouldn't cause different behaviour.

What about DELETE <id> = trash item (no action if already trashed), DELETE <id> force=true = permanently delete?

@danielbachhuber

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2015

since clients will just send a DELETE without caring about the internal intricacies of WP's trashing system.

Why should they care? This implies they have some prior knowledge about idiosyncrasies of WordPress internals, and want to maintain that behavior. I think trash is just another status.

If trashing is some special core behavior we should maintain, then we should support it for users, meta and terms

@rmccue

This comment has been minimized.

Copy link
Member

commented Feb 11, 2015

Trashing is part of the core publishing flow, which might indicate we can treat it as part of the status field, however it has pretty special behaviour. The trash status is the internal status to indicate that a post has been deleted, but that we're maintaining it temporarily to allow undo and similar behaviours. This is reflected by the fact that WP cleans up and permanently deletes trashed items after a certain number of days (see wp_scheduled_delete). "Trashing" is really just a deferred permanent delete.

(I'd argue by exposing the trash status we're revealing more of the internal behaviour than I'd like, but there are sensible reasons for that too.)

If trashing is some special core behavior we should maintain, then we should support it for users, meta and terms

It's up to the core team to decide whether other core objects should support trashing, not a call we can make here.

@danielbachhuber

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2015

I disagree then. I don't think we should expose complex idiosyncrasies through the API. DELETE is established language that means a simple thing.

I suspect there's history (and some hacks / compromises) for why core's trash behavior is closely coupled with deleting.

@joehoyle

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2015

+1 for @rmccue's suggestion. ?force=true should always permanently delete, DEETE alone should always send to trash

@danielbachhuber

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2015

Before we make a final decision, someone should look at the history of core's behavior and figure out why trashed posts are implemented this way. I think it would be useful context.

@mdawaffe

This comment has been minimized.

Copy link

commented Feb 12, 2015

As an uninformed outsider looking in, I like @danielbachhuber's suggestion better.

The original ticket: https://core.trac.wordpress.org/ticket/4529

I'm reasonably sure wp_delete_post() trashes a post for the sake of old plugins/code paths. When the trash feature was created, WP wanted to "upgrade" old plugins/code paths automatically to use the trash without having to change the plugins'/paths' code.

Yes, I made that up with no data and without really reading the ticket :)

If that's true, there's no need for the API to follow suit. DELETE can do what it says and PATCH status=trash (or whatever verb) can do what it says.

Even if it's not true, do we want to live in a world where we have DELETE changing the properties of an object, and we require extra properties (what does it mean to include a request entity with a DELETE? What is a "force"?) to do what we say we mean?

Here are some arbitrary analogies different types of users might be familiar with that I've inappropriately tried to shoehorn into the issue with no real value to the discussion :)

WordPress PHP API

wp_delete_post() trashes, wp_delete_post( $force = true ) deletes.

This is Ryan's proposal: DELETE trashes and DELETE force=true deletes.

WordPress UI

Trash trashes, Delete Permanently (more or less only available from trash) deletes.

I guess this is TRASH (or maybe PATCH status=trash) trashes and ... TRASH_THEN_DELETE deletes?

This is what the WordPress.com API emulates, and I don't like it.

rm

This is DELETE deletes. There is no trash.

rm -i

This is DELETE does nothing and DELETE force=true deletes, which is silly :)

OS X Right Clicking a file

This is DELETE does not exist and PATCH status=trash trashes. Or maybe this is DELETE does not exist, and TRASH exists and trashes.

OS X ⌘-Delete

This is DELETE trashes, and delete does not exist.

HTTP

http://tools.ietf.org/html/rfc7231#section-4.3.5

Some potentially relevant quotes:

A payload within a DELETE request message has no defined semantics; sending a payload body on a DELETE request might cause some existing implementations to reject the request.

 

If a DELETE method is successfully applied, the origin server SHOULD send ... a 200 (OK) status code if the action has been enacted and the response message includes a representation describing the status.

I don't know if that means "the status" of the now deleted the resource, or "the status" of the delete action.

The way I read the HTTP spec, DELETE should make the resource inaccessible from the (now deleted) path.

So if we want to get really crazy, we could do something horrifying like:

DELETE /posts/123

201 Created
Location: /trashed-posts/123

And with that, I'm going to walk away and let you all ignore everything I said :)

@danielbachhuber

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2015

@WP-API/amigos How do we reach consensus on this?

@rmccue

This comment has been minimized.

Copy link
Member

commented Mar 1, 2015

We discussed this on one of the calls, and I think we were all happy with, as @mdawaffe phrased it:

This is Ryan's proposal: DELETE trashes and DELETE force=true deletes.

Do we have any objections to that? (IIRC, @joehoyle, @danielbachhuber and myself all agreed on that; @rachelbaker was absent; and @mdawaffe's suggestion is insane ;) )

DELETE can do what it says and PATCH status=trash (or whatever verb) can do what it says.

FWIW, I disagree with this, because it seems like a leaky abstraction: we're exposing the trash status from the guts of WP. Really, status = trash is just a deferred delete: the post no longer exists, but we keep the data in case you want to undo it. (WP then cleans the trashed posts up 30 days later.)

@mdawaffe

This comment has been minimized.

Copy link

commented Mar 1, 2015

we're exposing the trash status from the guts of WP

FWIW, we already do that with that with the status (or is it post_status?) request variable.

@rmccue

This comment has been minimized.

Copy link
Member

commented Mar 1, 2015

FWIW, we already do that with that with the status (or is it post_status?) request variable.

That's true, and a fair point. I still think the workflow makes more sense with DELETE = trash, as it's pretty likely to be what you're intending to do. If we default to permanently deleting, the trash then becomes opt-in, which doesn't seem like something we want.

(I've started work on the PR in #959.)

@danielbachhuber

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2015

I'm with Mike - DELETE should always be delete. We shouldn't expose WP idiosyncrasies through the API

@danielbachhuber

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2015

@justinshreve

This comment has been minimized.

Copy link

commented Mar 28, 2015

I'm not sure if I ever put my two cents in on this either, but I agree with Mike and Daniel. We did delete = trash in the WordPress.com API and required two deletes to trash, and I originally thought that made sense, but I think it's really confusing. If you DELETE some other resource it deletes, but suddenly the behavior is different for posts? I think in terms of API design and outside developers, DELETE = delete makes much more sense.

@rmccue

This comment has been minimized.

Copy link
Member

commented Mar 30, 2015

#618 has important discussion on the response we should give in any case.

@nacin

This comment has been minimized.

Copy link

commented Apr 8, 2015

As a WordPress person, I feel like DELETE should trash. @mdawaffe more or less guessed it correctly that it had to do with code paths. A lot of it was also because it's a better experience to send stuff to trash than to outright delete once we had trash.

But, I agree that HTTP DELETE should delete.

Cross-referencing: https://core.trac.wordpress.org/ticket/11470. One of my first tickets suggested that wp_delete_post() should actually delete. I'm glad it wasn't followed. :-)

@rmccue

This comment has been minimized.

Copy link
Member

commented Apr 14, 2015

We discussed this on Slack, and made a decision:

  • DELETE /wp/posts/<id> will trash the post. If the post is already trashed, no action will be taken (we will likely return a 410 or similar).
  • DELETE /wp/posts/<id>?force=true (param name TBD) will permanently delete the post, regardless of current state.

With the edge-cases, we follow the logical consequences given WP's workflow. The workflow that WP follows is:

  1. Post is marked as trashed
  2. On a cron schedule (daily), the trash is checked for "expired" trash posts (posts trashed more than EMPTY_TRASH_DAYS days ago, 30 days by default)
  3. "Expired" trash posts are cleared from the trash

This leaves us with one edge case: EMPTY_TRASH_DAYS is set to 0 (trash disabled). We treat this conceptually as expiring the trash post instantly, and clearing it instantly. (The implementation will obviously just delete it.) The net effect is that DELETE and DELETE force=true will have the same behaviour for this.

There's one other ambiguous case: the post type doesn't support trashing. There's currently no way to indicate that with post types; wp_delete_post checks the post type directly against post or page. We will replicate the same behaviour for now, and treat non-supported types similarly to the above. To change this, core needs to add a post_type_supports check.

#959 is the canonical PR for this.

@nacin

This comment has been minimized.

Copy link

commented Apr 16, 2015

Sorry, reopening this discussion and am a -1 for any merge.

I don't think we have enough information about what is expected behavior from a RESTful API. I get that trash is a deferred delete, but I find that justification not quite strong enough to proceed here. I am not convinced in either direction abides by the principle of least astonishment.

Google Drive skips the trash: https://developers.google.com/drive/v2/reference/files/delete

I would like to seek some additional guidance -- names that come to mind include caseysoftware, kinlane, mikekelly, and @maxcutler. (@'s suppressed so we can better spell out our problem statement before dragging them into a random thread.) I venture to guess there may be some more academic reasons the API Craft crowd could share with us that could justify this one way or the other.

@apokalyptik

This comment has been minimized.

Copy link

commented Apr 17, 2015

So… My opinion is that: I think trashed or not should be treated as a property of the object. It’s analogous to an actual trash can. When placing something in the trash you’ve altered the property of its location but it still exists, you can remove it, alter it, etc, and it remains the same thing whether it's in the trash can or not. Deleting an object, however is an existential act. For an analogy: imagine a unicorn. Now stop imagining it. That unicorn has been deleted: it’s not somewhere else, it’s gone; you can’t get it back, you can only create a new one.

So trash/untrash should be a property modification just like password/not published/not etc. and the DELETE verb is cessation of its existence.

@danielbachhuber

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2015

Non-post contexts I think should be considered or addressed with each proposal:

  • wp_delete_user() deletes a user on single site, but only removes the user from the blog on multisite. What should DELETE wp/users/1 do on single site vs. multisite?
  • In the WordPress admin, a "trashed" post isn't editable in the trash. You either need to "Restore" or "Delete permanently". What should be accessible of a post through the API when it's in a trashed state? How should a client perform the "Restore" operation?
  • Comments are permitted to be trashed as well.
@apokalyptik

This comment has been minimized.

Copy link

commented Apr 17, 2015

A user is a shared resource across many sites. A post exists only for one site. These two are apples to oranges. Though I do see your point. It's not perfect but global resources can have a different behavior with the caveats noted.

The WordPress admin isn't exactly a concern. Allowing editing of deleted posts doesn't seem subversive. Consider wp-admin as a front end to the WordPress API you could easily make that same choice in the UX/UI context without any technical reason for it on the back end.

I'm unsure why comments being trash-able is relevant, they can be treated the same way. Self consistency between posts and comments (local resources) would be a good thing.

@jkudish

This comment has been minimized.

Copy link

commented Apr 18, 2015

FWIW, I agree with @apokalyptik and @justinshreve -- a DELETE request should permanently delete the resource. It's up to a client to put up any safeties that might be needed.

@maxcutler

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2015

In terms of prior art:

  • Dropbox - delete API moves to trash, no way to permanently delete through API
  • OneDrive - same as Dropbox, no hard delete through API.
  • Box has a more explicit notion of a trash folder in their API, with options to delete or restore a file/folder from the trash.
  • Amazon Cloud Drive - Similar to Box, has explicit trash APIs
  • Enchant - has trash boolean on objects that you PATCH to move to/from trash, and DELETE is a hard delete
  • Confluence - works like WP core, where first DELETE soft-deletes and second one hard-deletes

Some design discussions:

@mdawaffe dismissed the idea of treating trashed items as a separate resource endpoint, but that seems like a fairly popular design from what I've found.

The other popular option is to expose trash as a form of post_status or standalone boolean. I understand @rmccue's concerns here as I've seen this confuse non-WP devs in the XML-RPC API, but I still think it's a reasonable approach. The set of post statuses is already fairly confusing, and I don't think adding trash makes it any worse and at least reflects the real state of that object within WP core.

From the client dev perspective, in any approach I'd want the API to tell me which options are available so that I can give users a choice. If DELETE is an overloaded operation, then the GET response for a post needs to explain what the result of DELETE will be. If you go the post_status route, then the GET response needs to indicate whether this user can transition to trash status and/or can hard-DELETE it.

@rachelbaker

This comment has been minimized.

Copy link
Member

commented Apr 22, 2015

Merged #959 as a bug fix for Beta 1, and moving this issue to the Beta 2 milestone for a more permanent resolution.

@rachelbaker rachelbaker modified the milestones: 2.0 Beta 1, 2.0 Beta 2 Apr 22, 2015

@quasel

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2015

After thinking about it wp UI doesnt allow an item to be deleted if there is a trash it goes there first period. It's like a saftey net and developer know there way around it if nessecary. With wp delete results (in most cases) into a delayed delete. Currently i would vote in favor of keeping it that way and do somethin like its now with force, add trash as post_status (wich should be done anyway) , and force delete when status is trash

  • i would suggest to at least rename it to _force to keep it inline with #634
@rmccue

This comment has been minimized.

Copy link
Member

commented May 3, 2015

My opinion hasn't changed on this one, and likely won't unless there's significant issues or new evidence in favour of permanent deletion.

I think at the core of this, DELETE needs to cause the item to be trashed if the trash is available. We also need to keep available the ability to permanently delete items.

Ideally, we want consistent behaviour here, so that brings up the question of "what do we do for untrashable items?" - that is, core objects that don't support being trashed. I think it's possible that down the line core will support these being trashed, so we should plan with that eventuality in mind, even if it never happens. For example, what if terms get a deleted column added as a boolean? At that point, using a PUT with a status field change strays dangerously far from the core model (what if core adds a new status property later that happens to be inconsistent?). For internal consistency, I think we're better off incorporating both trashing and permanently deleting into DELETE, regardless of which the default is.


I don't think prior art/use in the wild supports either trash or permanently delete, as evidenced by some of the APIs @maxcutler checked above. One thing that does support trashing though is the change in terminology in the latest HTTP/1.1 spec update, where the definition for DELETE changed from:

The DELETE method requests that the origin server delete the resource identified by the Request-URI.

to:

The DELETE method requests that the origin server remove the association between the target resource and its current functionality. In effect, this method is similar to the rm command in UNIX: it expresses a deletion operation on the URI mapping of the origin server rather than an expectation that the previously associated information be deleted.

I couldn't find any relevant discussions in the httpbis working group where this change was discussed, but it's a significant enough one that stresses DELETE does not imply the data is completely destroyed.


From a user standpoint, I think we need to stick to the principle of least surprise. If I know WP supports trashing posts, and I say "OK WordPress, delete this post", I expect that post to be trashed. In my head, "delete" means "remove this post from my site", it doesn't mean "remove all data associated with this post from everywhere possible".

The user expects that deleting will remove the post from being visible for all intents and purposes. For us, trashing the item does fulfill what one expects: it removes the item from being visible anywhere except in the trash. Permanently deleting things is a rarely performed operation, especially because the trash cleans itself; I don't remember the last time I cleared my Gmail trash, or if I ever have.

We can also look at prior art here in WordPress. WP shifted from the "are you sure" model, where you're asked before your data is destroyed, to the undo model, where the data is kept around in case you change your mind. We didn't do this because the user is dumb and doesn't know what "delete" implies, but rather because people make mistakes. People are also going to make mistakes when using and developing API clients, and I'd rather they have 30 days to undo those mistakes.

@rmccue

This comment has been minimized.

Copy link
Member

commented May 3, 2015

Specific replies.

@nacin:

I don't think we have enough information about what is expected behavior from a RESTful API. I get that trash is a deferred delete, but I find that justification not quite strong enough to proceed here. I am not convinced in either direction abides by the principle of least astonishment.

With DELETE causing a trash, if you don't realise this beforehand, you have 30 days to correct your mistake. With DELETE permanently deleting, if you don't realise this, you no longer have any data. After 30 days, both converge anyway. If we're going to be astonishing to the user, I'd rather us err on the side of being conservative and keeping too much data around.

@apokalyptik:

Deleting an object, however is an existential act

I agree, but I also think trashing is an existential act in this scenario. To continue the analogy, if the unicorn becomes invisible and non-solid to everyone except when using a special light (that you use to make it non-invisible), then it's essentially non-existent. (Although we could get philosophical: If a tree falls in the woods...)

If you "delete" a post and it trashes it, it's removed from all of your frontend and it's removed from almost all admin UI, which fulfills your original intention. The only way to realise it still exists is to go looking for it specifically because you need it back.

@maxcutler:

@mdawaffe dismissed the idea of treating trashed items as a separate resource endpoint, but that seems like a fairly popular design from what I've found.

I do quite like this idea. /posts/trashed/<id> I think is quite nice, and can be adapted to the relevant model as well (comments, etc). It is somewhat annoying, and I'm not sure how we'd communicate it either; a 302 response to a DELETE seems strange.

@danielbachhuber:

What should be accessible of a post through the API when it's in a trashed state? How should a client perform the "Restore" operation?

I think those are specifics of the implementation, but I'm thinking a POST on the trashed resource, to "recreate" the resource again. I think the resource itself should be available, but linked resources probably not (i.e. you can access a trashed post, but not trashed post meta), although there's no real reason we need that distinction.

@maxcutler:

From the client dev perspective, in any approach I'd want the API to tell me which options are available so that I can give users a choice.

I agree; we need to tell users what's available in a nice way so that they know whether an item is trashable or not beforehand.

@quasel:

Currently i would vote in favor of keeping it that way and do somethin like its now with force, add trash as post_status (wich should be done anyway) , and force delete when status is trash

This was the previous behaviour (DELETE changes based on whether it's trashed), and that's definitely broken. Not only is it harder for users to understand what will happen, but it also breaks DELETE being idempotent, which is a fundamental property of it.


(End-note: I'd also like to refrain from DELETE = trash vs DELETE = delete. I think the terminology here can be harmful to the discussion, because it seems like DELETE = delete makes intuitive sense immediately due to the phrasing, without considering the reasoning behind each.)

@mncahill

This comment has been minimized.

Copy link

commented May 5, 2015

Traditionally in Newspaper or Content Management systems Delete is never truly a delete, it is actually more akin to archive. Kill functions, which are generally not available to anyone but super admins, are true delete functions. As Nacin properly notes, a true delete would leave you no way to correct an error.

@danielbachhuber

This comment has been minimized.

Copy link
Member Author

commented May 8, 2015

We've decided delete=trash. This is a design decision in favor of the user, because the spec is ambiguous.

Thanks to everyone who weighed in on this thread, in particular @maxcutler who spent the time researching behavior of existing APIs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.