After trying to work with this hook - implying it were just working as whatever alternation hook so far but failing to receive any result and also reading the recent comments in http://api.drupal.org/api/drupal/developer--hooks--core.php/function/hoo... I looked closer at comment.module and found some, let's say it gently: inconsistent constructions.

  • Both the 'update' and the 'insert' op fire AFTER the comment has already been written to the database which makes the hook either useless or requires additional, yet duplicate database action by the implementing module (and may, at worst, lead to very inconsistent results in the end).
  • As of the api docs, the 'validate' op implicates that $edit (=$a1) is passed by reference and, thus, may be altered. Actually it is not, as function comment_validate() is defined otherwise.

There is more (such as needlessly re-implementing in function comment_invoke_comment() what module_invoke_all() does pretty perfect), but nothing which has further side effects relevant for this issue. However, all this makes me wonder whether this module has really been revised throughout the major branches. If I'm missing whatever point here, please apologize and pardon me, otherwise this really needs to be cleaned up.

The attached patch does just this. It corrects the illogical effects mentioned above and makes it unnecessary to do some duplicate database operation. It also enables $op = 'validate' to be effective anyhow for those who wish to do more with it than just probably set a warning message.

I am not aware whether there are still plans on a non-security related 6.23 core, but if so, hopefully this will go into it.

CommentFileSizeAuthor
#3 comment_hook_comment-1388082-3.patch3.31 KBdoitDave
comment.hook_.patch3.27 KBdoitDave

Comments

doitDave’s picture

doitDave commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, comment.hook_.patch, failed testing.

doitDave’s picture

doitDave commented
Status: Needs work » Needs review
StatusFileSize
new comment_hook_comment-1388082-3.patch3.31 KB

Ups. Still a bit unfamiliar with patching not-own stuff.

ferrum’s picture

ferrum commented

I would like another modification in order to show the changes in the preview.
In function comment_form_add_preview you have to reset $edit after validation. So change

  drupal_validate_form($form['form_id']['#value'], $form, $form_state);
  if (!form_get_errors()) {

to

  drupal_validate_form($form['form_id']['#value'], $form, $form_state);
  $edit = $form_state['values'];
  if (!form_get_errors()) {
damien tournoud’s picture

damien tournoud commented
Status: Needs review » Closed (works as designed)

This is by design.

Both the 'update' and the 'insert' op fire AFTER the comment has already been written to the database which makes the hook either useless or requires additional, yet duplicate database action by the implementing module (and may, at worst, lead to very inconsistent results in the end).

This is by design. 'update' and 'insert' hooks are always called after the data has been saved. Drupal 7 has a 'presave' hook which doesn't exist in Drupal 6 (and that's most likely not going to change).

As of the api docs, the 'validate' op implicates that $edit (=$a1) is passed by reference and, thus, may be altered. Actually it is not, as function comment_validate() is defined otherwise.

This is a duplicate of #227688: hook_comment('validate') implementations cannot modify form values, which is also marked as by design.