(Un)publishing several comments doesn't work

Hi arhak, thanks for reporting this!

I was able to reproduce this, although it's actually the first query that gets executed. Starting at the second iteration, the conditions are added to the ones already there, resulting in a queries like

UPDATE comment SET status = published WHERE cid = 1 and cid = 2 and cid = 3 and …

I've spent some time looking into this, but haven't found a clean and simple way to alter or remove existing query conditions without resorting to hook_query_alter, which in this case would probably not be the best approach. In the end I've refactored comment_admin_overview_submit so that a new query object is generated for each individual comment operation. Probably not the best approach from a performance point of view, all suggestions to improve this are welcome.

Since obviously publishing/unpublishing multiple comments is currently not tested, I've added a test case too.

Patch attached.

I think this is critical. Things don't break horribly, but being forced to publish/unpublish comments one by one is a UX WTF...

Updated patch.

Please re-roll with cvs diff -up - it's hard to see where changes are being made without that.

Also it ought to be possible to change this to WHERE cid IN ($cids) - but building in array in the foreach(), then adding the condition afterwards.

Without checking, I think node mass operations does a node_save() on each node - this is to ensure anything which needs to act on publishing status changes (hooks, triggers etc.) gets fired. Comment mass updates should really do the same thing - it'd be slower but more correct.

Yes - the node admin page uses callbacks in the submit handler's iteration, so doesn't suffer from the same problem. It looks like the comment admin screen was a straight conversion to DB:TNG without taking the peculiarities of query objects into account.

Attached is the same patch as #3, but with more context to make reviewing easier. I'd prefer to keep the logic (ie perform all actions on one comment, then move to the next comment) instead of using an array in the condition to first update the database for all comments and then handle the 'collateral' operations one-by-one.

This should be $comment = comment_load($cid); $comment->status = 0; comment_save($comment);

Not using the API functions to update comments is in itself a critical bug.

Talked to catch in IRC, and agreed that the comment admin interface should use the existing comment api (comment_save and comment_delete_multiple) instead of doing the database and related updates itself.

Patch attached that:

• Rips out comment_operations(), which serves little purpose if we can't re-use the database objects returned by it anyway.
• Refactored comment_admin_overview and comment_admin_overview_submit now that comment_operations is gone
• Extended the tests so multiple approval and multiple deletion through the interface is now covered.

status...

comment_save() already calls cache_clear_all(), so it shouldn't be necessary to call it again. Otherwise this looks good.

Moved cache_clear_all to just after comment_delete_multiple. (As an aside, shouldn't comment_delete_multiple end with a cache_clear_all too?)

No, it shouldn't. Thats the callers job. API functions do not clear or cache or set messages.

This is fine now.

This assumes comment_save() suceeds. Perhaps a follow-up issue could add some error handling (compare #704646: node_save() fails silently).

Well the problem with node_save() was an transaction / exception was added, without it actually doing anything useful when the exception was thrown, we didn't do that for comments yet. Putting it in a transaction might well be worth doing, but definitely a followup issue.

Not sure that's correct: http://api.drupal.org/api/function/comment_save/7

Oh wow, when did that happen! Opened #721378: comment_save() fails silently.

The only thing that looks a bit funny to me is we're doing === instead of == in places like:

if ($operation === 'unpublish') {

Is there a reason?

Also, what's up with removing comment_operations()?

The '===''s are unnecessary in this case, re-rolled without them.

comment_operations was removed since IMHO it serves little purpose: the database objects it returns can be used only once if criteria are added, which caused this issue (looping while re-using the object and adding criteria). Additionally, we should be using the comment API functions (comment_save, comment_multiple_delete) instead of directly executing the database statements. Since it's tailored specifically for the comment administration page ("Offer different update operations depending on which comment administration page is being viewed."), I don't think this will be an issue for contrib...

Let's see if that makes for a happy webchick :)

code looks good to me.

Marked #352895: comment_operations bypasses comment_save as duplicate.

@mr.baileys take a look at #729628: Checkbox "select all" is broken on node and comment listing tables in /admin marked as duplicate.

I take a different approach - just to clear conditions in that loop, also changed a test.

I think this patch is better because using comment_save() which triggers hook_comment_presave() but a bit slowly.

My +1 to commit

I'll leave this patch for webchick to review as she provided a number of reviews earlier on.

Thanks, Dries. Looks good here! Thanks for the vastly expanded test coverage!

Committed to HEAD.