In admin/content/comments page, if the comment list is empty, clicking on update button throws the following php error.

Warning: array_diff(): Argument #1 is not an array in comment_admin_overview_validate() (line 144 of /Users/kirtandas/Sites/drupal/modules/comment/comment.admin.inc).

The patch attached takes care of this.

CommentFileSizeAuthor
#7 913322.patch458 bytesrayasa
#5 913322.patch439 bytesrayasa
comment_php_error.patch852 bytesrayasa

Comments

grendzy’s picture

grendzy commented
Status: Needs review » Reviewed & tested by the community

looks good to me.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Needs work 
+++ modules/comment/comment.admin.inc	2010-09-16 15:23:09.000000000 +0530
@@ -141,7 +141,7 @@ function comment_admin_overview($form, &
-  $form_state['values']['comments'] = array_diff($form_state['values']['comments'], array(0));
+  $form_state['values']['comments'] = is_array($form_state['values']['comments']) ? array_diff($form_state['values']['comments'], array(0)) : array();

This looks very odd (and ugly). I'd like to know

1) why 'comments' is not an array in the first place.

2) why we need array_diff() here at all.

And finally,

3) why we output a form to update comments if there are no comments.

Powered by Dreditor.

rayasa’s picture

rayasa commented

Thank you sun.

1) why 'comments' is not an array in the first place.

form_state['values']['comments'] is set to ' ' when the tableselect is empty unless the property #default_value is defined as array(). The following addition does work unless this isn't the right approach.

function comment_admin_overview($form, &$form_state, $arg) {
. . .
 $form['comments'] = array(
    '#type' => 'tableselect',
    '#header' => $header,
    '#options' => $options,
    '#empty' => t('No comments available.'),
    '#default_value' => array(),                   //  <----------- default value defined
  );
. . .
}
2) why we need array_diff() here at all.

Here, the use of array_diff() is similar to array_filter without a callback. It returns an array of selected options in the tableselect and drops those set to '0' (unchecked). Why not use array_filter? Not sure !!

3) why we output a form to update comments if there are no comments.

This is a question of consistency across the drupal platform. If we are ready to disable update options or disable the form altogether in case there are no items in tableselect for /admin/content, /admin/content/comment, /admin/people and don't know where else then I am for it !

gagarine’s picture

gagarine commented

By curiosity, why adding a '#default_value' => array() is not the right approach?

rayasa’s picture

rayasa commented
Status: Needs work » Needs review
StatusFileSize
new 913322.patch439 bytes

IMO this is the right approach. I looked into the bug more deeply only after the question came up that why comments is not an array and suggested a correction in #3.
Resubmitting the patch with changes as per #3...

Status: Needs review » Needs work

The last submitted patch, 913322.patch, failed testing.

rayasa’s picture

rayasa commented
Status: Needs work » Needs review
StatusFileSize
new 913322.patch458 bytes

retrying...

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

re: 3) So we always output those filter and update forms on node/comment admin pages - even if there is nothing to filter or update...?

joachim’s picture

joachim commented

I don't get a PHP error, I get a drupal warning:

Error message
Select one or more comments to perform the update on.

Which seems reasonable to me.

> re: 3) So we always output those filter and update forms on node/comment admin pages - even if there is nothing to filter or update...?

Indeed, that is silly, but it's maybe less important to fix before release :)

Tor Arne Thune’s picture

Tor Arne Thune commented

#7: 913322.patch queued for re-testing.

droplet’s picture

droplet commented
Status: Needs review » Closed (cannot reproduce)

@joachim & me both have no errors. maybe fixed in somewhere..