By kmillecam on

I keep running into this bug that throws a big red validation error whenever my users try to post a comment. It appears to be a known bug but it's not getting much attention.

If you can be the first to fix it, I'll pay you US$100 ... PayPal, check, whatever works.

Here are the details:

issue: http://drupal.org/node/63990
discussion: http://drupal.org/node/63554

Thanks,
Kevin Millecam

⋅ Categories: Drupal 4.7.x

Comments

coreyp_1’s picture

coreyp_1 commented

What other modules do you have installed?

- Corey

kmillecam’s picture

kmillecam commented

Here's a test site: http://dev1.webwiseone.com Feel free to set up an account and poke around.
I can give you admin rights and enable the devel module if it helps.

I'm currently running these contributed modules:
affiliates
glossary
gsitemap
invite
nodevote
notify
og
print
userpoints
xtracker

I'm having the same problem on the production site (http://juice.altiris.com) and am running these contributed modules:
glossary
gsitemap
nodevote
notify
og
print
xtracker

Kevin

http://www.webwiseone.com
It's all about community.

https://bkjdigital.com
We make magic.

carlmcdade’s picture

carlmcdade commented

I am not sure if this is a bug squasher but it does solve a coding error where the check for a form value should not be based on the existence of a variable. But also the contents of that variable.

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.'));
    }
  }

should be

if (isset($form['#token']) && !empty($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.'));
    }
  }

Hiveminds Magazine
http://www.hiveminds.co.uk
for web publishers and community builders

kmillecam’s picture

kmillecam commented

Thanks for taking a look Hiveminds.

The code is now more robust but unfortunately, I'm still getting the error :-(

http://www.webwiseone.com
It's all about community.

https://bkjdigital.com
We make magic.

heine’s picture

heine commented

The problem is that $form_values is overwritten by another form (eg. the poll); that is why the token check fails. Not sure how to solve in form.inc, but one could modify the module (eg poll) to not display the form on comment preview.

See the issue: http://drupal.org/node/63990
--
The Manual | Troubleshooting FAQ | Tips for posting | Make Backups! | Consider creating a Test site.

kmillecam’s picture

kmillecam commented

This makes sense since it appears to be reported in cases when there are other forms on the page like a poll or nodevote.

Still looking for a solution.

http://www.webwiseone.com
It's all about community.

https://bkjdigital.com
We make magic.

kmillecam’s picture

kmillecam commented

I've changed permissions on the site so guests can leave comments (in case you don't want to set up an account to troubleshoot).

Kevin

http://www.webwiseone.com
It's all about community.

https://bkjdigital.com
We make magic.

carlmcdade’s picture

carlmcdade commented

I just checked a clean install on window iis and apache and the error does not occur. So it must be in one of your contrib modules. Note them and then disable them one by one until the erro disappears.

Hiveminds Magazine
http://www.hiveminds.co.uk
for web publishers and community builders

kmillecam’s picture

kmillecam commented

Hiveminds,

Enable a poll, with comment capability, and try to leave a comment on the poll.

http://www.webwiseone.com
It's all about community.

https://bkjdigital.com
We make magic.

carlmcdade’s picture

carlmcdade commented

I am going to dig a bit deeper into this because this is the exact problem I am having with checkboxes in 4.7 the values just disappear or only one shows.

Hiveminds Magazine
http://www.hiveminds.co.uk
for web publishers and community builders

heine’s picture

heine commented

Here's the callstack when previewing a comment:

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

Step 13 overwrites the $form_values, causing the token check to fail (that happens later just after the form_builder call in 7)

--
The Manual | Troubleshooting FAQ | Tips for posting | Make Backups! | Consider creating a Test site.

heine’s picture

heine commented

A workaround for polls is to adapt comment.module, replace line 1403 with

   $node = node_load($edit['nid']);
    if($node->type != 'poll') {
      $output .= node_view($node);
    }

tags just for display. This needs to be properly fixed in form.inc.
--
The Manual | Troubleshooting FAQ | Tips for posting | Make Backups! | Consider creating a Test site.

kmillecam’s picture

kmillecam commented

Your fix didn't solve my problem (probably because mine isn't poll-specific) but you've earned your way onto the list of finalists for the research you've done.

If I don't get a solution in a couple of days, the cash is yours.

Thanks,
Kevin

http://www.webwiseone.com
It's all about community.

https://bkjdigital.com
We make magic.

carlmcdade’s picture

carlmcdade commented

I just tested this in form.inc line 63:

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

It works a charm. What it does is data type the variable to array whether it is empty or already there rather than reseting it.

Hiveminds Magazine
http://www.hiveminds.co.uk
for web publishers and community builders

kmillecam’s picture

kmillecam commented

Thanks for the help Hiveminds.

I can PayPal you the cash or drop a check in the mail, your choice.

Contact me with the details.

http://www.webwiseone.com
It's all about community.

https://bkjdigital.com
We make magic.

heine’s picture

heine commented

