$node->comment_count may not be set, depending on comment_maintain_node_statistics variable, comment module is broken

This applied cleanly (to Drupal 7) and fixed the problem I was having.

Thanks for the patch!

At the least, we need an automated test for the bug this fixes. I don't quite fully understand what that is, though... Can anyone provide some steps to reproduce without contrib?

Well, usually these sorts of bugs are the result of contrib not checking the data it passes to the API, and so they should usually be fixed in contrib that way. That's not always the case, though (and here it sounds like this particular line is inconsistent with the rest of the module), so what's some generic code to reproduce? That code would translate into a test case.

The check should be isset($node->comment_count) instead of empty($node->comment_count), the isset() version is safer and won't raise PHP notices if the object property does not exist. All the code everywhere in the Comment module assumes that for every node with comments enabled, this property exist, but see comment_node_insert() (and _comment_update_node_statistics() is quite similar):

/**
 * Implements hook_node_insert().
 */
function comment_node_insert($node) {
  // Allow bulk updates and inserts to temporarily disable the
  // maintenance of the {node_comment_statistics} table.
  if (variable_get('comment_maintain_node_statistics', TRUE)) {
    db_insert('node_comment_statistics')
      ->fields(array(
        'nid' => $node->nid,
        'cid' => 0,
        'last_comment_timestamp' => $node->changed,
        'last_comment_name' => NULL,
        'last_comment_uid' => $node->uid,
        'comment_count' => 0,
      ))
      ->execute();
  }
}

Both comment_node_insert() and _comment_update_node_statistics() functions will wrap this table records insert or update into variable_get('comment_maintain_node_statistics', TRUE). This variable is TRUE per default, but if we set FALSE here, the code may potentially leak PHP warnings or errors everywhere because the rest of the Comment module don't seem to care about: the only two references of this variable are in those two functions.

If we look at the comment_node_load() function:

/**
 * Implements hook_node_load().
 */
function comment_node_load($nodes, $types) {
  $comments_enabled = array();

  // Check if comments are enabled for each node. If comments are disabled,
  // assign values without hitting the database.
  foreach ($nodes as $node) {
    // Store whether comments are enabled for this node.
    if ($node->comment != COMMENT_NODE_HIDDEN) {
      $comments_enabled[] = $node->nid;
    }
    else {
      $node->cid = 0;
      $node->last_comment_timestamp = $node->created;
      $node->last_comment_name = '';
      $node->last_comment_uid = $node->uid;
      $node->comment_count = 0;
    }
  }

  // For nodes with comments enabled, fetch information from the database.
  if (!empty($comments_enabled)) {
    $result = db_query('SELECT nid, cid, last_comment_timestamp, last_comment_name, last_comment_uid, comment_count FROM {node_comment_statistics} WHERE nid IN (:comments_enabled)', array(':comments_enabled' => $comments_enabled));
    foreach ($result as $record) {
      $nodes[$record->nid]->cid = $record->cid;
      $nodes[$record->nid]->last_comment_timestamp = $record->last_comment_timestamp;
      $nodes[$record->nid]->last_comment_name = $record->last_comment_name;
      $nodes[$record->nid]->last_comment_uid = $record->last_comment_uid;
      $nodes[$record->nid]->comment_count = $record->comment_count;
    }
  }
}

It's clear that the code seems to assume every node will have a record in there, which is not true because this table state depends on the comment_maintain_node_statistics variable.

In my case, the variable is set to TRUE, but it seems that the heavy migrate jobs I'm doing are missing this table, I don't know why, so every node is broken.

While my own experience with this PHP warning is probably due to contrib, it will eventually happen with vanilla core as soon as the site admin just set the variable to FALSE, and may continue to happen when he reverts this variable to TRUE and nodes inserted in between have no new comments.

There is two "clean ways" to fix it:

• Change comment_node_load() to fill in void but existing object properties on nodes that don't have their own lines into the node_comment_statistics table. This is not the proper way because it doesn't care about the settings really, but it would work (and may return false negatives in node listings, saying 0 comments where the node could have some).
• Alter the code everywhere to both take into account the comment_maintain_node_statistics, and do smarter tests on node object properties using isset(), because at anytime the data state could be altered either by contrib, either by any other bug in core.

PS: Wrote that because I experienced the bug on D7, so the priority here is to fix the bug on D7 first IMHO (this is not a feature which needs backports, it is a bug report on the actual stable too, plus if the new entity handling OOP API, I don't know if this bug fits with 8.x anymore).

I think I have found *why* this problem happens in #1892970: Updating node comment statistics should use db_merge otherwise $node->comment_count will be undefined where I'm going to fix it with an upgrade function to fix bad data.

there's too many ways to get stuck with broken stats, but we need tests first