Untrashed comments could be deleted by wp_scheduled_delete()

Description

If you trash a comment, then change its comment status (say to approved) without running wp_untrash_comment(), the _wp_trash_meta_time comment meta will still exist.

wp_scheduled_delete() will then delete the comment once the comment meta time is old enough.

This isn't as much of an edge case, nor does it require a direct DB edit or for a plugin to get involved. The comment.php AYS form (when clicking a link in wp_notify_moderator() email) doesn't check what the comment's current status is. Thus, if the comment was already trashed, and it is then approved, wp_untrash_comment() will not be run and the comment meta will still exist.

First solution, prevent the blocker:

Have wp_scheduled_delete() also check whether the comment is currently in trash.

Second solution, prevent the whole situation from occurring, which would be good for 3.0:

Implement #11441: Comment moderation AYS should provide feedback on comment's current status. I hinted to this blocker in that ticket without realizing it was a blocker:

(For example, a trashed comment would need to first be untrashed before being approved. This doesn't happen now, which means meta is not cleaned up, etc.)

The double-check sounds good but instead of selecting only posts/comments that are currently in the trash, perhaps better to select all that are scheduled (as at the moment) and then check if the status == 'trash'. Then you can remove the trash timestamp meta for items that are not trashed.

We could add an argument to wp_untrash_comment() and wp_unspam_comment() which allows one to override the previous status read from the db and set an arbitrary new one. Then wp_set_comment_status() can check the status of the comment, and if it is trash or spam call wp_untrash_comment() or wp_unspam_comment() with this parameter specifying the new status.

Patch 4: I like it, and it looked like what I was considering but I haven't had time to re-patch.

I'm trying to run it through my head for how trashed -> spammed (and vice versa) would work, but this appears to fix the blocker.

Patch 5: I actually liked the simple wp_(action)_comment() functions as an API. They've also been around long enough in beta that removing them might be a compat issue. Combining them I suppose makes sense though.

Do we add back all sorts of pre- and post- actions?

Spam didn't previously have an expiration time; they just got wiped by wp_scheduled_delete() (at least how I understood it).

Patch 5: I actually liked the simple wp_(action)_comment() functions as an API. They've also been around long enough in beta that removing them might be a compat issue. Combining them I suppose makes sense though.

Agree: the wp_(action)_comment() functions should stay, even though they're just wrappers for wp_set_comment_status().

I'm trying to run it through my head for how trashed -> spammed (and vice versa) would work

Shouldn't ever happen...

Patch 5: I actually liked the simple wp_(action)_comment() functions as an API. They've also been around long enough in beta that removing them might be a compat issue. Combining them I suppose makes sense though.

I don't think removing them could be a compat issue. Anyone developing a plugin against a beta release should be prepared to have to make changes. And these are pretty minor changes from an API point of view.

Do we add back all sorts of pre- and post- actions?

I don't understand. What do you mean?

Spam didn't previously have an expiration time

It still doesn't - the time it was spammed gets stored in the DB like trash, because that's just simpler, but it is never automatically deleted.

they just got wiped by wp_scheduled_delete() (at least how I understood it).

I'm trying to run it through my head for how trashed -> spammed (and vice versa) would work

Shouldn't ever happen...

That's the point of the ticket. If a comment is already trashed, a link in a notif email will still take you to the comment SYS form and allow a change in status without untrashing. I'm going to work on a patch later today for that but I anticipated that being 3.0, as it isn't a regression (as long as the blocker is fixed).

Patch 5: I actually liked the simple wp_(action)_comment() functions as an API. They've also been around long enough in beta that removing them might be a compat issue. Combining them I suppose makes sense though.

I don't think removing them could be a compat issue. Anyone developing a plugin against a beta release should be prepared to have to make changes. And these are pretty minor changes from an API point of view.

But they're still simple, easy to use, and named well. I don't care so much either way.

Do we add back all sorts of pre- and post- actions?

I don't understand. What do you mean?

Ex.: The trash_comment hook ran before a comment was trashed, and a trashed_comment hook ran once trashed (if successful).

This is a double-check if the comments/posts have been properly "untrashed" and to prevent deleting them when not. All it needs to do is look at the "status" field and delete the meta timeout if it's not 'trash'.

Don't think we should be removing functions and filters that have been in core for few months, also the posts and the comments trashing API should be similar and easy to read/understand.