When a form is initially built and $form_state['cache'] is TRUE, the $form structure as well as (certain properties of) $form_state is cached.

Form #process handlers and #validate handlers are then supposed to use $form_state - as the name implies - as state machine for subsequent rebuilds of the form.

They are also supposed to use $form_state['storage'] to store stateful data.

However, after regular form processing, $form_state is never written back to the cache.

The cached $form and $form_state is only ever updated when drupal_rebuild_form() is invoked. That, however, is purposively not invoked in case of a validation error.

CommentFileSizeAuthor
#16 drupal.form-state-cache.14.patch2.54 KBsun
#15 form-state-cache.patch3.62 KBfago
#14 drupal.form-state-cache.14.patch2.54 KBsun
#12 drupal.form-state-cache.12.patch2.83 KBsun
#8 drupal.form-state-cache.8.patch11.38 KBsun
#6 drupal.form-state-cache.6.patch9 KBsun
#5 drupal.form-state-cache.5.patch8.89 KBsun
#3 drupal.form-state-cache.3.patch1.91 KBsun
drupal.form-state-cache.0.patch1.88 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue tags: +D7 Form API challenge

.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal.form-state-cache.3.patch1.91 KB
Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Looks correct. Committed to CVS HEAD. Setting to 'needs work' because this could use a test.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
drupal.form-state-cache.5.patch8.89 KB

Boy.

Somehow, I really doubt that anyone ever really used anything related to form storage and form caching in real applications. Even totally simple assertions fail -- like: verifying that form_build_id stays the same in multi-step scenarios?!?

I can't believe this.

Attached patch

- fixes aforementioned bug (and while doing so, streamlines the caching condition)

- adds tests

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal.form-state-cache.6.patch9 KB

That comment in the test read a little bit wonky. Fixed.

Dries’s picture

Dries CreditAttribution: Dries commented 
+++ modules/simpletest/tests/form.test	27 Nov 2009 20:00:16 -0000
@@ -450,12 +446,38 @@ class FormsFormStorageTestCase extends D
+  function testCachedFormStorageValidation() {
+    // Request the form with 'cache' query parameter.
+    $this->drupalGet('form_test/form-storage', array('query' => array('cache' => 1)));
+    // Fetch form_build_id.
+    $fields = $this->xpath($this->constructFieldXpath('name', 'form_build_id'));
+    $form_build_id = (string) $fields[0]['value'];
+
+    // Skip step 1.
+    $edit = array('title' => 'foo');

In general, this looks good to me, but it feels like the tests could be documented better. It would be beneficial to explain what exactly we're testing, and to outline the approach taken on a high-level. I think that would help people grok things -- I know it would have saved me quite a bit of time. So, if possible, add a two sentence introduction as that would have helped wonders.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal.form-state-cache.8.patch11.38 KB

True. Added extensive docs.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Needs work

Why should form_build_id be the same? If you re-submit some step of a multistep form and the form is gone from cache because you overwrote it what then...?

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Closed (duplicate)

the build_id change is bogus, the tests are moving to #622922: User input keeping during multistep forms. Thanks for the tests.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Closed (duplicate) » Needs work

While the test has been moved to #622922: User input keeping during multistep forms, we still want to do the minor clean-up to the usage of $form_build_id as condition for form_set_cache() from this patch.

I'll re-roll shortly.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal.form-state-cache.12.patch2.83 KB

chx additionally requested to update the inline comments.

Dries’s picture

Dries CreditAttribution: Dries commented

Admittedly, I'm not sure why form ID isn't always set. The comments don't explain what it means for form ID not to be set. I'm happy to commit it with chx' blessing, but for me, it is a bit magical at the moment. It would be nice to have a comment that explains when form ID is and isn't set and why that is the case.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal.form-state-cache.14.patch2.54 KB

You are right - that was a needless condition. Hence, this slightly revised patch simply brings consistency into both cases:

// Case: reload from cache
      $form_build_id = $form_state['input']['form_build_id'];
      $form = form_get_cache($form_build_id, $form_state);

// Case: build from scratch
      $form = drupal_retrieve_form($form_id, $form_state);
      $form_build_id = 'form-' . md5(uniqid(mt_rand(), TRUE));

Even more clean now, thanks!

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Title: $form_state is stateless, even with enabled caching » clean up form caching logic
FileSize
form-state-cache.patch3.62 KB

I realized that the caching logic needs more improvements. Currently when a form is going to be rebuilt and needs caching, it's cached, then rebuilt and cached again. So in case we do the rebuild we don't need to cache beforehand.

I improved your patch to fix that by moving the caching block below the rebuild, which also has the side-affect of making caching possible when $_SESSION['batch_form_state'] is set. However the case when running from $_SESSION['batch_form_state'] seems broken to me anyway, as $form never gets set somewhere? But that's another issue then.

Also $form_state['cache'] has a default value, so no need to use !empty() to check it.

>Form #process handlers and #validate handlers are then supposed to use $form_state - as the name implies - as state machine for subsequent rebuilds of the form.

Right, but that patch does fix it only for storage. So let's tackle that in #641356: Cache more of $form_state when form caching is enabled afterwards.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: clean up form caching logic » $form_state is stateless, even with enabled caching
Status: Needs review » Reviewed & tested by the community
FileSize
drupal.form-state-cache.14.patch2.54 KB

Argh. Please don't. I think you're losing $form_build_id now. At least, it's not passed to drupal_rebuild_form(), which performs its own caching.

That's the part that was contained in earlier patches of this issue. chx disagreed that $form_build_id should keep the same in rebuild scenarios, so I removed it, and I already mentioned in #622922: User input keeping during multistep forms that I want/need to open a separate issue about that.

Since you didn't change the code of the old patch, I'm re-uploading and marking this RTBC.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

I committed sun's patch, but happy to keep working on fago's suggested improvements. Please re-open the issue if necessary, fago. Thanks all.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

>Argh. Please don't. I think you're losing $form_build_id now. At least, it's not passed to drupal_rebuild_form(), which performs its own caching.

It was never passed to drupal_rebuild_form() inside from drupal_build_form() and my patch doesn't touch that at all, that's only done via the ajax handler. Also my patch doesn't touch the $form_build_id logic - still each rebuild gets its own build-id. But when you are already rebuilding you don't need to cache for the old build id too. So I re-rolled the patch and moved it into #647130: Cache only once when rebuilding a form

Status: Fixed » Closed (fixed)
Issue tags: -D7 Form API challenge

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +D7 Form API challenge

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.