refactor comment validate/save logic
hunmonk - November 16, 2008 - 17:16
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | comment.module |
| Category: | task |
| Priority: | normal |
| Assigned: | hunmonk |
| Status: | closed |
Description
it's time to bring comment module out of the dark ages here. attached patch refactors the comment validate/save functions to be more in line with node validation/saving. most importantly, it puts form-specific validation/submission logic in the form validate/submit handlers, and makes comment_validate() and comment_save() truely programatic functions.
one other change of note is that values for comment_validate() and comment_save() are passed by reference, which is especially nice for comment_save(), and you get the entire adjusted $edit array to work with, and not just the new $cid.
| Attachment | Size |
|---|---|
| comment_save_refactor.patch | 16.75 KB |
| Testbed results | ||
|---|---|---|
| comment_save_refactor.patch | failed | Failed: 7123 passes, 80 fails, 47 exceptions Detailed results |
#1
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.
#2
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.
#3
The last submitted patch failed testing.
#4
i'd prefer to do this incrementally. it truely sucks that comment module's validation and save functionality is this far behind, so let's take this first step to make it more usable, then we can continue to improve more in subsequent fixes.
#5
dangit! the bot posted while i was writing mine up. setting back to the right status.
#6
re-rolled for latest HEAD. this one passes all the comment tests on my local install, so let's give 'er a whirl...
#7
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.
#8
re-rolled w/ the CS fix in #7
#10
The last submitted patch failed testing.
#11
re-rolled to keep up with HEAD
#12
Looks fine to me.
#13
Yes it is good
#14
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:
<?phpnode_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.
#15
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 ;)
#16
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.
#17
thanks for everybody's thoughtful feedback so far!
this patch should address the outstanding issues from webchick's review:
#18
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.
#19
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.
#20
The last submitted patch failed testing.
#21
looks like the last commit to comment module included a whitespace change, which caused my patch to fail testing. here's a re-roll.
while i was re-rolling, i decided to go ahead and fix the issues in #18 as well -- other than that, everything else is unchanged.
#22
Read it over and it all makes sense. Thanks for showing comment module some love.
#23
Great stuff. Committed to HEAD! :)
These API changes need to be documented in the 6.x to 7.x upgrade guide
#24
tagging
#25
upgrade documentation written.
#26
Automatically closed -- issue fixed for 2 weeks with no activity.