Untrashed comments could be deleted by wp_scheduled_delete()

[nacin]

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

Working on a quick patch for wp_scheduled_delete() now.

[nacin]

This patch prevents wp_scheduled_delete() from deleting a comment with trash meta that isn't currently in the trash. Provides coverage for posts as well.

There should also be some scheduled cleanup to remove trash meta keys for items not in the trash.

[azaozz]

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.

[nacin]

Garbage collector added to wp_scheduled_delete()

[nacin]

Took a slightly different route, patch attached.

[azaozz]

Replying to nacin:

Took a slightly different route, patch attached.

Doing this directly with SQL would bypass the API and all hooks (although it may be acceptable in this case). Going through the API may be slower but would let plugins run too.

[caesarsgrunt]

Doing this stops plugins from being able to use wp_scheduled_delete() for other status such as spam (could be a common use case).

Better would be to make wp_set_comment_status() check whether the comment is spam or trash before setting the status, and if so run wp_untrash_comment() or wp_unspam_comment() first.

[caesarsgrunt]

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.

Sorry if that's not a good explanation... I'll upload a patch later.

[nacin]

I'm thinking we tie this into wp_transition_comment_status(). We could possibly even add a default action to handle it:

Either in wp_transition_comment_status() or tied to the transition_comment_status hook:

		if ( 'trash' == $old_status )
			wp_untrash_comment( $comment );
		if ( 'spam' == $old_status )
			wp_unspam_comment( $comment );

[nacin]

Never mind, that doesn't make sense. Too late in the process.

In wp_set_comment_status(), before the $wpdb->update:

	if ( 'trash' == $comment_old->comment_approved && $status != 'trash' )
		wp_untrash_comment( $comment_id );
	elseif ( 'spam' == $comment_old->comment_approved && $status != 'spam' )
		wp_unspam_comment( $comment_id );

I think that takes care of this. I can't think of a case why the extra parameter is necessary, because otherwise the situation this patch would prevent can occur.

[caesarsgrunt]

No, not really necessary; just an alternative way of going about it. But probably what you've just described, which is what I said in my first comment, is best. Just ignore my second comment...

[nacin]

Gah, that puts it into a redirect loop when wp_un(spam|trash)_comment is called.

[caesarsgrunt]

:-o

Oh dear. Time to start thinking again...

[caesarsgrunt]

I think 11442.4.diff fixes the problem satisfactorily.

This could, of course, be taken one step further by merging wp_trash_comment, wp_untrash_comment, wp_spam_comment, and wp_unspam_comment into wp_set_comment_status altogether.

[caesarsgrunt]

11442.5.diff takes the extra step mentioned in my previous comment, by merging wp_trash_comment, wp_untrash_comment, wp_spam_comment, and wp_unspam_comment into wp_set_comment_status.

[nacin]

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

[scribu]

Replying to nacin:

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

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

Absolutely.

[caesarsgrunt]

Replying to nacin:

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

No, spam is never automatically deleted at any point.

[caesarsgrunt]

We could make all the changes in patch 5, but keep the wp_(action)_comment() functions just as wrappers - eg

function wp_trash_comment($id) {
    return wp_set_comment_status($id, 'trash');
}

but I don't think it's really necessary. After all, why should trash and spam have separate functions when no other status does?

[nacin]

Replying to caesarsgrunt:

Replying to nacin:

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

[caesarsgrunt]

Replying to nacin:

But they're still simple, easy to use, and named well.

So is wp_set_comment_status($id, 'trash'). But I don't care much either way either...

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

Oh. Well, this doesn't happen for any other statuses, but it would be easy enough to make it so that this happened for all statuses.

[azaozz]

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.

[automattor]

(In [12412]) Check post and comment status before emptying trash, fixes #11442

[azaozz]

If refactoring of the comment trash/untrash is needed, please open another ticket.