Can you confirm that preview works? If I try this, the comment is immediately submitted instead of previewed.
--
The Manual | Troubleshooting FAQ | Tips for posting | Make Backups! | Consider creating a Test site.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

I can confirm this behaviour... happens only on polls though, not on other node types (page for example)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

responding to an existing comment does not exhibit this behaviour because the node isn't viewed.

carlmcdade’s picture

carlmcdade commented

You will get a direct post if you are the first user. Authorised users get the preview.

Hiveminds Magazine
http://www.hiveminds.co.uk
for web publishers and community builders

heine’s picture

heine commented

I'm posting as a regular user, with 'post comments without approval' permission. Also, preview is preview. Can we move the discussion to the issue?
--
The Manual | Troubleshooting FAQ | Tips for posting | Make Backups! | Consider creating a Test site.

heine’s picture

heine commented

Please test the patch at http://drupal.org/node/63990#comment-110146 (against 4.7 CVS). Though I believe it to be functional, it may need some (or a lot) of work, depending on review.

--
The Manual | Troubleshooting FAQ | Tips for posting | Make Backups! | Consider creating a Test site.

carlmcdade’s picture

carlmcdade commented

This is a better fix for this.

In line 309 of poll.module change to 

if ($_POST['op'] == t('Preview comment')) {
 	$output = '<ol>';
 	foreach ($node->choice as $i => $choice) {
      $output .= '<li>'.check_plain($choice['chtext']).'</li>';
    }
    $output .= '</ol>';
 	 return $_POST['$edit'] . $output;
 }
 else{
  return drupal_get_form('poll_view_voting', $form);
 }

This keeps the vote form from showing. It is not really needed in Preview mode because the user has voted already or will vote after they make comment. They have already decided to make a comment by clicking on the add comment then Preview button. If they wanted to vote then they will just use the main screen as expected.

Hiveminds Magazine
http://www.hiveminds.co.uk
for web publishers and community builders

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

The preview problem will take place on any node that shows a form. Therefore this solution won't work; you would need to do similar hacks in every contrib module that adds a form to a node.

nathandigriz’s picture

nathandigriz commented

Yep.
It is not a fix for the problem as this would require a rewrite of Forms API. It just is not built to be used in the manner that both contrib modules and core use it. But this is a fix that works everytime. Because you have to do it in every module does not take away from the solution. In this case this is the best solution possible with the available code.

Any other solution is going to involve a big change to core that many using 4.7 will not be able to implement or have the patience to wait for.

So the core developers for Forms API can either change the API or the documentation team will have to change the instructions on its usage. Because there are other screnarios where the use of drupal_get_form does not work:

grouping radios in a quiz
grouping checkboxes in a quiz or questionaire.
shoppingcarts with a list of products and a buy button for each.

I have written to my customers that this is not a bug but a design flaw. A redesign and a new release will not be coming tomorrow. This little code snippet let's me satisfy three client complaints. So as they say in the sales business. " Sell what is available today!".

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Since you seem to be able to reproduce this problem in lots of other scenarios, could you test Heine's patch as well and see if it fixes it and comment on the issue with your success/failure?

nathandigriz’s picture

nathandigriz commented

I don't use patches and don't have the means to set up anything to run them. If someone wants to give Heines work in plain PHP then I'll send it off for testing. I would be very interested in seeing if it solves the shoppingcart problem.

heine’s picture

heine commented

I'd like to highlight two bugs, involved in commenting on a node that shows a form (such as poll):

  1. Building a form inside another form leads to loss of FAPI globals (and validation fails).
  2. Comment.module attempts to render a form inside another form (not valid HTML).

1) is solved by creating a stack for FAPI globals.
2) is solved by modifying comment.module to carry the node in #suffix, rendering any form present, after the parent form.

1) means that any form can built any amount of forms while in drupal_get_form as long as it renders them outside eachother (remember, nested forms are not allowed).

2) means that this bug in comment module is solved. There may be modules that still attempt to render nested forms, but I consider that a bug in such a module.

Unless I see some example code of multiple checkboxes/radios I can't really comment on this. It may be due to erroneous use of FAPI (like in comment.module) or another issue alltogether.
--
The Manual | Troubleshooting FAQ | Tips for posting | Make Backups! | Consider creating a Test site.

kmillecam’s picture

kmillecam commented

Thanks for your work Heine, I would like to send you an award for your work also.

Can you send me your contact information?

Thanks,
Kevin

http://www.webwiseone.com
It's all about community.

https://bkjdigital.com
We make magic.

nathandigriz’s picture

nathandigriz commented

I am not that good of a coder but the way it is explained to me is like this when color size and giftwrap are checkboxes. This is just a single page php cart that sits in a node page.

form
-item 1
-color
-size
-giftwrap
/form

form
-item 2
-color
-size
-giftwrap
/form

form
-item 3
-color
-size
-giftwrap
/form

When a form is sent then all items become item 3 or whatever the last form has in it. Even though the button for item 1 is pressed items 3 values show as item 1 and then since item 3 has no checks there is a validation error. It's like all forms are being sent even though only one is being clicked. I'll see if I can get the code to print here.