Have wp_delete_comment trash/delete the same way as its post and attachment counterparts

[nacin]

In 2.8, if a plugin wanted to delete a post, it called wp_delete_post($id).

No more. Now, that simply calls wp_trash_post($id) and sends the post to the trash. You need to call wp_delete_post($id, true) to delete a post now, as in 2.9 the function is now wp_delete_post( $id, $force_delete = false ).

Since wp_trash_post() exists, then wp_delete_post() should do what it has always done -- delete the post. If a plugin wanted a post to be trashed, they can use wp_trash_post().

After ​an IRC discussion with azaozz, it appears intended to widen the scope of the Trash. But they're separate and equal functions and should perform their own actions. These should be standard across the board else the Trash API gets confusing. Otherwise, we have two functions, designed to perform two different actions, peforming the same action by default, which is counter-intuitive.

$force_delete also introduces consistency issues with 3.0. With media trash in core as expected, there will be wp_trash_attachment(). But wp_delete_attachment() has that same $force_delete = false argument that wp_delete_post() now has.

Suggested fix:

• Remove $force_delete arguments from wp_delete_post() and wp_delete_attachment().
• Remove wp_trash_post() call from wp_delete_post().
• Create wp_trash_attachment() -- better to standardize it now than to have a single function with $force_delete later when this whole API likely becomes wider functions with some wrappers.
• Remove trash actions from wp_delete_attachment().
• Go through core references of wp_delete_post() and wp_delete_attachment() and modify them as appropriate.

[westi]

The current implementation means that plugins don't have to know anything about trash and any delete options they offer are automatically converted to trash options I much prefer this behaviour over requiring every plugin author that offers a delete link having to put trash specific code in.

I also don't think we should change this logic this close to release.

[azaozz]

Yes, basically we have two options:

1. Merge wp_trash_post() into wp_delete_post() and do both trashing and deleting from there.
   • Pros: will catch all posts that are deleted and trash them instead.
   • Cons: will need for plugins to adapt by adding second param if they want to instantly delete a post.

2. Separate the two functions and stop calling delete() from trash() and back trash() from delete().
   • Pros: no API change for plugins.
   • Cons: posts that could have been trashed are deleted instead.

Same applies for attachments and comments.

[nacin]

The issue is partially standardization -- the comment functions use a third option, which is that wp_trash_comment() falls back to wp_delete_comment(), not the other way around. If there is a desire for the wp_delete_* functions to work strictly how they previously did, this is better than option 2. But I think I'm leaning toward 1 over 3 so plugins are exposed to Trash, even though I argued for the opposite.

I had been working with the comments admin recently, in a plugin and some Trac tickets, so I had been used to option 3 and strict back compat. But after looking through how wp_delete_post and wp_delete_attachment are called in core, I'm leaning toward option 1. azaozz had me nearly convinced as it is.

This would require us to only really change the comment functions and admin.

[caesarsgrunt]

I think all the wp_delete_* functions should call wp_trash_* if the item isn't already trashed - for back compat. Trash is the default now, and plugins which used ot delete should now trash. Unless there is a specific reason (in which case the force arg can be used), nothing should be deleted directly unless trash is disabled.

[azaozz]

Replying to caesarsgrunt:

I think all the wp_delete_* functions should call wp_trash_* ...

Then we don't really need all the wp_trash_*, just need to merge them into the wp_delete_* or perhaps can keep them as wrappers. No point in having two different functions do exactly the same thing and cross-call each other.

In any case we will have to revisit this in 3.0 to make it compatible with custom post types. Seems we can have one general function that would delete any post and few wrappers for back-compat, including a private function to handle attachments probably added by a filter/action.

[Denis-de-Bernardy]

Replying to caesarsgrunt:

I think all the wp_delete_* functions should call wp_trash_* if the item isn't already trashed - for back compat.

agreeing 100%

[scribu]

I vote for no. 2:

Replying to azaozz:

2. Separate the two functions and stop calling delete() from trash() and back trash() from delete().
   • Pros: no API change for plugins.
   • Cons: posts that could have been trashed are deleted instead.

Plugins that use wp_delete_*() will probably have AYS warnings. It's not ideal, but it's better than unpredicted behaviour.

[nacin]

This should probably be a dev chat agenda item?

[nacin]

I take back some previous comments. wp_delete_comment() does call wp_trash_comment() -- in fact, there is no $force_delete override if trash is enabled.

	if (wp_get_comment_status($comment_id) != 'trash' && wp_get_comment_status($comment_id) != 'spam' && EMPTY_TRASH_DAYS > 0)
		return wp_trash_comment($comment_id);

