Cache more of $form_state when form caching is enabled

I now identified the need for this in #644150: $form_state is stateless, even with enabled caching.

But we probably still want only a selected subset, or, white-list. Reversing it into a blacklist still sounds too much for D7 to me.

As far as D7 is concerned, I don't think that a patch that requires developers to update their code from $form_state['rebuild'] to $form_state['build']['rebuild'] (or whatever) will be in the loop.

For D7, I guess that only the white-list approach, or, shifting/copying around $form_state properties before/after getting/setting the cache will be possible.

I.e.

function form_set_cache() {
  $form_state['build_info'] += array(
    'cache' => $form_state['cache'],
    ...
  );
}

function form_get_cache() {
  $form_state['cache'] = $form_state['build_info']['cache'];
}

Perhaps we're talking past each other. Can you clarify what exactly you want to do? Either an example snippet like I did in #8 or a conceptual patch that will perhaps fail would be great.

This *might* clash with #629794: Scaling issues with batch API - which I really wish got committed ;-).

From the OP over there:

2nd issue is that batches defined within a form submit workflow (most common use case) store the whole $form and $form_state array. Problematic for 'big' forms like the modules page (which triggers a batches .po import) or simpletest list. In D7, $form_state['complete form'] contains the $form, so no need to store both. Additionally, only a few edge cases actually need the full $form_state, in most cases we only need a small subset.

From the patch over there:

