hook_comment('validate') implementations cannot modify form values

Aha, I knew I couldn't have been the only one noticing this. I hope this will get an official fix soon.

However, there's an other possible reason for this bug, and thus another possible fix: comment_form_validate() calls comment_validate() but does not store the return value.

Either references or return values must be used in order for the user validation to propagate through the system, but right now neither are used. Which one will get chosen is up to the devs to decide, of course.

Okay, I've tested implementing the reference, and it works.

HOWEVER...

It only works when posting comments. It doesn't work when previewing comments. And here's the reason why (comment.module line 1421):

function comment_form_add_preview($form, &$form_state) {
  global $user;
  $edit = $form_state['values'];
  drupal_set_title(t('Preview comment'));

  $output = '';
  $node = node_load($edit['nid']);

  // Invoke full validation for the form, to protect against cross site
  // request forgeries (CSRF) and setting arbitrary values for fields such as
  // the input format. Preview the comment only when form validation does not
  // set any errors.
  drupal_validate_form($form['form_id']['#value'], $form, $form_state);
  if (!form_get_errors()) {
    _comment_form_submit($edit);
    $comment = (object)$edit;
...

This is the function that creates previews. First, $form_state['values'] is stored in $edit. Then, $form_state is passed to drupal_validate_form as a reference. If no errors occur, processing goes on... using $edit!!! Any changes made during validation are simply thrown out.

Obvious solution: move both the $edit and $node definitions to after the call to drupal_validate_form()

Thanks for all the input, your spend time etc. pp.. But first of all it should be referenced wether that function should validate (in this case, there is no need to change something) or filter (that is the proper verb for changing something) data.

Keep in mind that mixing the concepts of Validation and Filtering leads to serious security Issues which can destroy a webapplication in it's whole.

Therefore I can only strongly suggest that Validation Hooks should never be able to change data passed to them.

Since the main part of this Issue is about changing input data in a validation hook which is plain wrong, it must be closed.

Hmm, if we decide that validate operations should not be able to modify input data, then we can't just close this issue - the documentation at http://api.drupal.org/api/function/hook_comment/6 badly needs changing.

"validate": The user has just finished editing the comment and is trying to preview or submit it. This hook can be used to check or even modify the node. Errors should be set with form_set_error().

I tend to agree that it's odd to modify data in a validate operation, but then which $op should the data be modified in? The docs don't say.

Jeff

I think this is a pure misunderstanding. The Sentence: It states there that it can be used to "modify" the node. is there for giving a hint that you can modify the node data and to tell you the truth, you can. It is inside the Examples of the documentation page:

<?php
function hook_comment($a1, $op) {
  if ($op == 'insert' || $op == 'update') {
    $nid = $a1['nid'];
  }

  cache_clear_all_like(drupal_url(array('id' => $nid)));
}
?>

In this case, the nodes cache is "modified". Take care that "modify" itself stands inside QUOTES.

Since the patch changes a function head and uses bad design this way (passing variables by reference implies a lot so it is adviced to take much care here), I suggest that the ticket is closed until one of the core devs can say wether or not an API change is necessary.

Otherwise I suggest that the original poster provides more information about his/her motivation to apply the patch. Maybe there are better ways to achieve the same.

I will post my reasons for wanting this:

I am using Drupal to run an online roleplaying game. I want people to be able to roll dice in an easy way but without too much obvious ways to cheat.

So, the solution I chose is that the people write [roll]1d6[/roll] in their text, which then automagically turns into a random value between 1 and 6.

However, achieving this is tricky. After all, this random value may only be generated ONCE. It's impossible to play if the value keeps changing every time someone loads that particular node/comment.

The only solution to this that I see is to change the text itself. When someone presses 'Post' or 'Preview' on a text containing a [roll], the text itself must be modified, replacing the [roll] tag with a unique [rollid] tag (basically just a hash of some relevant values). This rollid is stored in the database along with the randomly generated result of the dice roll. Afterwards, the standard text filtering functionality is used to turn the [rollid] tag into a human readable text informing everyone of the roll and result.

But back to the problem at hand: the text of the post that is being made must be changed. It must be changed in such a way that every preview, every edit, every view of the text later on contains the modified version. It must be stored in the database in its modified form.

When it comes to nodes, this is not a problem:

function diceroller_form_alter(&$form, $form_state, $form_id) {
	if (isset($form['type']) && $form['type']['#value'] .'_node_form' == $form_id) {
		$form['#validate'][] = 'diceroller_match_node';
	}
}

function diceroller_match_node($form, &$form_state) {
	$newtext = diceroller_match($form_state['values']['body']);
	$form_state['values']['body'] = $newtext;
}

These simple functions make sure that the text of every node gets sent to the diceroller_match function, which does some regex magic and other stuff and then returns the modified text, which then gets stored in the form again. Because $form_state is passed as a reference, the changes are final and global.

(Do note that this is done using the 'validate' functionality of nodes.)

But when it comes to comments, this simply isn't possible. The comment gets passed down as a copy, not a reference, and you can change all you want in the local values, it won't affect anything.

This has forced me to use the following hack to get the functionality I need:

if (isset($_POST) && isset($_POST['form_id']) && $_POST['form_id'] == 'comment_form') {
	diceroller_match_comment();
}

function diceroller_match_comment() {
	$newtext = diceroller_match($_POST['comment']);
	$_POST['comment']= $newtext;
}

The first piece of code is actually in my index.php, right after the drupal_bootstrap call... The function is in the module file, but it's still one ugly hack, totally bypassing the whole hook system for the very simple reason that the hook does not do what it promises: it does not give me the ability to modify the comment.

Validation should not change the contents of the form in any way visible to the user, because they expect to be able to edit the comment or node and see their text as they typed it. And for security reasons it is necessary to have HTML filtered on the way out of the database, so there is normally no reason to filter it on the way in.

Contradicting this, here are two use cases for changes during validation:

• Validation involves an expensive web service call (for example). Then you may want to have successful validation add a signed hash of the current values as an invisible form value, so future submits can skip the web service step if the values are unchanged.
• You are using the Wysiwyg API and want to allow for users to paste content from Microsoft Word

In the latter case, we have the problem that Word includes several kilobytes of hidden data in the HTML, but this becomes visible because some output filter or other escapes the comment opener that is supposed to hide it. Worse, some versions of Microsoft Word represent lists not as lists but as paragraphs with bullet characters at the start, and we wanted to normalize everything to straightforward HTML. After various unsuccessful attempts with doing it within TinyMCE or as a filter or a submit handler, our current version works by adding a validator that removes the Microsoft Word data islands from the HTML and replaces the appropriate P tags with UL or OL and LI. Note that if this works, the user sees no change (because they edit it in rich-text editor mode). So it does not break the user's expectations.

This works OK in most situations, but fails when previewing comments, because (as mentioned above) the $edit variable is a copy of the form values and is not munged when the comment is validated.

Also, the above change to $edit does not allow changes during validation to be retained in the form in the case when the user has clicked the Preview button and not Post. This is because the preview is added during drupal_rebuild_form, and this uses a fresh copy of the submitted values.

My temporary solution is on $op='update', you can modify directly in the database. A poor design, but if you don't want to break the D6 API...
I don't know if this is changed in D7 (i.e. a $op='presave' like with node).

Edit: just find a new way to modify comment without doing the db_query directly.

function mymodule_form_alter(&$form, $form_state, $form_id) {
  switch ($form_id) {
    case 'comment_form':
      if (empty($form['#submit'])) {
        $form['#submit'] = array('comment_form_submit');
      }
      array_unshift($form['#submit'], 'mymodule_modify_comment');
      break;
  }
}

Then modify your data after it is validated and before the default submit function takes place!

This is the expected behavior. You should use hook_form_alter() to add a proper form validation handler.