I think there's a problem in comment.module. Form validation can fail for no reason when replying to certain node types, but there's never a problem when replying to another comment. I've posted what I know here: http://drupal.org/node/63554

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mjolley@buy-hot.com’s picture

I guess I should summarize.

One theory is related to the number of forms on the page. I think it's an invalid reference to the edit buffer, possibly caused by unintended recursion via callbacks. I'm pretty sure that the content of the database is not the issue so I'm thinking it's the stack. This appears to be "read only" corruption in that behavior is predictable, and only the form validator is affected. This bug screams "bad stack frame."

kmillecam’s picture

Version: 4.7.0 » 4.7.2

I'm running 4.7.2 and am still seeing this validation error.

I've confirmed that it only occurs when commenting on a node, not when commenting on another comment.

The error I used to get with 4.7.0 included a phrase about "the comment field is empty". This phrase no longer appears.

The current error reads: "Validation error, please try again. If this error persists, please contact the site administrator."

Kevin

kmillecam’s picture

This is still a problem on my systems.

I've found the code that generates the error but still don't know the cause. Has anyone else made any progress?

From form.inc, line 169:


if (isset($form['#token'])) {
    if ($form_values['form_token'] != md5(session_id() . $form['#token'] . variable_get('drupal_private_key', ''))) {
      // setting this error will cause the form to fail validation
      form_set_error('form_token', t('Validation error, please try again.  If this error persists, please contact the site administrator.'));
    }
  }

Dries’s picture

When looking at the form submission form, check whether there is a hidden token field. Is there is no such field, something is wrong. Can we inspect your form somewhere?

Do you happen to use the backport module or the forms_api_backport.module? If so, try disabling it.

Heine’s picture

This happens with polls. When previewing a comment on a poll the $form['#token'] is for example 'comment3' but $form_values contains values from the poll form upon comment preview:

var_dump($form_values):
array
  'choice' => -1
  'nid' => '3' (length=1)
  'vote' => 'Vote' (length=4)
  'form_id' => 'poll_view_voting' (length=16)
kmillecam’s picture

"Can we inspect your form somewhere?" ... sure http://dev1.webwiseone.com Try to post a comment.

"This happens with polls." ... and, it appears, with other modules that display extra forms on a page (like nodevote).

Heine’s picture

Correct, I think that form_values should be keyed on form_id, anyway not just overwritten.

kmillecam’s picture

This behavior can be reproduced on a clean 4.7.2 install.

1) create a poll with comment capability
2) comment on poll

Kevin

kmillecam’s picture

Some good solid research regarding this problem is documented here: http://drupal.org/node/70509

Heine’s picture

Here's a callstack

1 {main}() D:\drupal_installs\drup47\index.php:0 
2 menu_execute_active_handler() D:\drupal_installs\drup47\index.php:15 
3 call_user_func_array () D:\drupal_installs\drup47\includes\menu.inc:418 
4 comment_reply() D:\drupal_installs\drup47\includes\menu.inc:0 
5 comment_form() D:\drupal_installs\drup47\modules\comment.module:484 
6 drupal_get_form() D:\drupal_installs\drup47\modules\comment.module:1357 
7 form_builder() D:\drupal_installs\drup47\includes\form.inc:116 
8 comment_form_add_preview() D:\drupal_installs\drup47\includes\form.inc:435 
9 node_view() D:\drupal_installs\drup47\modules\comment.module:1403 
10 node_invoke() D:\drupal_installs\drup47\modules\node.module:530 
11 poll_view() D:\drupal_installs\drup47\modules\node.module:297 
12 poll_view_voting() D:\drupal_installs\drup47\modules\poll.module:467 
13 drupal_get_form() D:\drupal_installs\drup47\modules\poll.module:309

The second call to drupal_get_form obliterates earlier $form_values by setting it to array().

kmillecam’s picture

A solution to this problem is posted here: http://drupal.org/node/70509#comment-132162

Heine’s picture

I can no longer preview comments when trying to use:

