comment updates shouldn't touch things that aren't specified

Please add some comments to the code, it wasn't obvious (at least to me) why were you doing this.

I am testing this patch and it seems to work as advertised. Thanks wtanaka!

Hmm you overdid it a bit ;)
I've removed most of the comments.

I've changed it a bit so it fixes this bug: Editing own comment produces error
The problem is that in comment_save() UPDATE uses %s instead of %d for several fields, that are integer type: status, format
Attached patch fixes this by changing '%s' to %d.

I've also changed some of ' to " to avoid \', and some other small fixes.

I've tested this patch and it works as advertised.

There's one problem: when admin (or someone with administer comments right) edits a comment, he can change the date. But the changes are not saved.

It seems that comment_validate() changes $edi['date'] to $edi['timestamp'], but when comment_save() is called, the changes are not there.

I don't know how to fix it.

i'm curious why format is using %s currently since it does seem like it must be an integer.

Because it's a bug ;)

The problem with not setting data is because date is passed in $edi['date'], not 'timestamp'. I've fixed this, but there's another problem ;)
When previewing your edit as someone who can't change the date, the current date is displayed (because _validate() sets the date to current if it's not specified). But when saving the edit the old date (unchanged) is left, i.e. it's not changed - because it's not set by form.

Another problem, I think not related with comment module, is that _validate() is called twice when previewing reply. To reproduce: in forum click edit on your own reply, and then preview message.

Another problem: you can't change the author. The changes are not saved.

I guess that means that the code needs more work? :)

The names of the variables need work too. We don't glue words together. I suggest dropping the 'sql' bit from the variable names (if that doens't make the names conflict), as well as renaming $arrayindex to $index.

The code is getting a bit hairy though. A simple if (user_access('administer comments')) check might be more elegant?

Similar bug http://drupal.org/node/42265 with a bit different fix?

At least as far as timestamp vs. date is concerned, this patch here seems to be fixing a side-effect of the real bug, which is fixed more directly (by adding a single character to the code :-) in http://drupal.org/node/42265

I am not sure if other fields, such as author ever need to be changed or updated, etc. Perhaps there is a deeper issue there as well (at least I think I saw another bug report somehwere about comment authors...)

From the http://drupal.org/node/42265

by the way, comments i've heard from chx led me to believe that validate methods (at least the ones in core) shouldn't be changing form values.

The comment module have

function comment_nodeapi(&$node, $op, $arg = 0) {
  switch ($op) {
[...]
    case 'validate':
      if (!user_access('administer comments')) {
        // Force default for normal users:
        $node->comment = variable_get("comment_$node->type", COMMENT_NODE_READ_WRITE);
      }
      break;

So maybe that solution (changing values in validate) is acceptable?

I've been doing some research. In short the code is a mess ;)

1) When admin adds new comment or reply, comment_save() gets following $edit (besides "preview", "submit" etc):
subject, comment, format, cid, pid, nid, uid

2) When admin edits a comment 3 more fields are set:
author, date, status

3) When anonymous user adds a new comment or reply, $edit is the same as in (1) with following exceptions:
instead of format there is 'Filtered HTML' => 1 [or "'Full HTML' => 3" if that's the default]
cid and uid are set to NULL

4) When anonymous user with administer rights adds a new comment or reply, $edit is the same as in (1) with following exceptions:
instead of format there is 'Filtered HTML' => 1 [unless more then 1 filter]

5) Situation like (3) but more then 1 filter enabled for anonymous: $edit the same as in (1)

Eh. I might have made a mistake somewhere.
I think this can be described shortly as:
a) if there is more then 1 filter format for the user, the format is set, otherwise its name is used. The name can be changed by admin (think localization).
b) anonymous never has set author, date, status, even with 'administer comments' or 'administer nodes' right (haven't checked authenticated user).

Another bug: you can unpublish a comment, but when you try to publish it again (from admin/comment/list/approval) you get "Illegal choice in ."

I think part of the problem is the function comment_validate() modifies certain values that are lost when this function is invoked by comment_form_validate().

I tried the patch posted here:

the patch referred to in http://drupal.org/node/40762#comment-62232 can be found here: http://drupal.org/files/issues/comment_form_validate.patch

But it didn't work because the way comment_form_validate() is invoked by _form_validate() (in form.inc). It uses call_user_func_array($function, $args); where $args is not passed by reference. Therefore it is necessary to declare $form_values as global in #validate kind of functions.

Patch attached.

BTW, changing issue status, just in case it gets missed. ;)

Using a global for this is not advised. Can't we make the argument passing work better?

Not sure what to say. Probably this is something that is not clear in the new forms API.

$form_values is declared as global in several places (_submit and/or _validate functions) ...when it needs to be modified. Note that using the & in the argument declaration of the function doesn't work, for the reason I explained in my previous post.

You can see an example, pretty similar to this one in the contact module:

function contact_mail_page_validate($form_id, &$form) {
  global $form_values;
  if (!$form['cid']) {
    // Look if there is only one category
    $result = db_query('SELECT cid FROM {contact}');
    if (db_num_rows($result) == 1) {
      $category = db_fetch_object($result);
      $form_values['cid'] = $category->cid;
    }
    else {
      form_set_error('category', t('You must select a valid category.'));
    }

    if (!valid_email_address($form['mail'])) {
      form_set_error('mail', t('You must enter a valid e-mail address.'));
    }
  }
}

This code wouldn't be correct either. Note the argument &$form is, in fact, $form_values (passed by value). $form_values is declared here as global because it needs to change an element of the array. It checks for !$form['cid'] and then sets its value via $form_values['cid']. hmmm ...it works, though. So...

Related to http://drupal.org/node/43325

I have found a way to pass $form_values by reference, please see attached file.

If adopted, there would be a bit of work to cleanup all _validate kind of functions that are actually declaring $form_values as global.

No! If there are validate functions that cringe to their old and broken ways, it's not to be cured by allowing it. Validate validates and does not change.

err... there are _validate functions in drupal core that change $form_values elements. We're talking about the comments module here, but there are others such as the user or contact modules.

???

Alright ... is there a workable patch for this issue, or should I sit tight?

dummy post to subscribe to this critical issue. Definitely a blocking RC issue.

Comment editing fully works with http://drupal.org/node/43325.

Just updated my test site to the latest CVS (19th Jan 11:45am GMT) and the comment module still isn't happy. When I try to submit a comment I get an error which reads "You have to specify a valid date." I assume this bug is related to this current thread.