Patch attached to continue the discussion. This reduces the wp_trash_(post|comment) functions to wrappers for the wp_delete_* functions. wp_delete_* will trash the item if the situation warrants. wp_trash_attachment will be handled in #11455.

The only issue I see is this. I understand that calling wp_delete_* on a trashed item should delete it. But should calling wp_trash_* on a trashed item delete it as well? Perhaps a check couldn't hurt. Or, we treat these as strict wrappers and don't do a check, such that calling wp_trash_* on a trashed item will permanently delete the item. (Until now, wp_trash_* did not delete an already trashed item.)

[caesarsgrunt]

I don't like the solution in the ticket at all. I see no reason to merge the wp_delete_* functions and the wp_trash_* functions, and think they are much better kept separate (or alternatively, merge all of wp_delete_*, wp_trash_*, and wp_spam_comment into the wp_set_*_status function).

Whether the wp_delete_* functions should call wp_trash_* functions is another matter. I can see arguments on both sides, and having thought the matter over I don't mind much.
Can we have some examples of plugins which might be affected by this?

[Denis-de-Bernardy]

based on:

​http://plugins.trac.wordpress.org/search?q=delete&noquickjump=1&ticket=on&changeset=on&wiki=on

... no plugins whatsoever in the plugin repository would be affected by either change.

[Denis-de-Bernardy]

I'd like to suggest we punt and revisit the whole thing in 3.0, though. I'm extremely suspicious at the idea of introducing API changes in an RC.

[caesarsgrunt]

Replying to Denis-de-Bernardy:

​http://plugins.trac.wordpress.org/search?q=delete&noquickjump=1&ticket=on&changeset=on&wiki=on

Or even ​http://plugins.trac.wordpress.org/search?q=wp_delete_&noquickjump=1&ticket=on&changeset=on&wiki=on.

... no plugins whatsoever in the plugin repository would be affected by either change.

So... this isn't very important! Based on that, I don't care whether the wp_delete_* functions call the wp_trash_* functions.

But I am against the merging of wp_delete_* and wp_trash_*, as mentioned above, unless all of wp_delete_*, wp_trash_*, and wp_spam_comment into the wp_set_*_status functions.

I'd like to suggest we punt and revisit the whole thing in 3.0, though. I'm extremely suspicious at the idea of introducing API changes in an RC.

I agree. Setting to 3.0, unless someone else wants to change it back.

[nacin]

Should be handled in 2.9 or set as wontfix and addressed separately in 3.0.

I brought this up for back compat and forward-thinking reasons. The new 2.9 implementation is inconsistent among the different wp_delete_* functions that were modified, and also changes the 2.8 implementation.

If we're fine with the change from 2.8 to 2.9, that wp_delete_* now trashes by default, and I think we are, then they need to be standardized. More or less, that means wp_delete_comment needs a $force_delete parameter and some core calls need to be modified to reflect that.

I submitted the patch as one possible fix only because I agree with azaozz that it is redundant to maintain both. But we can leave that for 3.0 as long as we make them all work the same across post, attachment, and comment.

[johnl1479]

This should be addressed in 2.9 since the Trash was introduced in 2.9.

I understand the arguments against changing the API during an RC, but as nacin intially pointed out, this causes undesired side-effects with existing code.

[nacin]

Patch for wp_delete_comment() to function the same way as wp_delete_post() and wp_delete_attachment(). Also adds some phpdoc to clarify how exactly the wp_delete_* functions work.

As azaozz said, these functions will likely be redone in 3.0 into a single function (with wrappers) so they can handle custom post types. Them behaving consistently is an expectation, and would also generate less headaches when we abstract them.

Unless there is a specific reason for wp_delete_comment to not work the same way as wp_delete_post, then this should be a commit. There's nothing drastic here.

[azaozz]

Agree with westi, too close to release to fix this now, moving to early in 3.0.

[nacin]

First problem in the wild: #11534.

[sirzooro]

#11534 has been closed as a duplicate of this one, so I am adding my patch here again, to make sure you will take it into account.

[sirzooro]

Force delete for action=delete links

[nacin]

Refocusing the ticket on the second patch from me -- ​11470-standardizes-wp_trash_comment.diff. sirzooro's patch should be committed as well -- an action=delete should force-delete. (Perhaps we should spin that back off into #11534.)

[hakre]

Patch looks OK in my eyes. changes in wp-includes/comment.php could benefit a bit from properly following the code-guidelines (add some spaces there). That's all I've seen so far.

[nacin]

(In [13995]) Add $force_delete to wp_delete_comment(). see #12766, see #11470.