If you publish a comment through the comments approval queue page at /admin/content/comment/approval, you end up with a WSOD. This is because the implementation of hook_comment_publish() in this module always assumes it's receiving an array as an argument. The bulk operation actually passes in an object.

The simple fix in this patch is to cast the argument, $form_values, to an array before looking for the nid key.

CommentFileSizeAuthor
search_comment_publish_cast_array.patch787 bytesrszrama

Comments

rszrama’s picture

rszrama commented

It strikes me that an alternate approach would be to cast the object to an array in comment.admin.inc when it's passed as an argument into comment_invoke_comment().

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs work » Needs review

Re-submitting for testing. Bot stuffs up on that test sometimes, for some reason.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Also, hm. On a quick read, the changed line makes absolutely no sense, since search_touch_node() takes a $nid. I realize it's order of operations that's saving us here (first it's casting to an array, then it's returning the 'nid' index) but I'm wondering if we could do the same fix in a more legible way. Otherwise it seems likely to be something someone's going to "fix" later on.

I think I like the idea of fixing this in comment module better, since it's the one passing around the silly input.

Also, the fact that this bug exists also tells me that our testing framework is lacking tests in this area. Ryan, how would YOU like to be the proud author of some comment bulk operations tests?! :D

rszrama’s picture

rszrama commented
Status: Needs review » Needs work

Sounds like a plan. ; )

I don't like the fact that hook_comment_publish() specifies sending in an array of form values. The fact that search_comment_publish() names its argument $form_values is misleading, since for the bulk operations its not actually sending in fields from a form submission. In my mind, it would be better for these comment hooks to always pass in a comment object, even if it just means converting an array of form values to an object in the comment module. Does that sound reasonable? I've been bitten before by the fact that sometimes the same hooks and ops send an array and other times an object.

I'll see what I can do about the tests as well... that's gonna take a bit of re-learning, since I haven't been able to touch it sine Szeged. : P

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

bump. Can anyone confirm that this still needs to be fixed in Drupal 7?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue summary: View changes
Status: Needs work » Closed (cannot reproduce)

This was filed as a D7 issue, but it seems to have been fixed/changed in D7 a long time ago:

/**
 * Implements hook_comment_publish().
 */
function search_comment_publish($comment) {
  // Reindex the node when comments are published.
  search_touch_node($comment->nid);
}