function drupal_get_form($form_id, &$form, $callback = NULL) {
  global $form_values, $form_submitted, $user, $form_button_counter;
  $form_values = (array)$form_values;

They get submitted directly.

Please keep the discussion in this issue. I don't care much about money, but I want this fixed properly.

carlmcdade’s picture

Try and change your session id . The key may be set already and when you refresh to test the post clears automatically.

webchick’s picture

This appears to be tied to the 'vote on polls' permission.

When I first tried this with just a dummy uid 2 I too wasn't exhibiting this problem. After checking all permissions for authenticated users I was again. I unchecked "vote on polls" and the problem went away.

carlmcdade’s picture

I just did another test on a fresh install of 4.7.2 and preview works fine for both anonymous and authorised users.

carlmcdade’s picture

Yes, adding in vote on polls makes a direct comment without a preview. Another problem I think. Checking on a possible solution.

webchick’s picture

Yes, you won't be able to reproduce this on a fresh install. You need to give anonymous/authenticated users "vote on polls" permissions, which they won't have by defauilt. Then you'll see this behaviour.

Heine’s picture

Chx had the idea to put the contents of node_view(node_load($edit['nid')) already in the comment forms array, for example as type markup, then render that, so you avoid the two nested drupal_get_form calls.

I'll try this route.

Heine’s picture

Title: Problem in reply-to-node execution path versus reply-to-comment » Commenting on a node containing a form leads to validation error

Better suiting title

carlmcdade’s picture

The problem with loading it up that way is that it does not solve the problem of actually needing to call drupal_get_form multiple times for other usages. This is where my checkbox comparisons get overwritten. There has to be a more robust solution.

Heine’s picture

Another approach would be a keyed array: $form_values[$form_id]

1 - this would break a lot
2 - what happens when a form builds another instance of itself while in drupal_get_form?

Heine’s picture

Saving the globals in a static variable proved easy enough.

However, the subsequent rendering of nested forms is not valid html. Attached patch also renders the node as suffix of the form to prevent this from happening.

Note: I've tested most comment scenarios. The patch against 4.7 CVS requires thorough review however.

Heine’s picture

Status: Active » Needs review
FileSize
2.01 KB

And a patch against HEAD.

Heine’s picture

Thanks to chx a more elegant & general patch against HEAD.

carlmcdade’s picture

I think a better solution would be to not allow the poll to validate. This way only the cause is influenced and does not get in the way of proper usage of globals.

$form = form_builder($form_id, $form);
if (!empty($_POST['edit']) && (($_POST['edit']['form_id'] == $form_id) || ($_POST['edit']['form_id'] == $callback))) {
if($_POST['edit']['form_id'] == 'poll_view_voting'){
exit;
}
else{
drupal_validate_form($form_id, $form, $callback);
}

This works without causing a direct post of a comment.

carlmcdade’s picture

scratch that. Only works for anonymous.

carlmcdade’s picture

This has gotten very confusing as there is no consistent behaviour in the forms. When anonymous is previewed it is previewed with each press of the preview button. When logged in the preview button only works once then redirects back to the default form view. This makes it hard to know what is design and what bug.

Also the question arises why does an untouched form.inc work perfectly with comments and polls for anonymous but not authorised users?

Heine’s picture

I think a better solution would be to not allow the poll to validate.

Apparantly poll is not the only module node type with commenting problems.

This has gotten very confusing as there is no consistent behaviour in the forms. When anonymous is previewed it is previewed with each press of the preview button. When logged in the preview button only works once then redirects back to the default form view.

Is this behaviour after the patch in #24?

Heine’s picture

And here's a patch against the 4.7 branch for testing (#24 contains the patch against HEAD)

carlmcdade’s picture

I took sometime to think about the flow of all this. What I came to was that when a user is previewing a comment they do not need to see the poll form. They are in a process that they have already decided upon. They either have voted or if they do want to vote will do so after they make a comment. So the vote form is not needed in the comment preview mode.

changing line 309 in poll.module

if ($_POST['op'] == t('Preview comment')) {
 	 return $_POST['$edit'];
 }
 else{
  return drupal_get_form('poll_view_voting', $form);
 }

I like this because it can easily become a coding convention for other modules that use comment forms.

webchick’s picture

@HiveMinds, that's not an acceptable fix... First, because it breaks default behaviour. But secondly, because this problem will replicate itself anytime a form is displayed in a node.

Heine's solution looks more on target, I'll try testing later today.

chx’s picture

FileSize
1.24 KB

I have modified a very little bit Heine's version so that form_values are available during rendering. This is for HEAD. But I suspect it would apply against 4.7, too...

carlmcdade’s picture

The last patch does work and supresses the error. Which is good for users.

Sadly it did not solve the problem of needing to render groups of checkboxes or rather multiple calls to drupal_get_form. I no longer get errors but you can't call the function twice without it overwritting and only showing the last instance of the call. And it still runs each form through validation which I don't want.

Heine’s picture

Thanks for reviewing the patch.

Sadly it did not solve the problem of needing to render groups of checkboxes or rather multiple calls to drupal_get_form. I no longer get errors but you can't call the function twice without it overwritting and only showing the last instance of the call. And it still runs each form through validation which I don't want.

It was not intended to do so. Unless you can show how this is related to the fapi & comment validation bugs, I suggest you open a new issue and include some examples of the problem.

drumm’s picture

Status: Needs review » Reviewed & tested by the community

Tested on 4.7 while logged in and using a module I wrote (karma) which adds a form to the node.

I have no clue what Hiveminds Magazine is talking about since they haven't provided a proper patch or complete description of the problem they are having.

mjolley@buy-hot.com’s picture

I think he's saying that the underlying cause of the problem is that multiple forms are sharing a single input buffer. This makes sense from what I was seeing. The error being displayed on the screen was correct because the comment buffer was empty -- It had been overwritten.

drumm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

killes@www.drop.org’s picture

also to 4.7

Dries’s picture

-    $output .= node_view(node_load($edit['nid']));
+    $form['#suffix'] = node_view(node_load($edit['nid']));

Looks like a hack to me...

Anonymous’s picture

Status: Fixed » Closed (fixed)
mercmobily’s picture

Status: Closed (fixed) » Active

Hi,

So... anonymous (unverified) closed the issue straight after Dries writing "It looks like a hack to me".

Is this issue *actually* closed? I am struggling with it a _lot_ in Drupal 4.7.2. I run Free Software Magazine, which is a major web site, and I am a little disturbed to see that anonymous closed an issue straight after the "big boss's" complaint.

We would like to enable "votenode", but now I am scared.

StevenPatz’s picture

Status: Active » Postponed (maintainer needs more info)

Issues are closed automatically 2 weeks after being marked fixed.

dww’s picture

Version: 4.7.2 » 6.x-dev
Status: Postponed (maintainer needs more info) » Active

Dries is totally right in #39 -- this is a hack, and this is now causing much pain and grief for issue followups as comments.

comment_form_add_preview() is trying to be sneaky about displaying the original node at the bottom of the preview form when you're replying to the original post (instead of replying to a specific comment), with the following code:

  $form['#suffix'] = node_view($node);

The idea is that if the original $node itself contains a form, you can't have nested <form> HTML, so we stick the original node in the #suffix so that it comes after the </form> of our preview form. The problem is that $form['#suffix'] is already set to something at this point in many cases. For example, in my testing, printing out the value of the form's #suffix at that point yielded: "</div></div></div>". Needless to say, once this was clobbered, all sorts of CSS went haywire, blocks were showing up at the bottom of the form, etc, etc.

Core should never be destroying form values like this. If we could assume that $form['#suffix']was itself an array that we could non-destructively append our own stuff to, that'd be one thing, but that's not how #suffix works. :(

Furthermore, in the project_issue case, it'd be nice to undo this behavior entirely, since our previewing logic is already printing the original node. I've tried various approaches and none of them are fully working in both cases yet. I've got to run right now, but I promised hunmonk I'd write up my findings before I left so that we don't duplicate effort. ;) Point being, this is definitely still broken, but I think I'm fairly close to a solution.

Worst case is we check if (!empty(#suffix)) and append instead of clobber in that case. project_issue would still have trouble trying to unset this node_view(), but at least we wouldn't be destroying form values anymore.

Ideally, #suffix could be an array, and comment_form_add_preview() would put the node_view() in a well-known location in that array, so that other modules could use #after_build to unset it if they wanted...

hunmonk’s picture

Status: Active » Needs review
FileSize
853 bytes

simple implementation of our fallback strategy: not clobbering existing #suffix values. i'm not seeing a better solution here so far. AFAIK we don't have the complex data structures in place to solve this more elegantly -- #suffix is the $node->body of drupal 4.6 days.

this fix would be enough to solve our project_issue challenges for issue followups -- we can just slap on the comments after the node output in #pre_render. not pretty, but workable for now until we have a better rendering mechanism for comments.

dww’s picture

Version: 6.x-dev » 5.x-dev
Status: Needs review » Reviewed & tested by the community

That patch is RTBC for D5, but doesn't apply to D6.

dww’s picture

Version: 5.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Needs review
FileSize
678 bytes

Here's a version for D6.

chx’s picture

Version: 6.x-dev » 5.x-dev
Status: Needs review » Patch (to be ported)

Fix D5. Let's contemplate D6 a bit more.

dww’s picture

Status: Patch (to be ported) » Reviewed & tested by the community

I believe chx meant RTBC. ;)

dww’s picture

Just to be clear: I tested the two worst possible cases: previewing a comment on a poll, and the IFAC issue followup preview. Both actually work with this patch. Of course, a simple comment on a regular, non-form post also works. Just wanted to let everyone know this isn't a blind RTBC...

drumm’s picture

Version: 5.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Needs review

Committed to 5.x

sun’s picture

FileSize
679 bytes

Switched empty() to !empty(), didn't test it.

hunmonk’s picture

@sun: not sure what that buys us, and i think not using not is less confusing... ;)

sun’s picture

Sorry, I just thought it would be more consistent with the rest of core. IMHO, we should try to (somehow) protect core committers from having to remark things like that.

hunmonk’s picture

Status: Needs review » Needs work

if we're contemplating a different solution, then this probably needs work.

chx’s picture

Status: Needs work » Reviewed & tested by the community

*sniff* so many things are possible here, but none of them are elegant. comment module needs a rewrite... i think i have my task for seven.

hunmonk’s picture

for code consistency (with the patch that was applied to D5), we should probably use the patch in #46.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Indeed, committed #46 for consistency. Thanks.

Anonymous’s picture

Status: Fixed » Closed (fixed)