Off-shoot from: #642702-9: Form validation handlers cannot alter $form structure

comment_form() is entirely incompatible with form caching. ($form_state['cache'])

When form caching is enabled, the constructed $form plus any alterations are cached and is not rebuilt.

When form caching is enabled for comment_form(), and previewing a comment is configured to be required, then a comment form never has a "Submit" button.

No form constructor and no hook_form_alter() implementation should contain any form processing/validation logic.

CommentFileSizeAuthor
#14 drupal.form-cache-all-test.14.patch3.76 KBsun
#8 drupal.form-cache-all-test.8.patch3.93 KBsun
#6 drupal.form-cache-all-test.6.patch1.11 KBsun
#4 drupal.form-cache-all-test.4.patch1.08 KBsun
#2 drupal.form-cache-all-test.patch800 bytessun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

To clarify - code like this in comment_form():

  // Only show save button if preview is optional or if we are in preview mode.
  // We show the save button in preview mode even if there are form errors so that
  // optional form elements (e.g., captcha) can be updated in preview mode.
  if (!form_get_errors() && ((variable_get('comment_preview_'. $node->type, COMMENT_PREVIEW_REQUIRED) == COMMENT_PREVIEW_OPTIONAL) || ($op == t('Preview')) || ($op == t('Save')))) {
    $form['submit'] = array('#type' => 'submit', '#value' => t('Save'), '#weight' => 19);
  }

...belongs into a #process callback.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Needs review
StatusFileSize
new drupal.form-cache-all-test.patch800 bytes

I just want to know how many other forms in Drupal core have this bug. In D7, form caching needs to work for all forms.

And we probably need to update Form API documentation.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal.form-cache-all-test.4.patch1.08 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal.form-cache-all-test.6.patch1.11 KB

ugh. So the installer is incompatible with form caching? I didn't expect failures so early. Let's try with a workaround. Still want to see what else fails.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal.form-cache-all-test.8.patch3.93 KB

Not sure why CommentInterface tests are not failing...

Status: Needs review » Needs work

The last submitted patch failed testing.

moshe weitzman’s picture

moshe weitzman commented

IMO this required Preview is a very obscure feature that can move to contrib. Lets just keep the form consistent during its lifetime. I'd like to see the feature and admin UI clutter gone from core.

dries’s picture

dries commented

Just to be clear: comment previews itself aren't an obscure feature IMO -- especially not for anonymous commenters who can't edit their comments after the comment was published. Required comment previews could be considered obscure and I'd be OK with taking out the option to enable/disable whether previews are required.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

errr. Not sure what all this talk about changing the available comment preview options is about, but that's not the topic of this issue ;)

This is a purely technical issue, because comment form relies on form rebuilding to change the available form buttons in the form constructor. It should not do that. Stuff like that, and also stuff like previews, clearly belongs into #process.

That said, due to #642702: Form validation handlers cannot alter $form structure, comment_form_validate() would now be able to inject a preview and alter #access on buttons during form validation - which is exactly what it wants to do.

fago’s picture

fago
Primary language German
Location Vienna
commented

I don't see why the form shouldn't use rebuilding to inject another button? Why should we misuse the validation handler instead?

>When form caching is enabled, the constructed $form plus any alterations are cached and is not rebuilt.
This doesn't make sense to me. Form-caching doesn't deactivate rebuilding.

The problem is probably an ajax callback updating the cached $form with #access set to FALSE, thus then the submit from this cache fails. If this is what happens, the problem is missing $form_state in the ajax callback, thus it should be fixed with the patch from #641356: Cache more of $form_state when form caching is enabled.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: comment_form() is incompatible with form caching » Some forms are incompatible with form caching
Component: comment.module » forms system
Assigned: Unassigned » sun
Status: Needs work » Needs review
StatusFileSize
new drupal.form-cache-all-test.14.patch3.76 KB

ok, but I'm talking about form caching only. No rebuilds involved.

It seems like comment_form() is compatible with form caching in HEAD, because it also rebuilds the form, and in general, large parts of form building/processing/caching/rebuilding have been revamped.

So let's focus on the remaining failing tests this patch triggered.

Since #647130: Cache only once when rebuilding a form has been committed, I'm curious to see which will remain now.

Status: Needs review » Needs work

The last submitted patch failed testing.

fago’s picture

fago
Primary language German
Location Vienna
commented

I justed tested a comment_form with the patch applied from #641356: Cache more of $form_state when form caching is enabled, manually enabled caching and required previews - I was able to submit the comment without troubles. Any incompatibilities with caching are usually that $form_state is gone which #641356: Cache more of $form_state when form caching is enabled fixes.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Closed (won't fix)

From all the fails that this patch revealed, I don't really see any functionality/form that would suffer from not supporting #ajax. I'm sure there are edge-cases and we will likely see bug reports for those, but for now, I consider this issue as closed.

The comment.test clean-up would be cool to do, but that really doesn't belong in here.