WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 3 years ago

#11470 closed defect (bug) (fixed)

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

Reported by: nacin Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9
Component: Trash Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (3)

11470-trash-functions-become-wrappers.diff (6.7 KB) - added by nacin 6 years ago.
11470-standardizes-wp_trash_comment.diff (6.9 KB) - added by nacin 6 years ago.
trash-patch.diff (2.6 KB) - added by sirzooro 6 years ago.
Force delete for action=delete links

Download all attachments as: .zip

Change History (33)

comment:1 @westi6 years ago

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.

comment:2 follow-up: @azaozz6 years ago

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.
  1. 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.

comment:3 @nacin6 years ago

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.

comment:4 @bi0xid6 years ago

  • Cc raven@… added

comment:5 follow-ups: @caesarsgrunt6 years ago

  • Cc caesar@… added

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.

comment:6 in reply to: ↑ 5 @azaozz6 years ago

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.

comment:7 in reply to: ↑ 5 @Denis-de-Bernardy6 years ago

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%

comment:8 in reply to: ↑ 2 @scribu6 years ago

I vote for no. 2:

Replying to azaozz:

  1. 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.

comment:9 @nacin6 years ago

This should probably be a dev chat agenda item?

comment:10 @nacin6 years ago

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.)

comment:11 @caesarsgrunt6 years ago

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?

comment:12 follow-up: @Denis-de-Bernardy6 years ago

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.

comment:13 @Denis-de-Bernardy6 years ago

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.

comment:14 in reply to: ↑ 12 @caesarsgrunt6 years ago

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.

comment:15 @caesarsgrunt6 years ago

  • Milestone changed from 2.9 to 3.0

comment:16 @nacin6 years ago

  • Milestone changed from 3.0 to 2.9

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.

comment:17 @johnl14796 years ago

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.

comment:18 @nacin6 years ago

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.

comment:19 @azaozz6 years ago

  • Keywords early added; dev-feedback removed

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

comment:20 @azaozz6 years ago

  • Milestone changed from 2.9 to 3.0

comment:21 @nacin6 years ago

First problem in the wild: #11534.

comment:22 @sirzooro6 years ago

  • Cc sirzooro added

comment:23 @sirzooro6 years ago

#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.

@sirzooro6 years ago

Force delete for action=delete links

comment:24 @hakre6 years ago

  • Keywords has-patch added

comment:25 @nacin6 years ago

  • Summary changed from Don't have wp_delete_post and wp_delete_attachment trash the item by default to Have wp_delete_comment trash/delete the same way as its post and attachment counterparts

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.)

comment:26 @hakre6 years ago

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.

comment:27 @nacin5 years ago

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

comment:28 @nacin5 years ago

  • Milestone changed from 3.0 to 3.1

comment:29 @nacin5 years ago

  • Keywords early removed
  • Milestone changed from Awaiting Triage to Future Release

comment:30 @nacin3 years ago

  • Milestone changed from Future Release to 3.0
  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.