refactor comment validate/save logic

In going through this patch we found all kinds of odd things that look like they've been there since the Drupal 4.5 days. If this passes all the tests ;) it seems like a good clean-up to modernize comment module quite a bit and eliminate some of the mega-frustrating inconsistencies between the comment and node modules.

Looks like a good clean-up. Do we want to go the whole hog and make comment_save take an object? Not necessarily in this patch of course.

Minor CS issue..

+        $taken = $query->where('LOWER(name) = :name', array(':name' => $form_state['values']['name']))
+          ->countQuery()
+          ->execute()
+          ->fetchField();

where should be on the next line.

Patch looks great. Convert $comment/$edit consistently to an object in a follow-up would be great too, as it would help to clean up the comment hooks.

Looks fine to me.

Yes it is good

Er. Guys? In the future, can you please post more substantiative reviews than "looks good"? I have no idea if you actually read the patch in detail or not, so I have to assume you didn't.

-    comment_save($comment);
+    $comment = comment_save($comment);

While it's definitely better to get a full comment object than merely a cid, this differs from node_save(), which doesn't return anything. The value that's passed in by reference is just referenced later on. For example:

  node_save($node);
  if ($node->nid) {
    watchdog('content', '@type: updated %title using Blog API.', array('@type' => $node->type, '%title' => $node->title), WATCHDOG_NOTICE, l(t('view'), "node/$node->nid"));
    return TRUE;
  }

Of course, if you remove the return value then this:

+  $edit = $form_state['values'];
+  $node = node_load($edit['nid']);
+  _comment_form_submit($edit);
+  if (user_access('post comments') && (user_access('administer comments') || $node->comment == COMMENT_NODE_OPEN)) {
+    $comment = comment_save($edit);

has to change to this:

+  $comment = $form_state['values'];
+  $node = node_load($comment['nid']);
+  _comment_form_submit($comment);
+  if (user_access('post comments') && (user_access('administer comments') || $node->comment == COMMENT_NODE_OPEN)) {
+    comment_save($comment);

However, $edit is a more descriptive variable name since we are talking about form values here. I guess what we need is a follow-up patch so that node_save() returns a $node object as well. This'll be a nice DX improvement.

+    $redirect = array('comment/' . $comment['cid'], array(), "comment-" . $comment['cid']);

should be 'comment-' since there's no interpolation.

The changes to comment_validate() confuse me. http://api.drupal.org/api/function/hook_comment_validate/7's example code shows that the array coming in is a set of form values, and that the correct response is to form_set_error() if something's amiss. Yet, we remove all of comment module's native validation, which includes form_set_error()s handling to comment_form_validate(), and only call contrib modules' validation handlers in comment_validate? That doesn't really make any sense. Shouldn't all or nothing of this code be moved out?

Looking at http://api.drupal.org/api/function/node_validate/7, the form_set_error() code remains in the node_validate function. So I would keep it in comment_validate() as well.

While it's definitely better to get a full comment object than merely a cid, this differs from node_save(), which doesn't return anything. The value that's passed in by reference is just referenced later on. For example:

The difference is that $node is an object, while $comment is still an array and we thought that it is better to do the array -> object conversion in a later patch as it would make this quite big. After the conversion, we get by reference for free and it will also help to fix the hook_comment_$op() hell ;)

What Berdir said in regards to comment save.

should be 'comment-' since there's no interpolation.
I don't understand this - comment- is in the patch already?

Leaving needs work for the validate comments.

More in-depth review:

watchdog('content', 'Comment: updated %subject.', array('%subject' => $comment['subject']), WATCHDOG_NOTICE, l(t('view'), 'node/' . $comment['nid'], array('fragment' => 'comment-' . $comment['cid'])));
+  }

This should use the new lovely comment permalinks - comment/$cid#comment-$cid - looks like I missed this one in the patch :(

Same with this one

+    // Add an entry to the watchdog log.
+    watchdog('content', 'Comment: added %subject.', array('%subject' => $comment['subject']), WATCHDOG_NOTICE, l(t('view'), 'node/' . $comment['nid'], array('fragment' => 'comment-' . $comment['cid'])));
+  }

+      // This is a comment with a parent comment, so increase
+      // the part of the thread value at the proper depth.

looks like this wraps at less then 80 chars?

I'm pretty sure both of these come under "I'm just moving code around, not my fault", so don't think they should stop the patch getting in and hence not setting CNW.

subscribe ... lets not nitpick this one too long. the net improvement is huge here, even if we mess up some whitespace and leave an inconsistency utouched.

Read it over and it all makes sense. Thanks for showing comment module some love.

Great stuff. Committed to HEAD! :)

These API changes need to be documented in the 6.x to 7.x upgrade guide

tagging