Cached forms make all subsequent forms cached

Reproduced with Poll. Story works fine to me so looks like specific to that module.

To reproduce:

Create a poll.

Try to post a comment against it with the default "require preview" setting - no luck.
Change it to make preview optional.
Try previewing a comment against the node - still no luck
Try saving a comment against the node - this works fine.
Try previewing a reply to your first comment - this work fine whether previews required or not.

So it's only previews of comments against the initial poll node, not replies to comments - but this makes it impossible to comment at all with default settings.

What fun..

Investigate whether there is a collision between the poll form and the comment form. As the above tests indicate (which I reproduced), the comment preview fails if and only if the poll form is displayed on the same page.

The form IDs are distinct and the forms clearly separate, so Form API should not choke on them. I'm wondering if the poll form builder does something weird that could collide with other forms.

Gah, I got what I've heard described as a Heisenbug. Adding var_dump($form_state) to both the poll form and the comment form resolved the problem... wth?

Nope, found it. I was able to get previews working by removing $form['#cache'] = TRUE; in the poll form. This worked for both the vote-canceling form and the voting form itself. However, I don't know why. A Form API guru has to look at this.

Yeah, I also found that form caching is the problem. As the poll form is cached, comment_form() is not called for some reason. On other node types, comment_form() is called. Why does poll forms cache gets to cache the comment form as well is a good question.

While debugging this, I also found a bad check for an $op which was changed quite some time ago. Now comments also have simple "Preview" buttons. I DO NOT mark this as needs review, since my patch does not solve anything, but please include this fix in the later patches.

the form caching was introduced by the AHAH-ification of the module, but I can't figure why it's necessary here. I've tried without the form caching and the multiple voting forms on the same page work fine.

We are on the issue with chx. Basically form_builder() static's the cache flag, so all subsequent forms on the same request are also cached. This needs to be fixed.

Props to Gabor who found the problem is due to the static $cache . Before testing, truncate your cache_form table and only then visit the poll form.

That's a much more generic issue.

Success, I'd say RTBC.

Created poll
Entered comment, clicking preview shows preview correctly.
Changed poll content type comment setting to not require previewing.
Created another poll.
Entered comment, clicking preview shows comment preview correctly.

Replies also still preview correctly.

Great, committed. #8 is also RTBC then for 7.x.
catch: you see, poll module proved as a good regression test suite to have again (see webchick's comment on the issue you linked in) :)

I came to the same conclusion, but there is another bug in that part of the code. At the bottom of form_builder(), we have:

  // If some callback set #cache, we need to flip a static flag so later it
  // can be found.
  if (isset($form['#cache'])) {
    $cache = $form['#cache'];
  }
  // We are on the top form, we can copy back #cache if it's set.
  if (isset($form['#type']) && $form['#type'] == 'form' && isset($cache)) {
    $form['#cache'] = TRUE;
  }

Which means that:

• we set $cache regardless of its previous values (i.e. elements could override each other),
• we set $form['#cache'] = TRUE; if any element set the $cache value to any value.

The following patch takes care of setting the form to non-cacheable if any of the element (nor the form itself) specifically required it to be non-cacheable.

Damien

Sorry for the collision. Here is the same patch rerolled.

This patch won't apply unless the previous one is rolled back.

Just marked #219010: Setting $form['#cache'] = false shouldn't activate caching as a duplicate of this one.

@catch: I rerolled it in #13.

Sorry damz, cross posted.

The patch to commit is http://drupal.org/node/216515#comment-721058 , #cache FALSE shall not do anything -- if there is an AHAH part that asks for caching then any other subsequent requests asking for not caching should be disregarded otherwise said AHAH element will be broken.

chx: I don't understand re. the AHAH part. I have a form which includes an #ahah file upload and require that the form isn't cached so that on submit the $form array is built again so as to include a thumbnail of the file uploaded with the ahah in case the form doesn't pass validation (e.g. title or some other required field being missing).

I have an if() statement in hook_form that returns differently if a photo is uploaded with the form, and this has to be reflected when the form comes back with validation errors

If the form does not pass validation then you get another chance for building a form -- just store something in form_state['storage'] or set form_state['rebuild] and your form create function will be called again. If you use AHAH without #cache TRUE, your code is broken.

This hasn't made it into 7.x yet.

Also there are changes to form.inc, so it no longer applies.

This is a reroll of chx's patch in #8 for Drupal 7. This has been previously reviewed and accepted.

still applies.

Applies with offset, so re-rolled.

I've committed both patches. Next time, please submit a test case! Thanks.

Let's mark this back to active for a test. Commenting on a poll catches this, and isn't currently in poll.test, wouldn't be too much extra to add.

does this need a backport?

No, it was fixed in Drupal 6 in #11, the day of the release of Drupal 6, if I remember correctly.

Marking this back to fixed, and creating a proper tests component issue instead: #296497: TestingParty08: Posting a comment on a poll