--- includes/form.inc	12 Nov 2009 20:07:05 -0000	1.398
+++ includes/form.inc	13 Nov 2009 12:18:56 -0000
@@ -579,12 +579,23 @@ function drupal_process_form($form_id, &
       // that is already being processed (if a batch operation performs a
       // drupal_form_submit).
       if ($batch =& batch_get() && !isset($batch['current_set'])) {
-        // The batch uses its own copies of $form and $form_state for
-        // late execution of submit handlers and post-batch redirection.
-        $batch['form'] = $form;
-        $batch['form_state'] = $form_state;
+        // Store $form_state information in the batch definition.
+        // We need the full $form_state when either:
+        // - Some submit handlers were saved to be called during batch
+        //   processing. See form_execute_handlers().
+        // - The form is multistep.
+        // In other cases, we only need the information expected by
+        // drupal_redirect_form().
+        if ($batch['has_form_submits'] || !empty($form_state['rebuild']) || !empty($form_state['storage'])) {
+          $batch['form_state'] = $form_state;
+        }
+        else {
+          $batch['form_state'] = array_intersect_key($form_state, array_flip(array('programmed', 'rebuild', 'storage', 'no_redirect', 'redirect')));
+        }
+

+++ includes/form.inc
@@ -270,16 +270,16 @@ function drupal_build_form($form_id, &$form_state) {
 function form_state_defaults() {
   return array(
-    'args' => array(),
     'rebuild' => FALSE,
     'redirect' => NULL,
     'build_info' => array(),
-    'storage' => array(),
     'submitted' => FALSE,
     'programmed' => FALSE,
     'cache'=> FALSE,
-    'method' => 'post',
-    'groups' => array(),
+    'internal' => array(
+      'method' => 'post',
+      'groups' => array(),
+    ),
   );
 }

This is too much for D7.

We can fix this internally in form_set_cache() and form_get_cache().

+++ modules/simpletest/tests/form.test
@@ -660,3 +660,30 @@ class FormsRebuildTestCase extends DrupalWebTestCase {
+class FormTestStoragePersist extends DrupalWebTestCase {

This should be added to the FormsRebuildTestCase() instead, if required at all.

+++ modules/simpletest/tests/form_test.module
@@ -678,3 +686,36 @@ function form_test_form_rebuild_preserve_values_form_submit($form, &$form_state)
+function form_test_storage_persist($form, &$form_state) {

Do we really need a new test form for this?

+++ modules/simpletest/tests/form_test.module
@@ -678,3 +686,36 @@ function form_test_form_rebuild_preserve_values_form_submit($form, &$form_state)
+  // caching of the form, e.g. elements having #ajax.

#ajax is #process, not _alter.

I'm on crack. Are you, too?

+++ includes/form.inc
@@ -269,11 +269,10 @@ function drupal_build_form($form_id, &$form_state) {
-    'args' => array(),
...
     'build_info' => array(),

I'm not sure why this bug still exists, it should have been part of #622922: User input keeping during multistep forms.

We need to move 'args' into 'build_info'. Otherwise, drupal_build_form() will throw notices in certain situations.

+++ includes/form.inc
@@ -385,16 +382,41 @@ function form_set_cache($form_build_id, $form, $form_state) {
+ * Form elements that want to add further data that is related to the current
+ * build only and thus shouldn't be cachedm should add it to
+ * $form_state['build'].

This info belongs into Doxygen of drupal_build_form(), where also 'storage' needs to be updated.

Instead of 'build', we should use a more precise term here. 'temp' or 'temporary' would sound more appropriate to me.

+++ includes/form.inc
@@ -385,16 +382,41 @@ function form_set_cache($form_build_id, $form, $form_state) {
+function form_state_keys_no_cache() {
+  return array(
+    'always_process',
+    'build',
+    'buttons',
+    'cache',
+    'clicked_button',
+    'complete form',
+    'groups',
+    'input',
+    'method',
+    'must_validate',
+    'no_redirect',
+    'rebuild',
+    'redirect',
+    'submit_handlers',
+    'submitted',
+    'validate_handlers',
+    'values',

Can we group these after "public" and internal properties?

public: cache, must_validate, no_redirect, etc. - basically all that are mentioned in PHPDoc of drupal_build_form().

internal: all the others set by form building and input handling.

That would make more obvious that we missed no_cache here, for example. Perhaps more.

On that note though, we also need to discuss whether 'cache' shouldn't be cached, too. I think I already added a @todo to form_test.module that complains about the property not persisting.

I'm on crack. Are you, too?

+++ includes/form.inc
@@ -104,10 +104,6 @@ function drupal_get_form($form_id) {
- *   - storage: An array that may be used to store information related to the
- *     processed data in the form, which can be accessed and used within the
- *     same page request, but also persist across multiple steps in a multi-step
- *     form.

I think we need to move this information above the documented special properties now, so developers know that $form_state is/can be cached and what exactly is cached (and not).

+++ includes/form.inc
@@ -269,11 +267,10 @@ function drupal_build_form($form_id, &$form_state) {
+    'build' => array(),

Forgot to rename?

+++ includes/form.inc
@@ -385,16 +380,38 @@ function form_set_cache($form_build_id, $form, $form_state) {
+function form_state_keys_no_cache() {
+  return array(
+    'cache',
...
+    'buttons',

Thanks! Can we add an inline comment above 'cache' and 'buttons' to denote the two property groups?

+++ includes/form.inc
@@ -449,14 +466,14 @@ function drupal_form_submit($form_id, &$form_state) {
+  // Merge in default values.
+  $form_state += form_state_defaults();
 
   $form = drupal_retrieve_form($form_id, $form_state);
...
-  // Merge in default values.
-  $form_state += form_state_defaults();

Good catch! That makes totally sense. :)

+++ modules/simpletest/tests/form.test
@@ -660,3 +660,30 @@ class FormsRebuildTestCase extends DrupalWebTestCase {
+class FormTestStatePersist extends DrupalWebTestCase {
...
+  function testFormStatePersist() {

Please add this to class FormsFormStorageTestCase instead. (we probably also want to rename that test case, but we don't necessarily need to do that in this patch)

+++ modules/simpletest/tests/form_test.module
@@ -94,6 +94,14 @@ function form_test_menu() {
+  $items['form-test/state-persist'] = array(
+    'title' => t('Form test'),

This title could use some more love.

+++ modules/simpletest/tests/form_test.module
@@ -94,6 +94,14 @@ function form_test_menu() {
+    'type' => MENU_CALLBACK,

We actually should stop defining types. Makes it easier to unhide the test module and manually access the test pages in Navigation menu.

This review is powered by Dreditor.

Can we use 'temporary' instead of 'temp'? From the documentation it is not clear how temporary temporary data is? Does it persist across page requests? From the documentation, it is also not clear when you're supposed to use the temporary data. In other words, the documentation is a bit light.

+++ includes/form.inc
@@ -94,7 +94,10 @@ function drupal_get_form($form_id) {
  * @param &$form_state
  *   An array which stores information about the form. This is passed as a
  *   reference so that the caller can use it to examine what in the form changed
- *   when the form submission process is complete.
+ *   when the form submission process is complete. Furthermore it may be used
+ *   to store information related to the processed data in the form, which can
+ *   be accessed and used within the same page request, but also persist across
+ *   multiple steps in a multi-step form.

I agree with Dries that we want to connect this info to form caching (persisting across requests), but also state that $form_state stays intact during the same request in case a form is rebuilt.

+++ includes/form.inc
@@ -385,16 +383,40 @@ function form_set_cache($form_build_id, $form, $form_state) {
+    // Publicly used properties.
...
+    'clicked_button',
...
+    // Mostly internal used properties.

I think we should call them

"Public properties defined by form constructors and form handlers."

and

"Internal properties defined by form processing."

'clicked_button' definitely belongs to the internal properties. I think the public list should really equal the Doxygen of drupal_build_form().

I'm on crack. Are you, too?

+++ includes/form.inc
@@ -94,7 +94,10 @@ function drupal_get_form($form_id) {
+ *   to store information related to the processed data in the form. The data
+ *   put into $form_state is persisting across page requests, thus it can be
+ *   accessed and used during the whole form workflow. 

uhm, across page requests? That is only true when form caching is enabled and when form rebuilding is disabled.

This review is powered by Dreditor.

Well, this:

"put the data in $form_state and it will be there later on"

means to me: The data I put in there, which I may have altered in a #process or #validate will be contained as is.

But that's not necessarily the case, because form caching needs to be enabled. Form rebuilding will contain the same data during the rebuild, but not across page requests. That is, because we don't re-use the $form_build_id when invoking drupal_rebuild_form(). And that said, that's one of the most awkward WTFs in Form API, IMHO.

I don't believe you, since I had enormous WTF moments with form caching/rebuilding recently. Ideally, this should be covered tests. But that doesn't belong into this issue.

So let's just clarify that comment and make it more precise regarding cross-request persistence, based on the assumptions we currently have, and be done with this issue. ;)

We don't need to be lengthy, but the information is absolutely required for the PHPDoc of $form_state, because this is essential API documentation. Anyone who wants to change the API needs to be able to understand how it is intended to work.

+++ includes/form.inc
@@ -94,7 +94,10 @@ function drupal_get_form($form_id) {
  * @param &$form_state
  *   An array which stores information about the form. This is passed as a
  *   reference so that the caller can use it to examine what in the form changed
- *   when the form submission process is complete.
+ *   when the form submission process is complete. Furthermore it may be used
+ *   to store information related to the processed data in the form. The data
+ *   put into $form_state is persisting across page requests, thus it can be
+ *   accessed and used during the whole form workflow. 
  *   The following parameters may be set in $form_state to affect how the form
  *   is rendered:

So I suggest to change this to:

Furthermore, it may be used to store information related to the processed data in the form, which will persist across page requests when the 'cache' or 'rebuild' flag is set.

(also note trailing white-space here)

+++ includes/form.inc
@@ -385,16 +385,40 @@ function form_set_cache($form_build_id, $form, $form_state) {
+    'clicked_button',
+    'buttons',

Flipping these two would make sense.

+++ modules/simpletest/tests/form.test
@@ -541,6 +541,19 @@ class FormsFormStorageTestCase extends DrupalWebTestCase {
+  

+++ modules/simpletest/tests/form_test.module
@@ -678,3 +686,36 @@ function form_test_form_rebuild_preserve_values_form_submit($form, &$form_state)
+  

Trailing white-space here.

+++ modules/simpletest/tests/form_test.module
@@ -678,3 +686,36 @@ function form_test_form_rebuild_preserve_values_form_submit($form, &$form_state)
+    '#value' => t('Submit')

Missing trailing comma.

+++ modules/simpletest/tests/form_test.module
@@ -678,3 +686,36 @@ function form_test_form_rebuild_preserve_values_form_submit($form, &$form_state)
+function form_test_state_persist_submit($form, &$form_state) {
+  drupal_set_message($form_state['value']);
+ $form_state['rebuild'] = TRUE;
+}

Wrong indentation and missing PHPDoc.

+++ modules/simpletest/tests/form_test.module
@@ -678,3 +686,36 @@ function form_test_form_rebuild_preserve_values_form_submit($form, &$form_state)
+ * Implement hook_form_FORM_ID_alter().

s/Implement/Implements/

+++ modules/simpletest/tests/form_test.module
@@ -678,3 +686,36 @@ function form_test_form_rebuild_preserve_values_form_submit($form, &$form_state)
+function form_test_form_form_test_state_persist_alter(&$form, &$form_state) {
+  // Simulate a form alter implementation inserting form elements that enable
+  // caching of the form, e.g. elements having #ajax.
+  $form_state['cache'] = TRUE;
+}

I'd like to see the 'cache' and 'rebuild' flags tested separately, i.e. conditionally.

Just setting 'cache' => TRUE in this alter hook should make the $form_state persist. You may also need to trigger a validation error to properly test this.

Just setting 'rebuild' => TRUE in the submit handler should make the $form_state persist as well.

This review is powered by Dreditor.

+++ includes/form.inc
@@ -94,7 +94,10 @@ function drupal_get_form($form_id) {
+ *   persist across page requests when the 'cache' or 'rebuild' flag is set.
+ *

It is unfortunate and stupid and I'm totally sorry to have to request another re-roll, but as explained in http://drupal.org/node/1354, newlines are NOT supported within @param by the API parser.

This review is powered by Dreditor.

heh, thanks! ;)

Committed to CVS HEAD. Thanks!

46: form-state-persistence9.patch queued for re-testing.

Seems that this issue was re-opened by mistake. Closing again.