User input keeping during multistep forms

No, value callbacks are passed $form_state for special purposes only; most of them shouldn't need it. Instead, they get a $input parameter that _form_builder_handle_input_element() should create for them correctly in this code:

  // Set the element's #value property.
  if (!isset($element['#value']) && !array_key_exists('#value', $element)) {
    $value_callback = !empty($element['#value_callback']) ? $element['#value_callback'] : 'form_type_' . $element['#type'] . '_value';

    if ($form_state['programmed'] || ($form_state['process_input'] && (!isset($element['#access']) || $element['#access']))) {
      $input = $form_state['input'];
      foreach ($element['#parents'] as $parent) {
        $input = isset($input[$parent]) ? $input[$parent] : NULL;
      }
      // If we have input for the current element, assign it to the #value property.
      if (!$form_state['programmed'] || isset($input)) {
        // Call #type_value to set the form value;
        if (function_exists($value_callback)) {
          $element['#value'] = $value_callback($element, $input, $form_state);
        }
...

Can you post the steps to reproduce the bug you're seeing?

x-post. thanks for posting the example :)

Just chiming in to comfirm this bug. $form_state['values'] gets wiped after the call to drupal_rebuild_form in ajax_form_callback() - and rfay's description above looks pretty dead on based on what I was seeing when trying to debug. Also marking as critical, because I think it is ;-)

um, now marking as critical

Ok, I think I might have found the problem:

• drupal_rebuild_form() sets $form_state['input'] = array()
• Then _form_builder_handle_input_element() calls $value_callback($element, FALSE, $form_state) instead of $value_callback($element, $input, $form_state), because there's no $form_state['input'].
• Then _form_builder_handle_input_element() calls form_set_value($element, $element['#value'], $form_state) which wipes out the data from $form_state['values'].

I don't know what the solution is, but this definitely seems like a critical bug.

Some guidance for how to test this:

• Go to the example page from #3.
• Breakpoint the line $form = drupal_rebuild_form($form_id, $form_state, $form_build_id); within ajax_form_callback().
• Trigger an AJAX submission.
• Before the breakpointed line, $form_state['values'] contains data for all submitted elements.
• After the breakpointed line, $form_state['values'] does not contain data for elements that have a value callback (for example, 'css_data_commands'), and neither does the corresponding element's #value property within $form.

Expected behavior: after the breakpointed line $form_state['values'] and elements' #value properties should contain the data submitted by the AJAX form.

I don't, but FAPI often makes no sense to me, so I'd like to know what quicksketch, sun, or another FAPI guru thinks.

+++ includes/form.inc
@@ -1476,6 +1499,9 @@ function form_type_select_value($element, $input = FALSE) {
+  if ($form_state != NULL && !empty($form_state['values'][$element['#name']])) {
+    return $form_state['values'][$element['#name']];
+  }

It seems like you're doing the same everywhere.

Hence, not a job for each value callback.

I'm on crack. Are you, too?

Wait a second. There was a very clear intention of destroying POST before rebuild form is called, so that every form is starting from the same state. By pulling values out of form_state['values'] you are circumventing this protection and we get back to the sorry mess we had before.

I talked with rfay and he promised he will post a bug report.

@chx: I'm curious what rfay posts with respect to a bug report, but please see #11. That's either a very clear FAPI bug report, or else that behavior of drupal_rebuild_form() is by design, in which case, ajax_form_callback() is calling drupal_rebuild_form() with the wrong expectation, and this becomes an AJAX bug. But I think only you can answer whether the "Expected behavior" in that comment is the wrong expectation.

Well. Apparently people keep inventing clever ways to code around the clarifications we tried to build in. So what about tthis?

And to clarify where I stand -- if you blindly copy user input from one form to the next, then you will not be able to uncheck a checkbox in a multistep form.

I have a lot to say sorry for the many posts here: "Stepping through ajax_form_callback() it's clear that $form_state is destroyed during the call to drupal_rebuild_form() in ajax_form_callback(). Everything is fine to that point." This is not so. We delete form_state['input'] and after my patch form_state['values'] so that every form, whether it's regenerated or not starts from the same, clean state. You have your liberty of using a submit handler to populate form_state as you please and reuse that as node_form does... What is the Drupal 7 (supposed?) workflow if not this?

We need quicksketch or merlinofchaos to look at this. We need to:

1) Understand what the correct expectation is for what's in $form and $form_state within a #ajax['callback'] function.
2) Ensure that ajax_form_callback() is written in a way to satisfy that expectation.

Yes, AJAX in D7 *should* be made easy. People implementing #ajax['callback'] functions should understand what $form and $form_state represent, and all the details involved in getting them properly initialized should be safely hidden away from them by whatever magic needs to exist in ajax_form_callback().

It looks like this bug report will have an API change. Yes, we urgently need merlinofchaos and quicksketch in here.

This is exactly how it worked in D6 and it seems that the misunderstanding is that although the Ajax side of things has changed greatly in D7, FAPI itself hasn't, so a submit handler is still required in order to hold on to anything from $form_state['values'], i.e. by putting it into $form_state['something_else'].

Here's more puzzling info that doesn't require rfay's example module. I did a fresh install of HEAD, edited the "page" content type, added an "unlimited values" text field, then went to node/add/page. Then I filled in the following:

• Clicked on the "revision information" tab, and checked the "Create new revision" checkbox and entered "Log message" for the "Revision log message".
• At the top of the form, set "Title" to "Title" and "Body" to "Body".

Then I breakpointed the line described in #11 and started a debug request by clicking the "add another item" for the multi-valued text field. I got the following results before and after the breakpoint.

Before rebuild
$form_state['values']['body']['zxx'][0]['value'] = 'Body'
$form['body']['zxx'][0]['value']['#default_value'] = 'Body'
$form['body']['zxx'][0]['value']['#value'] = 'Body'

$form_state['values']['revision'] = 1
$form['revision_information']['revision']['#default_value'] = false
$form['revision_information']['revision']['#value'] = 1

$form_state['values']['log'] = 'Log message'
$form['revision_information']['log']['#default_value'] = ''
$form['revision_information']['log']['#value'] = 'Log message'

After rebuild
$form_state['values']['body']['zxx'][0]['value'] = 'Body'
$form['body']['zxx'][0]['value']['#default_value'] = 'Body'
$form['body']['zxx'][0]['value']['#value'] = 'Body'

$form_state['values']['revision'] = false
$form['revision_information']['revision']['#default_value'] = false
$form['revision_information']['revision']['#value'] = false

$form_state['values']['log'] = 'Log message'
$form['revision_information']['log']['#default_value'] = 'Log message'
$form['revision_information']['log']['#value'] = 'Log message'

These results were the same regardless of whether I was using HEAD, or the #19 patch.

Questions:

• Does it make sense that before the rebuild, the #default_value for the body field is set to 'Body'? As I understand FAPI, before the rebuild, #default_value should be what the initial default value was, which in this case, would be empty string. In this regard, the behavior of 'revision' and 'log' make more sense to me.
• If I understand chx correctly, then rebuilding the form should wipe all input data from $form_state['values'] and the #value property. But this only happened for the 'revision' checkbox. The body and log fields did not get wiped.
• If I also understand multi step forms correctly (and chances are, I don't), then it seems like if $form_state['values'] and #value are being wiped after a rebuild (since we're now dealing with a "new" form that hasn't been submitted yet), then at least #default_value should reflect the default value of that new step which should be based on the #value of the previous step, but in the case of 'revision', it's not. So, after the rebuild, there is nothing we can inspect to know what was submitted with respect to the 'revision' checkbox.

As I understand it, one of two things needs to happen:

• The intention is either that after a rebuild, we should still have $form_state['values'] and #value properties be what they were before the rebuild, and we currently have a bug that this doesn't seem to be true some of the time (such as 'revision' in the above example), and we need to fix it so that it's always true.
• Or, as I understand chx, after a rebuild, $form_state['values'] and #value properties should be fully wiped, and new information should live exclusively in #default_value properties. In which case, we still need to find out why that's not happening for 'revision'. Also, if this is the way multi-step forms are supposed to work, then I suggest not calling drupal_rebuild_form() within ajax_form_callback(), but instead, let the AJAX callback itself decide if and when it's time to rebuild. If all AJAX callbacks could do was return new content to replace old content, then always doing a rebuild would make sense. But, since AJAX callbacks could do all sorts of interesting things and choose not to return replacement content, then we shouldn't assume that all AJAX callbacks are meant to follow multi-step form logic.

Summary: merlinofcahos, quicksketch, chx: please help us understand if either of the above statements is true, or if I'm totally missing the point of how any of this is supposed to work.

1) There is a certain expectation by developers.

2) The current code in HEAD does not seem to do what developers expect.

3) We have a whole framework for expectations.

So why don't we start with a test that fails, but shows the expectation? We need a test for whatever this patch results in anyway.

We can certainly mimic AJAX requests in the current testing framework. Note that the JS unit testing framework is only about unit tests within JavaScript.

A test for this is definitely possible and a great idea. I've never written a test before, but what a great use-case to use for teaching myself how to do it. I have some other work I need to get done today, but I'll work on this as soon as I can unless someone beats me to it.

The purpose of the test is to inspect $form and $form_state before and after drupal_rebuild_form() under different circumstances. So, we can create a module that implements a menu callback that returns a form and another menu callback that the form can be submitted to. That 2nd menu callback can duplicate what ajax_form_callback() does up until the call to drupal_rebuild_form(). The test can then make assertions of what's in $form and $form_state before and after calling drupal_rebuild_form(). The difficulty is we don't know what the proper expectation is (what should we be asserting?), but a little playing with it should reveal some pattern, which we can use to form an expectation, and then let more knowledgeable folks tell us that our expectations are wrong.

It's possible that this will reveal that there actually is consistency and that drupal_rebuild_form() does indeed destroy $form_state['values'] and #value properties, and that this is by design. If this turns out to be the case, then see my last bullet point in #27.

Wow.

I just read the original post and first comment on #367006: [meta] Field attach API integration for entity forms is ungrokable once again. And I just realized that it's (partially) talking about the very same wonky issue that requires some wonky form values massaging to make AJAX callbacks and form submissions work without JS.
However, that issue has been turned into more in-depth WTF-issue about Field API + Form API integration, so I just want to mention that this issue is not the only one.

@effulgentsia you looked at node form which does this in the beginning of the node_form:

  if (isset($form_state['node'])) {
    $node = (object)($form_state['node'] + (array)$node);
  }

which copies over values. That's why you are puzzled.

Node form is not the only form that contains such weird magic. While working on #414424: Introduce Form API #type 'text_format', I realized that there are quite some other (entity) forms that implement this or even more weird code to temporarily store + resurrect a "thing" in $form_state, most often converted from array to object, and on rebuild, fetch that "thing" back, most often converted from object to array, or, the other way around. Countless of code lines, plus a lot of technically unnecessary form submit handlers and separate form handling functions, or in other words:

Stuff we don't want.

Well, I took good historical look at the problem. Let me see

1. The tale begins http://drupal.org/drupal-4.6.2 here. There was a need to make sure the filter form is always access checked.
2. Adrian Rossouw writes form API. It quickly becomes clear that it provides a solution for the previous problem. Obviously it was not the sole reason to not release Drupal 4.7 for so long but it definitely was a serious reason.
3. In 2007 spring, Jeff Eaton and I sit down to look at form API #138706: FormAPI 3: Ready to rock. I discover another security weakness in poll module #138706-31: FormAPI 3: Ready to rock. Such weakness must not happen ever again.

Sun's patch while convinient goes against years of Drupal hardening. However, salvaging it is possible and not too hard: just make sure that form_builder nukes form_state[values] in case of a form_error. So add to form_builder:

   // Special handling if we're on the top level form element.
   if (isset($element['#type']) && $element['#type'] == 'form') {
  // the above lines exist the lines below are new
     if (form_get_errors()) {
       $form_state['values'] = array();
     }
  }

that will make sure form_state[values] wont get a chance to be reused if the form failed validation, most importantly the option checker.

Edit: observe how #10 suggested the same but for all cases even when it's not necessary.

To summarize once again:

1. By default every form that is built starts from scratch. With an empty $form_state['input']. Not considering any stored 'values' at all.
2. The previous patch was a regression in terms of security, because it took over arbitrary values from $form_state['values'] without making sure that those values underwent form validation. That is, because ajax_form_callback() calls drupal_rebuild_form() directly, and any other form building callback is able to so either.
3. We then investigated to move this check:
   

   if ((!empty($form_state['storage']) || $form_state['rebuild']) && $form_state['submitted'] && !form_get_errors()) {
     $form = drupal_rebuild_form($form_id, $form_state);
   }
   

   right into drupal_rebuild_form() itself, so drupal_rebuild_form() would immediately bail out and NOT return a new rebuilt form in case there was an validation error. (The important part is !form_get_errors() here.)

4. So the next conclusion was to enhance the previous patch with a condition that prevents $form_state['values'] from being taken over if there was a validation error. But since _form_builder_handle_input_element() is invoked recursively, we'd have to use a static variable or whatever to make it perform.
5. If there were validation errors, then we simply want to display back the same form.
6. And we also want to prevent that any processing functions invoked by form_builder() do not get arbitrary values. Hence, this munging must happen in form_builder() where special handling for the top-level form element lives.
7. The first potential issue mentioned was that checkboxes may not have any values upon rebuild. However, it is likely that #179932: Required radios/checkboxes are not validated fixes this.

   Note that the question whether we will be able to uncheck a checkbox may still remain. (Needs tests)

We need tests for this patch.

• remove the $node += form_state['node'] from node form and hit preview
• check that taxonomy stays clicked when previewing

That and then we need a multistep form that has a checkbox on it and check whether you can check, press next, uncheck, save and it gets saved unchecked. And then we need a test that posts tampered POST to a form (have a select box and post something that's not among the options) and make sure it does not survive in the next step.

rfay will summarize the remaining problem (or already has). In short: We need to setup the value before value callbacks are invoked.

#41 doesn't look right to me, so any further stuff should be based on #39.

Well, fine, but adding a if (!empty($form_state['values'])) check would be good to avoid iterating #parents when there is nothing (which, let's face it, is the common case.

@rfay, re: #37: No. I'm still finding myself confused by FAPI. Here's where I'm still stuck: #367006-23: [meta] Field attach API integration for entity forms is ungrokable.

You are changing general multistep form API behavior so the tests indeed belong here. Note that with pifr v2 you can actually click on the failures on qa.drupal.org and see what they were.

oh my - how stupid from me. Sorry :P

FYI: I'm working on an alternate approach and additional tests. I'm hoping to get somewhere with it today and post it here.

Let's keep this focused.

This one takes #59, adds some form rebuilding tests, but replaces the form.inc changes with my proposal. There remains a test failure and a comment within the form.inc hunk that explains it.

The new tests in this patch address what I was talking about in earlier comments that the wiping of $form_state['values'] and #value properties within drupal_rebuild_form() is BY DESIGN. What I perceive this issue to be about is that AJAX callbacks don't necessarily want to treat the form as a normal multi-step form, but rather as a special multi-step form where input values are not wiped, but are carried over from the pre-rebuild. But, they still need to be re-validated with the new $form definition.

+++ includes/ajax.inc	15 Nov 2009 19:51:52 -0000
@@ -259,7 +259,11 @@ function ajax_form_callback() {
+  if (empty($form_state['storage']) && empty($form_state['rebuild'])) {
+    $form_state['rebuild_progressive'] = TRUE;
+  }
   $form = drupal_rebuild_form($form_id, $form_state, $form_build_id);
+++ includes/form.inc	15 Nov 2009 19:51:53 -0000
@@ -315,6 +315,20 @@ function drupal_rebuild_form($form_id, &
+  // Also clear out values, but keep them in a separate variable in case some
+  // function wants to inspect it.
+  $form_state['values_before_rebuild'] = $form_state['values'];
+  $form_state['values'] = array();
+
+  // Support a progressive rebuild, where this step's input comes from previous
+  // step's values.
+  if (!empty($form_state['rebuild_progressive'])) {
+    $form_state['input'] = $form_state['values_before_rebuild'];
+    // Every rebuild step needs to set this explicitly; don't preserve this mode
+    // from one rebuild step to the next.
+    unset($form_state['rebuild_progressive']);
+  }
+++ modules/simpletest/tests/form_test.module	15 Nov 2009 19:51:55 -0000
@@ -393,3 +417,178 @@ function form_test_form_state_values_cle
+function form_progressive_rebuild_test_form_submit_add_item($form, &$form_state) {
+  $form_state['rebuild'] = TRUE;
+  $form_state['rebuild_progressive'] = TRUE;

I wonder whether this whole thing doesn't boil down to:

drupal_rebuild_form() is invoked with empty($form_state['rebuild']).

Or in other words: A rebuild is requested without $form_state['rebuild'] = TRUE.

This review is powered by Dreditor.

+++ includes/form.inc	15 Nov 2009 19:51:53 -0000
@@ -315,6 +315,20 @@ function drupal_rebuild_form($form_id, &
+  $form_state['values_before_rebuild'] = $form_state['values'];
...
+    $form_state['input'] = $form_state['values_before_rebuild'];

Also, you cannot do this...

Form element validation handlers and value callbacks can alter the 'value' of an element based on 'input'.

A good example is http://api.drupal.org/api/function/form_type_password_confirm_value/7

This review is powered by Dreditor.

OK, how about this?

I do not like this progressive rebuild thing, what if you want your form to fall back elegantly, you are back to square zero. There is no need for such distinction, AJAX or not, not wiping perfectly valid values (while makign sure user input triumphs over) is a nice thing and I think I made it secure. Just please make the tests pass.

Crosspost, but I still do not see the point in having two modes.

@chx: please look at form_test.module, the form_multistep_test_form() function, within patch 65. Am I wrong that this is a specifically designed feature of FAPI? That step 2 can have the same form element name refer to a different element, and therefore need a clean value, because it's essentially a different form. That's the reason for 2 modes. Unless, we decide we don't want to support these kinds of multi-step form constructions, in which case developers need to know that different elements needs different names across the entire multistep sequence.

+++ includes/form.inc	15 Nov 2009 22:57:58 -0000
@@ -309,15 +326,42 @@ function drupal_rebuild_form($form_id, &
+  if ($form_state['rebuild_mode'] == FORM_REBUILD_FROM_VALUES && form_get_errors()) {
+    // If a form asked to be rebuilt from validated values, but there are
+    // validation errors, then rebuild from input instead.
+    $form_state['rebuild_mode'] = FORM_REBUILD_FROM_INPUT;
+  }

@chx: if you like the basic structure of the overall patch, can you specifically give feedback on this block? I'm not clear on when you would ever rebuild a form that has errors. If you do, is this correct, or does it need to be set to FORM_REBUILD_CLEAN?

This review is powered by Dreditor.

So, yeah, I think all of that boils down to this one.

Not even sure whether this is about $form_state['values'] at all... anyway, fixed a stale comment.

@sun: any chance of you popping in on IRC so we can discuss?

+++ includes/form.inc	15 Nov 2009 23:56:45 -0000
@@ -309,7 +309,16 @@ function drupal_rebuild_form($form_id, &
+  if (!empty($form_state['rebuild'])) {

What about if a form uses $form_state['storage'], but doesn't set $form_state['rebuild']? Do we want that re-populating from input? Do we want those forms to have an option as to whether they repopulate from input? See #68. If what's in this patch is what we want, ok, I just want clarity.

+++ includes/form.inc	15 Nov 2009 23:56:45 -0000
@@ -309,7 +309,16 @@ function drupal_rebuild_form($form_id, &
+  else {
+    $form_state['input'] = $form_state['method'] == 'get' ? $_GET : $_POST;
+  }

What about programmatically submitted form? This won't work for them. Why not just leave $form_state['input'] alone, since this is in the else statement?

+++ includes/form.inc	15 Nov 2009 23:56:45 -0000
@@ -1044,6 +1053,11 @@ function form_builder($form_id, $element
+    // If there was a validation error, prevent usage of stored form values in
+    // rebuild scenarios.
+    if (form_get_errors()) {
+      $form_state['values'] = array();
+    }

Unrelated to this issue. What does this solve? Should we create a separate issue for it?

I'm on crack. Are you, too?

Also merged the tests

>Crosspost, but I still do not see the point in having two modes.

As mentioned in #68 keeping form values doesn't make much sense when the form changes. Well this is nice if you just reload the same form several times, but this won't make much sense for multistep forms having multiple different steps.

Thus I don't see why we should keep the form values at all. The usual multistep form workflow just lets the user care about setting the right form values (regardless whether it's an AJAX callback or not), which works fine as long as the form workflow is implemented in clean way. Yes, right now it's a mess and I think this is the real problem here.

See http://drupal.org/node/367006#comment-2272166 for a proposed clean worklfow using storage. Thus once the data is in the form storage it can be used to set the element values - thus fixing this problem.

related bug: #634984: Registration form breaks with add-more buttons

OK then for two modes, I can see this... keep values flag? Or simply if you do not want to keep em just delete it from form_state yourself? Is it harder to write $form['#progressive'] = FALSE; than $form_state['values'] = array(); ?

@fago: I think we're on the same page that we need to have some solution that allows both wizard-style multi-step forms to work AND have it be possible to add non-js and js "add more" buttons to multi-step or non-multi-step forms and have those work in a sane way. I'd like to understand better your proposal regarding $form_state['storage'] as a way of solving this. Within patch 65 exists test forms in form_test.module: form_multistep_test_form() (a wizard style form) and form_progressive_rebuild_test_form() (a form with non-js "add more" buttons). The patch includes simpletests for them, but you can also enable the form_test.module and run the forms visually in the browser to see how they work. Can you post your vision for how these can be written to use the "clean workflow" that you suggest?

@chx: Where do you propose setting $form_state['values'] = array();? In a submit handler? How can the submit handler know that it's the last submit handler running? Or do we want to establish a convention that submit handlers shouldn't read from $form_state['values'], but instead only from an element's #value, in which case, any submit handler can reset $form_state['values'] without breaking later submit handlers.

@chx: Or, maybe instead of a submit handler clearing the values, it can do the opposite: $form_state['rebuild_with_values'] = $form_state['values']? Is that better than $form_state['rebuild_mode'] (note, patch 65 replaced 'rebuild_progressive' with 'rebuild_mode')?

fago's solution may render this one irrelevant, but this patch tries to incorporate all feedback so far.

1) fago brought up a good point on #634984: Registration form breaks with add-more buttons, which is we shouldn't decide whether or not we want clean vs. carried over values based on $form_state['storage'] alone. Because, one could add an AJAX "add more" button on a multistep form.

2) This one simplifies. No need for different "modes". This uses a variant of chx's suggestion to be more direct. And sun's initial instinct that $form_state['rebuild'] is the important property to switch on.

3) No need to go through the 'input' pipeline again as is done in #75. Going through the 'input' pipeline runs into incompatibilities with "add more" functionality in conjunction with #default_value, as evidenced by the test failure. We have 'values' that have already been processed through the value callback. This uses them.

+++ includes/form.inc	17 Nov 2009 19:54:34 -0000
@@ -306,6 +306,16 @@ function drupal_rebuild_form($form_id, &
+  // A submit handler or other function in the form processing pipeline can
+  // set $form_state['rebuild_values'] to explicitly specify data values to use
+  // for populating the rebuilt form. If not explicitly set, and
+  // $form_state['rebuild'] has been set or the form has errors, we want to
+  // build a fresh state, but otherwise, we want to carry over values from
+  // before the rebuild.
+  if (!isset($form_state['rebuild_values']) && !$form_state['rebuild'] && !form_get_errors()) {
+    $form_state['rebuild_values'] = $form_state['values'];
+  }

*Why* would I want to $form_state['rebuild_values'] ?

I'm very opposed to introducing yet another $form_state property.

+++ includes/form.inc	17 Nov 2009 19:54:34 -0000
@@ -1193,6 +1213,21 @@ function _form_builder_handle_input_elem
+    // If rebuilding from previously validated values, set #value based on them.
+    if (!empty($form_state['rebuild_values'])) {

Maybe the reasoning for the change in form_builder() in the previous patches didn't come through:

There is nothing that ensures that 'values' have been validated.

Hence, chx's idea was to make form_builder() nuke 'values' in case there was a validation error.

This review is powered by Dreditor.

>Regarding introducing possible troubles with real multistep forms:
With that I meant that an rebuild keeping values might lead to wrong values in the next step of a form. You addressed that already by making the feature optional.

So this patch looks fine, however I'm not sure whether it's a good thing to do.
* Currently forms and multistep forms work quite different. When you want to do a multistep form you have to work around quite some caveats you don't have to deal with usually. With that patch I see the differences increasing.. What if you suddenly need to write a AJAX callback for a multistep form? It won't work as usual and probably you won't find much docs about it..
* Instead of the form workflows becoming easier and working always the same there is just another magic option confusing developers...

Instead of that I'd like to see the existing form workflows cleaned up. For that I'd propose to use the form storage, see http://drupal.org/node/367006#comment-2272166 for an example. Once you put in your data into the form storage initially, you have it available any time and you can modify it any time. You have one single copy of it, thus you don't get confused with old or new form values. When constructing the form, you always populate it from your data and it will always contain the latest values. It would work that way for regular forms and it would work for real multistep forms - no difference. Having no difference means also a form_alter() can make a form multistep without breaking the form e.g. it could add an "add more" or "hide" button. Also things working always the same way ensures it's easy to add another step to an existing simple form - without having to fix the existing stuff.

Well to make the form storage that consistent to use we need to remove it's magic, so you could use it although you don't plan to do a multistep form -> see #634440: Remove auto-rebuilding magic for $form_state['storage'].

I have no problems with this approach. We always said that the submit handler should stuff the values on which the next step could build into something. If you pick rebuild_values as that something, then it's automatically reused. If you, on the other hand, set rebuild_values to array() then no harm done. Minor: a nonempty rebuild_values should autotrigger form_state['rebuild'] however.

Security wise, we are good. Automatic copying of values to rebuild_values does not happen if there was a validation error. Manual copying won't commence either because submit are not fired on validation errors.

I crossposted with fago. I agree with fago too :D

I fear we're not getting anywhere. Let's do a general discussion in #635552: [meta issue] Major Form API/Field API problems and go back to individual issues afterwards. Thanks.

Update to folks following this issue: Patch 82 will need a re-roll due to #633156: Need tests for AJAX Commands landing, and to incorporate feedback. However, I'm waiting for #634440: Remove auto-rebuilding magic for $form_state['storage'] to land first, since it will also affect some of the details of this patch. After it lands, I'll re-roll this patch, and we'll be able to continue a discussion about this approach vs. #84.

@chx: if you're seeing this message, you are being solicited to review #634440: Remove auto-rebuilding magic for $form_state['storage'].

btw, http://api.drupal.org/api/function/text_field_widget_formatted_text_value/7 goes one step further and even provides an explanation for more weirdness: Form element value callbacks don't receive values for elements with #access => FALSE.

This patch includes a re-roll for #633156: Need tests for AJAX Commands and update due to the decisions in #634440: Remove auto-rebuilding magic for $form_state['storage']. It also includes more documentation in response to #83. I changed $form_state['rebuild_values'] to $form_state['rebuild_options']['values'], because, let's face it, the whole rebuilding thing is pretty complex, so let's assume we'll need more options some day (whether in D7 or D8).

From #85:

Minor: a nonempty rebuild_values should autotrigger form_state['rebuild'] however.

This patch does not include that, and I'd prefer it doesn't, because this type of magic is along the lines of what we've been trying to remove in other issues. If you want a form to be rebuilt, set $form_state['rebuild'] = TRUE. If you want to turn off rebuilding, set $form_state['rebuild'] = FALSE. Let's not include other properties that can turn rebuilding on or off. In that spirit, $form_state['rebuild_options'] affects what will happen IF a form is rebuilt, without affecting WHETHER or not it's rebuilt.

Re #84: While I would like us to explore cleaning up form construction and alter functions to use $form_state['storage'] in a consistent way, and while that may be necessary in some forms for Field API integration to be grokkable, I'm skeptical that it makes sense to require this on all forms, core and contrib, especially considering that code freeze has happened and people have started porting their modules. However, I think it's important that adding an "add another item" button (not just in the context of Field API, but any contrib module that wants to add a button to a form that changes the form in some small incremental way) should be possible on all forms, and therefore, I think this patch makes sense even if we also implement the improvements fago is suggesting to forms that let you edit entities and fields.

Re #89: I'm not clear how that affects this patch. It makes sense that #access=FALSE should cause $form_state['input'] to be ignored for that element, which is the behavior in HEAD and is unchanged by this patch.

Same as #90 except:

+++ modules/simpletest/tests/ajax.test	23 Nov 2009 21:54:20 -0000
@@ -141,3 +141,46 @@ class AJAXCommandsTestCase extends AJAXT
+      $commands = $this->drupalAJAXPost($form_path, $edit, 'checkbox');

s/drupalAJAXPost/drupalPostAJAX/

Sorry. #90 and #91 still had extraneous hunks that should have been removed due to #633156: Need tests for AJAX Commands landing. This one should be good, but still needs code review.

#90 and #91 should have failed bot, but didn't because the AJAX tests didn't run. I'll report that as a separate PIFR issue. However, clicking on "view details" for #92 does show that the AJAX and form tests ran.

As noted above, #92 is fine and can proceed to being reviewed, but for anyone troubled by the fact that #90 and #91 passed bot, I opened #641038: Sometimes, tests in a buggy patch don't run and therefore don't fail, which can lead to broken HEAD.

Re #90: I see your point, however I still think we should really avoid mystifying real multistep forms even more. (see #84)

So what could we do to make it easier to make all forms "multistep-aware"? So what about making $form_state persistent as whole, like the storage? Imo the name already suggests that the data in their persists the whole form state and as of now the $form_state is quite useless. See #641356: Cache more of $form_state when form caching is enabled.

If we would do it that way all we would need to do for the current implemented forms is to cleanup the form workflow, so that the form values get populated from the data in the form_state if it's there. That way we would enforce cleaner form workflows - having the data only once: in the form_state in contrast to having forms with X different data values (the original value e.g. $form['#node'], the form element default value, the submitted form element value)

As mentioned before on my own, I agree with fago.

It seems like all this patch wants is to keep 'values' somewhere floating around. Many form handlers, which need to re-access the values already do something like in this new patch.

However, looking some more at this code now, I don't quite understand why we do not simply try to keep 'values'. The only difference to my earlier patches in the more recent patches is the location where 'values' are taken into account during form element input handling. Aside from that, it applies the very same logic, and the very same protection for form validation errors.

Same as #97, but fixing the failing test.

And here is the approach I mentioned in #97:

As in earlier patches in this issue, we simply keep $form_state['values'] as long as there are no validation errors. The difference to my earlier patches is that those values are processed in a different location in _form_builder_handle_input_element(), as in the patches by effulgentsia.

Additionally, the example multi-step form that has been added in form_multistep_test_form() was a bit strange. It tried to verify that a form element appearing identically in multiple steps with the same name would not take over a submitted value. That's entirely not what developers expect. Developers expect that the identical element will get the previously submitted value. Otherwise, you will use a different element.

If this one works, then I think we have reached a great simplification of the form processing and rebuilding workflow here.

>It tried to verify that a form element appearing identically in multiple steps with the same name would not take over a submitted value.

Yep, it needs to work that way when the next step is another form but has overlapping form element names. Thus being "real multistep".

I would never expect this behavior. And it seems to overly complicate the problem we're trying to solve.

If I'd want that, then I'd do:

$form['#tree'] = TRUE;

$form['account']['name'] = ...

$form['friend']['name'] = ...

Without an account/friend key - expecting that 'name' will be reset + emptied in multi-step scenarios doesn't work at all. At least not with the previous attempts in this issue, in which we tried to take over any values from a previously submitted step. That is, because there is no information about which values should be kept and which not. Hence, we would have to supply an array of form element parents to denote a list of special form elements that shall take over the value, and skip the value for all other elements not listed in there.

Doing that, however, would make this entire workflow so complex that there is no real benefit from accounting for any previously submitted values at all. It would also break the use-case of making a non-multi-step form multi-step, because one (but also potentially more than one) hook_form_alter() implementation would have to define the values that should be taken over during rebuilding, whereas a single hook_form_alter() isn't necessarily the last hook_form_alter(), and thus, some elements/values may be missing.

So. I don't really have a clue what "real multi-step" means, but the presumption that the _identical_ form element within the _same_ form should be emptied under certain circumstances but not under others seems entirely wrong to me - and is incompatible in itself with the approached solution of this issue.

@sun: Re #102: The problem we're trying to solve is this. FAPI has for several Drupal versions behaved such that drupal_rebuild_form() wipes $_POST (prior to Drupal 7) / $form_state['input'] (Drupal 7 improvement so that rebuilding one form doesn't affect other forms on the same page) and rebuilds the form in such a way that element #value properties and $form_state['values'] get built fresh without implicit continuity from the previous step. To have continuity, form building and processing functions are expected to put things into / take things out of $form_state explicitly. The summary and first paragraph of the PHPDoc for the function pretty much states this (though perhaps less clearly than it should).

I don't know if we consider this a good thing or not, but it's how it is. And some people who write multistep forms thinks it makes sense. Step 2 of a wizard is different than step 1, and a 'name' field in step 2 isn't necessarily the same field as the 'name' field in step 1. Similarly, a delete confirmation form is different than the form on which 'delete' was clicked, and so data submitted for the 'name' field on one isn't necessarily expected to carry over to the value of the 'name' field on the other.

At the heart of this issue is that we have specific use-cases where we DO need continuity of values. AJAX in general, and "add another item" buttons specifically (whether AJAX or not).

Which leads us to EITHER changing the behavior across the board, as you suggest, so that we change FAPI from ALWAYS rebuilding fresh to ALWAYS rebuilding with continuity, OR expanding FAPI to make it an option, as I have tried to suggest. I have submitted earlier patches trying to clarify this option ($form_state['rebuild_progressive'] in #61, $form_state['rebuild_mode'] in #65). In #79, chx agreed that two modes make sense, but suggested that instead of introducing a new property in $form_state, a form wanting to drop continuity should explicitly wipe $form_state['values'] in its submit handler. My concern with that idea is that it would require the submit handler to assume it's the last one running, because any later submit handler that runs prior to the rebuild would lose access to $form_state['values'], which would be weird. That led me to introducing a new $form_state variable to contain values for the rebuild without affecting $form_state['values'] itself prior to when drupal_rebuild_form() runs.

I think if we wanted to, we could keep polishing the name of the new $form_state[] property ('rebuild_preserve_values', 'rebuild_wipe_values', etc.) until it conveyed the concept clearly. Whatever property we introduce would be fine, because when a submit handler sets $form_state['rebuild'], it knows whether it wants the rebuilt form to have continuity or not (the handler knows whether it's operating as a "add another item" type of handler, or whether it's moving to a new step in a wizard, etc.).

However, I can also see that sun, fago, and chx are all against introducing any new property to $form_state. But that requires that we fundamentally change the design of how FAPI models multistep forms. We would have to make it clear that in ALL cases (validation errors aside), drupal_rebuild_form() maintains continuity of submitted values, and that if you're building a wizard where you don't want that, then you're responsible for unsetting $form_state['values'] in a submit handler that you've setup to run sufficiently late relative to other submit handlers that need to inspect $form_state['values'] (or better yet, just build your wizard steps to use unique element names). I'm fine with this decision if we think it's what's best for FAPI in order to keep it simple. I don't think that many people will be affected by it. But I think we have to accept that it's a pretty fundamental change to how FAPI models multistep forms.

If we're not comfortable making that kind of change to FAPI at this stage of the release cycle, then I see no way around introducing some new property in $form_state that can make it clear which of the two behaviors we want out of a particular rebuild.

>If we're not comfortable making that kind of change to FAPI at this stage of the release cycle, then I see no way around introducing some new property in $form_state that can make it clear which of the two behaviors we want out of a particular rebuild.

Well there is another way out - leave it like it's now but implement clean form workflows: Put your data into form state storage and then always use this data to populate or update values - that's it. Of course in this scenario only cleaned up forms would be "multistep-save".

#641356: Cache more of $form_state when form caching is enabled would just help migrating forms to work that way as we don't have to move everything into 'storage'. That we could just use $form_state which is often already in use - but of course it would work with storage too.

@fago: I still don't understand that proposal. We have $form_state['input'] and $form_state['values'] already. It sounds like you want to add yet another property and alter many forms so that they copy over 'input' into 'whatever', and base their form element values on 'whatever'. Which, in effect, is the same as respecting the existing 'input' first, and any potentially existing 'values' afterwards automatically. That's what the latest patch is doing, which needs no conversion and is pretty transparent when looking at the overall form processing flow in a big picture. Therefore, all forms can be multi-step and all forms can be rebuilt without major modifications to the form construction and form handling.

In #104, I said that I'm ok with a decision to change FAPI to always have continuity of valid values across a rebuild. I do think it makes sense. I don't think it's much of a burden to require multi-step forms to be written with this in mind. I don't think very many contrib modules will be negatively affected by this decision. As long as chx (as FAPI maintainer) and webchick or Dries are ok with it, let's see where it leads us. In fact, I'm still not clear whether the loss of continuity was an intentional FAPI design decision or a side-effect of needing to clear $_POST because of security issues. If continuity can be introduced in a secure way (and chx has said that the basic approach we've been taking meets that standard), maybe now is the time to get rid of that no longer wanted side-effect.

With this in mind, here's a continuation of #100. It will need some more cleanup, but I want to see what bot thinks and get feedback on the basic approach. The big change here relative to #100 is that we can't just re-use $form_state['values'] directly. That leads to unwanted side-effects (most of the test failures in #100). We need to put the old $form_state['values'] somewhere and use that and let the new $form_state['values'] get built cleanly. But, the difference between what I was proposing earlier and this new approach (based on #102) is that the "somewhere" can be somewhere private to FAPI and not intended for form implementations to use. So, in this patch, I put it into $form_state['build_info']['values'] since in prior issues it was suggested that 'build_info' should be thought of as a private FAPI property. I'm open to using a different variable (e.g., $form_state['_rebuild_values']) if 'build_info' isn't a logical place to store this information (the description of build_info is that it's for information needed to rebuild from cache, and that's NOT what's happening here).

Also, I added a temporary fix in field_add_more_submit(), which will need a better fix. Field API is one place in core that DOES assume a loss of continuity. Interestingly enough, it's the main use-case where continuity is needed, so I expect the better fix will result in the cleaning up of all sorts of convoluted code.

@effulgentsia: I'm not sure I understand the last paragraph in #107 - care to elaborate ?

@yched: Sure. I'm not sure how much of this issue you've been tracking, so I'll risk over-elaborating. One (but not the only) problem behind #367006: [meta] Field attach API integration for entity forms is ungrokable, #634984: Registration form breaks with add-more buttons, and related issues is that an "add another item" button must trigger a form rebuild. By itself that's not a problem, but the way FAPI works, element #value properties and $form_state['values'] get wiped during a rebuild, but after you click "add another item", it's desired for the rest of the form (elements unrelated to the field for which you're adding an item) to retain their values, and since FAPI doesn't do this for you, the specific form implementation must make this happen. And since Field API must work on any entity editing form, it needs to jump through hoops to make this happen. This issue is about changing drupal_rebuild_form() to maintain continuity of element #value properties and $form_state['values'] for elements that are the same in the rebuilt form. While the "add another item" of Field API is one use-case for this, this issue was originally opened to solve other AJAX-related WTFs (see top of issue).

While I believe this change will assist in making Field API integration with entity forms more grokkable, I'd like to leave that work to #367006: [meta] Field attach API integration for entity forms is ungrokable, and not take that up in this issue.

However, because the current Field API code has been written with the way drupal_rebuild_form() currently works, the change to drupal_rebuild_form() introduced in #107 causes test failures in FieldFormTestCase (see test details from #100). The test failures have to do with element names not being re-ordered by weight. So, #107 adds a hunk to wipe $form_state['values'] within field_add_more_submit(), enabling the form building functions invoked during the rebuild to receive wiped values as they currently do in HEAD. But, I see this as a temporary measure to move this issue along, and after this issue lands, I think we can take advantage of the change to drupal_rebuild_form() to clean up Field API in #367006: [meta] Field attach API integration for entity forms is ungrokable.

@effulgentsia: Thanks ! Great summary, and the 'temporary fix' makes more sense now.
FWIW, approach looks good to me, but I'll let you guys agree on this.

>The test failures have to do with element names not being re-ordered by weight.

That's a case where the next "step" should have different form values. So how is with those cases now dealt? Is one supposed to unset the "internal" $form_state['build_info']['values'] ?

@sun #106
No I suggest to introduce anything new. I suggest to use the a clean workflow as I already described in #367006: [meta] Field attach API integration for entity forms is ungrokable - let's explain in detail:

* If the storage isn't set, populate it with the initial entity (loaded or created from scratch) -> e.g. $form_state['storage']['node']
* Populate the form set the and initialize the form values with data from $form_state['storage']['node'].
* Once validated submit the values by updating the entity in the form storage. -> $form_state['storage']['node'] is fully updated node object now.
* If the form is rebuilt, everything works fine without any magic as the updated entity from the storage is used.
* On the final submit, just save the updated entity $form_state['storage']['node'].

I've implemented that workflow in profile2 and works fine. That way the form constructor always has full control over the actual form values, also each form handler, ajax callbacks or whatever has always easily access to the latest data.

@fago: Yes, that's what we want to do in that other issue. But that is only related to forms dealing with entities, or, stateful objects. #629252: field_attach_form() should make available all field translations on submit already implements a first mini-step towards that. However, we have to solve this problem for all forms.

@effulgentsia: $form_state['build_info'] is cached with the form when caching is enabled. I'm still not sure whether I understand why we need to copy/duplicate the values, but caching them doesn't sound right to me.

On a related note, #642702: Form validation handlers cannot alter $form structure proves that this is required for all forms. I've added a commented out assertion in the patch over there, which should be uncommented in this patch.

@#112
We could apply this pattern to all forms, exchange $entity with your $form_data and it'll work exactly the same way...

Let's see what bot thinks of this.

+++ includes/form.inc	26 Nov 2009 20:08:53 -0000
@@ -1188,6 +1190,7 @@ function _form_builder_handle_input_elem
+    // Set #value based on submitted input.
     if ($form_state['programmed'] || ($form_state['process_input'] && (!isset($element['#access']) || $element['#access']))) {
       $input = $form_state['input'];

The previous patches worked, because we moved the entire handling for 'values' below this condition - which also solved the "no value for #access FALSE elements" problem in one fell swoop (#access is contained in this condition).

This review is powered by Dreditor.

.

Re: #117, I don't get it. The reason for moving it below was because we couldn't run 'input' through value callbacks a second time. If '#access' is FALSE, then we shouldn't override #value with anything other than what the constructor/alter/#process functions do.

This one solves the test failures.

If an element has #access FALSE when a form is rebuilt/processed again, it has no #value, because there is no #input. The current code falls back to "Load defaults", i.e. tries to invoke a potential value callback, and if still no #value is set, takes over #default_value. That was at least a part of the essential changes in the very first patches of this issue.

OK. This patch fixes it so that #access isn't checked for input from a rebuild.

I removed unnecessary test cases from the earlier patches (if we're changing the expectation of a rebuild from what I thought it was to what we've agreed to since #100, then that expectation only needs to be tested once, not in 3 redundant ways) and cleaned up the remaining test case.

I fixed the places where $form_state['rebuild'] was being set in a validation handler, based on your explanation in #642702-9: Form validation handlers cannot alter $form structure. And I updated the PHPDoc for the 'rebuild' property to match.

I cleaned up some other comments based on my evolving understanding of how this all fits together.

The main thrust of this patch (as well as the last couple) that is different from #107 and earlier is the copying of $form_state['values'] to $form_state['input'], and the initializing of 'input', 'groups', and 'values' BEFORE invoking the form constructor. To me, this makes a lot more sense. In a normal form build, during form construction and altering, 'input' contains the input to the form and 'values' is empty, and during the form_builder() phase (i.e., #process functions), 'values' starts empty and is progressively added to as form_builder() iterates through all of the elements. IMO, it makes sense to follow the same pattern during a rebuild. This enables all of the form pipeline functions to be less concerned about whether they are running in a rebuild context or not, which I think at least partially gets us to fago's vision.

+++ includes/form.inc	27 Nov 2009 21:13:36 -0000
@@ -317,6 +320,19 @@ function form_state_defaults() {
 function drupal_rebuild_form($form_id, &$form_state, $form_build_id = NULL) {
...
+  $form_state['input'] = form_get_errors() ? array() : $form_state['values'];
...
+  $form_state['values'] = array();
+++ modules/taxonomy/taxonomy.admin.inc	27 Nov 2009 21:13:37 -0000
@@ -111,8 +111,8 @@ function taxonomy_form_vocabulary($form,
-  if (isset($form_state['confirm_delete']) && isset($form_state['values']['vid'])) {
-    return taxonomy_vocabulary_confirm_delete($form, $form_state, $form_state['values']['vid']);
+  if (isset($form_state['confirm_delete']) && isset($form_state['input']['vid'])) {
+    return taxonomy_vocabulary_confirm_delete($form, $form_state, $form_state['input']['vid']);

ugh. Until here, I really liked this patch.

Can't we avoid this anomaly by applying the same logic to 'values' like we do for 'input'?

I know that this wouldn't strictly follow your idea of 100% same processing flow, but...?

+++ modules/simpletest/tests/form.test	27 Nov 2009 21:13:37 -0000
@@ -71,6 +71,7 @@ class FormsTestCase extends DrupalWebTes
           $form = $form_state = array();
+          $form_state += form_state_defaults();

heh. We can simplify this. ;)

+++ modules/simpletest/tests/form.test	27 Nov 2009 21:13:37 -0000
@@ -533,3 +534,34 @@ class FormStateValuesCleanTestCase exten
+class FormsRebuildPreservesValuesTestCase extends DrupalWebTestCase {
...
+      'name'  => 'Form state values preservation',

Just FormsRebuildTestCase. There are most likely more tests regarding further combination with form caching, form storage, etc. required in follow-up issues.

+++ modules/simpletest/tests/form.test	27 Nov 2009 21:13:37 -0000
@@ -533,3 +534,34 @@ class FormStateValuesCleanTestCase exten
+  function testRebuildPreservesValues() {
+    $user = $this->drupalCreateUser(array('access content'));
+    $this->drupalLogin($user);

Common for all form tests, can be moved into setUp().

+++ modules/simpletest/tests/form_test.module	27 Nov 2009 21:13:37 -0000
@@ -477,3 +485,67 @@ function _form_test_checkbox_submit($for
+ * Enables testing that a rebuild caused by clicking an "add another item"
+ * button results in a rebuilt form that has correct values for the elements
+ * controlled by that button as well as elements not controlled by that button.
+ */
+function form_test_form_rebuild_preserve_values_form($form, &$form_state) {

"Enables testing" ?

This review is powered by Dreditor.

With feedback from #122.

Can't we avoid this anomaly by applying the same logic to 'values' like we do for 'input'?

No. We cannot retain $form_state['values'] and use it to populate $form_state['values']. It creates some unexpected circularity in the form_builder() phase. Try debugging the test failures from #100 to see for yourself.

However, as you say yourself in #635552-21: [meta issue] Major Form API/Field API problems, form constructor functions should not have processing and validation logic, and I would extend that to mean that they should not access anything in $form_state['values']. During the construction phase, $form_state['values'] is empty (at least during initial build, and IMO, it should be this way during rebuild too). The other forms that check for 'confirm_delete' don't use $form_state['values'], and taxonomy shouldn't either. This patch fixes it in a way that's more consistent with the other ones (e.g., node deletion).

Same as #123, but taking into account that objects aren't arrays in the taxonomy_form_vocabulary() hunk :)

One more improvement. As stated in #121 and #123, I feel strongly that we should clear $form_state['values'] at the beginning of drupal_rebuild_form(), have it be empty during form construction and altering, and then get populated in the form_builder() phase from $form_state['input'].

However, while #125 cleans up taxonomy_form_vocabulary() to work with that change, there are other form constructor functions that would need to be cleaned up too (for example, authorize_filetransfer_form()). I'm not sure why tests didn't fail for that form with #125 (possibly, because the necessary test doesn't exist). In any case, we're already on 100+ comments for this issue, so I'd prefer to leave that cleanup for another issue.

Therefore, this patch comments that bit out and leaves it as a todo for another issue.

I think this is almost ready to fly.

+++ includes/form.inc	28 Nov 2009 01:06:45 -0000
@@ -317,6 +320,23 @@ function form_state_defaults() {
+  // @todo: Uncomment the below line after fixing the workflow of all forms to
+  // not have form constructors of forms being rebuilt rely on
+  // $form_state['values'] containing values from before the rebuild.
+  // See http://drupal.org/node/622922.

1) No colon after @todo, and, todos that wrap over multiple lines should be indented starting from the second line to make clear where the todo starts and where it ends.

2) @see

+++ includes/form.inc	28 Nov 2009 01:06:45 -0000
@@ -317,6 +320,23 @@ function form_state_defaults() {
+  // AJAX and other contexts may call drupal_rebuild_form() even if
+  // $form_state['rebuild'] has not been set, so set it here, to let form
+  // building functions know if they are running in a rebuilding context.
+  $form_state['rebuild'] = TRUE;

The part that tells that functions can check this property to get to know something makes me feel a bit uneasy. Technically, anything can set 'rebuild' at any time, and technically, anything can unset 'rebuild' at any time. No big deal, I guess we can keep this, even when it's a bit fragile.

+++ modules/simpletest/tests/ajax.test	28 Nov 2009 01:06:46 -0000
@@ -141,3 +141,46 @@ class AJAXCommandsTestCase extends AJAXT
+    $form_path = 'ajax_forms_test_get_form';
...
+      $commands = $this->drupalPostAJAX($form_path, $edit, 'select');
...
+      $commands = $this->drupalPostAJAX($form_path, $edit, 'checkbox');

Can we inline this, please? If at all, it should be $path, but I don't see a reason to abstract this here.

+++ modules/simpletest/tests/form.test	28 Nov 2009 01:06:46 -0000
@@ -413,16 +412,14 @@ class FormsFormStorageTestCase extends D
+    $user = $this->drupalCreateUser(array('access content'));
+    $this->drupalLogin($user);

This should be assigned to $this->web_user, so an individual test can still login/logout if necessary.

However, we can as well remove this from this patch, because #644150: $form_state is stateless, even with enabled caching already contains this change (except for the drupal_json_decode() fix you squeezed in here).

+++ modules/simpletest/tests/form.test	28 Nov 2009 01:06:46 -0000
@@ -533,3 +523,37 @@ class FormStateValuesCleanTestCase exten
+    $user = $this->drupalCreateUser(array('access content'));
+    $this->drupalLogin($user);

Same as above, but this one we want to keep.

I'm on crack. Are you, too?

We need this nice test + form constructor also for #370537: Allow suppress of form errors (hack to make "more" buttons work properly), let's get this done. :)

Re-rolled with those minor review issues.

I think this is ready to fly.

Awesome. Thanks. #129 looks good to me.

$form_state['input'] = form_get_errors() ? array() : $form_state['values'];

this won't work. value is not a filtered copy of $_POST it can be vastly different. I do not know where this got into the patch when I said "I am ok with this" it was not there IMO.

Edit: looking at patch history, it was a separate piece just a few days ago $form_state['build_info']['values'] = form_get_errors() ? array() : $form_state['values']; but then it slipped in...

Hold. I am working on this.

So now i have loaded the form_test_form_rebuild_preserve_values_form and I think we have some very deep misunderstanding here again? Here is the basic form API workflow:

+----------------+                  +--------+
| Construct form |  =============>  | Render |
+----------------+                  +--------+
                                          |
+----------------+    Submit to the server|
| Construct form |  <---------------------+
+----------------+

When you are constructing the form submitted back, what information do you have that would hold the number of textfields? You have not put anything in the form itself, there is nothing in form_state['storage'] ...

I am terribly sorry for not having enough time to work on these issues more, I am trying to find a form that should work and it does not.

Hey! fago helped me understanding the problem is not that the count of fields is lost but that the value in the other field is lost. Ah-ha... ok working

The form provided in the test case is broken as it's making wrong usage of the form_state as chx described above. But apart from that it correctly tests whether the values are kept or not.

OK so you want this

function form_test_form_rebuild_preserve_values_form($form, &$form_state) {
  $form['grocery'] = array(
    '#type' => 'textfield',
    '#title' => 'Grocery item',
    '#default_value' => 'DEFAULT',
  );
  $form['submit'] = array(
    '#type' => 'submit',
    '#value' => 'Submit',
  );
  return $form;
}

function form_test_form_rebuild_preserve_values_form_submit($form, &$form_state) {
  $form_state['rebuild'] = TRUE;
}

not to lose the value on pressing the button. I can see how this is felt as a bug.

This patch

1. Keeps form_state['input']. If you look at the comment it is no longer necessary because drupal_process_form was removed earlier and replaced by form_builder so now the validate/submit handlers cant possibly fire. This keeps the input values as we wanted.
2. Removes a weird edge case coming from the original form API 3.0 patch: if you were passing in a NULL for drupal_execute it was ...
   skipped. However, the way you create a drupal_execute payload is to take a $_POST and change the values in there to your variables. So if you had a checkbox and wanted to uncheck in you needed to check in something explicit to drupal_execute while $_POST passed in nothing. (Yes drupal_execute is gone but the explanation was simpler this way).
3. Keeps the tests from the issue (even if groceries are vastly simplified) and make them pass.

Edit: In a separate issue we can discuss the AJAX hardening part of this patch which added an error check before calling drupal_rebuild_form. I feel that's not necessary we just need to set $form['#submit'] = array(); to make very very sure the top level submit handlers cant fire but the button level ones can. This will solve the more button issue, but we will see.

With that patch the values are kept, which really simplifies doing ajax stuff in 1-step forms. Wizard like forms still can set form_state['input'] = array(); , what should be documented then. But it's simple and makes sense for wizards.

Also in a follow-up we should fix up and simplify the node form to make use of this functionality - as it's exactly a case for which this is intended to be used.

Re-rolled and included the hank from sun's patch to temporary fix the field forms.

too silly. 'input' is our key to unset now.

Brought in lots of sun tests from another issue. I think all this passes and the changes to form.inc are relatively small.

Forms need to be aware whether they want to keep user input or not. Node or field multiple for example does not -- these need additional cleanup. Note that this patch is a continuation of #138 not using the above two.

Same as #145, except I re-inserted the testRebuildPreservesValues() test and corresponding form constructor from #129. However, I reworked the form_test_form_rebuild_preserve_values_form_submit_add_item() handler to match chx's solution to drupal_rebuild_form(). But this required a small change to _form_builder_handle_input_element() to allow $input to be FALSE. That change is commented in this patch.

@chx, @fago: can you confirm that this test addresses your concerns in #133 and #136?

+++ modules/simpletest/tests/form_test.module	29 Nov 2009 19:40:14 -0000
@@ -567,3 +600,73 @@ function _form_test_checkbox_submit($for
+function form_test_form_rebuild_preserve_values_form_submit_add_item($form, &$form_state) {
+  // During rebuild, the number of items constructed is based on
+  // $form_state['input']. To generate an extra item, add to the appropriate
+  // place in that variable. For the new item to have its value be its default
+  // value, its input must be set to FALSE.
+  $form_state['input'][$form_state['clicked_button']['#target_element']][] = FALSE;
+  $form_state['rebuild'] = TRUE;
+}

I don't understand this. Is this an attempt to keep only some submitted form values, while chx's reset of $form_state['input'] will kill all values?

I'm on crack. Are you, too?

#145 is a good solution with respect to keeping input values and works well for the simplified version of this test in #137. However, if you re-insert the test from #129 into the #145 patch, then you get 1 test failure in this test, because the newly added item gets its value set to empty string instead of 'DEFAULT'. Because $form_state['input'] exists, but is NULL for the newly added item, and NULL gets passed to the value callback. This updated test (and updated hunk in _form_builder_handle_input_element()) solves this by allowing a submit handler to add FALSE within $form_state['input'], making the #145 solution work in a situation where new items are being added to the form during a rebuild.

This, by the way, is why upto #129, we weren't simply keeping $form_state['input'] -- because it doesn't correctly allow default values on newly added items. But, if we can assume that the submit handler that triggers the rebuild knows about all new items that will be added, then #147 is workable because that handler can add $input=FALSE to those items.

Oh, and the reason that NULL must be passed to the value callback, is because otherwise, checkboxes don't work. I still don't understand why #129 wasn't an acceptable solution, but I trust that chx has good reasons, so I'm trying to find a way to work within the design constraint of #145.

Put another way, preserving a non-empty $form_state['input'] that contains data that must be sent to the value callback results in $form_state['process_input'] being true, which results in the value callback being called for each element, including elements that are new in the rebuilt form (for which there is no user submitted input), and this results in those new elements not having their default values respected. However, for checkboxes, there's no way to distinguish a lack of user input from user input of an unchecked checkbox. This is why previous patches attempted to create some solution where a rebuild would not require re-invoking value callbacks, but could use data from $form_state['values'], which does distinguish unchecked checkboxes from new checkboxes.

FYI: I'm working on another patch that includes tests and comments that clarify #152. Will post it when it's ready.

Thanks for working on the issue.

#129 copied form_state['values'] into form_state['input']. That operation does not make too much sense alas because we derive pretty freely our value from_state['input'] and after a process callback or two form_state['values'] does not resemble really form_state['input'].

Now this groceries-movies stuff you added back. Care to tell me why did you add it back? What workflow are you testing that were not covered in my patch? Anyways, even if it's necessary the comment needs improvement " For the new item to have its value be its default value" -- this needs a mention that the default value comes from storage.

Now, the big block of text about input FALSE, second sentence, that's not really grokkable for anyone not inmately familiar with the issue we are struggling with: " However, from a form submitted with drupal_form_submit(), or during form processing, an input variable can be set to FALSE for the element's default value to be used." what about something along the lines of "When working with a multistep form user input by default is kept from one step to the next. There is a problem with the checkbox element: the lack of user input means uncheck. So if a checkbox needs to keep its state from one step to the next, put its value in form_state['storage'], assign #default_value based on that and change the relevant $form_state['input'] key to FALSE in the submit handler. The same problem might arise when programatically submitting a form." Huh :)

Hm, read that again. If you want something to be the default disregarding user input then why on earth you are are not setting #value instead? Or put it in form_state['storage'] and not display to the user? What's going on? What's the real case here for that input FALSE...?

Thanks, chx. Working on it... Will post soon.

#145 plus a test that demonstrates why it fails based on #152. Will discuss options on IRC.

#157 plus chx's refactoring of form.inc to solve the checkbox problem in a more logical way. Let's see what bot thinks. Then we can do code review.

Adding back in a little code that was too aggressively removed.

This one should pass tests. Since this issue has gotten quite long, here's a (unsurprisingly, also long) summary. [EDIT: This summary has been edited from its original to be relevant to the patch in #187]

• In HEAD, drupal_rebuild_form() wipes $form_state['input'] resulting in the rebuilt form having element #value properties and $form_state['values'] being set to the elements' default values, instead of submitted values. This hasn't been identified as a problem prior to this issue, because there aren't that many multistep forms in Drupal, and where there are, they deal with this being the way FAPI works.
• This issue was opened, because in the context of a rebuild triggered due to an AJAX submission, it results in a total WTF. See top of issue. This WTF existed in Drupal 6 AHAH, but there were so many WTFs in Drupal 6 AHAH, that most people just got scared away from doing much with the AHAH framework, and those that didn't figured out how to work around it. The much improved D7 AJAX framework inspired rfay to flush out examples and documentation, and in that process, he opened this issue. This patch includes a test within ajax.test that asserts what someone building an AJAX callback would expect: $form_state['values'] populated with what was submitted. This test fails with the code in HEAD.
• The problem, however, is not limited to AJAX. Field API "add more" buttons need to work without AJAX, and jump through hoops to do so. It's possible this patch will help with #367006: [meta] Field attach API integration for entity forms is ungrokable. Even if it doesn't (i.e., if Field API continues to be complicated, because of how it needs to integrate storage in a generic way), it should be possible to build modules with simpler variants of "add more" buttons. This patch includes a FormsRebuildTestCase test for a really simple "add more" button. This test fails with the code in HEAD.
• This patch also includes an expansion of FormsFormStorageTestCase. This was a test that used to live in #644150: $form_state is stateless, even with enabled caching, but got moved here, because caching and storage are so interrelated with form rebuilding.
• Prior to #100, we explored supporting an option within $form_state for whether a particular form should be rebuilt the way drupal_rebuild_form() currently works in HEAD (no preservation of input values), or whether it should be rebuilt in the newly desired way (with preservation of input values). This would enable the newly added tests to work, while maintaining backwards compatibility for core forms and module developers used to the way things have been. However, this was deemed as adding too much complexity on top of an already complex system. It was decided that there are relatively few cases where the old behavior is desired, and these can be dealt with in those cases rather than adding options to $form_state. In other words, it was decided that drupal_rebuild_form() should be CHANGED to ALWAYS preserve input values.
• Simply removing $form_state['input'] = array() from drupal_rebuild_form() is insufficient. The FormsRebuildTestCase demonstrates why. Basically, it's hard to know whether an input value of NULL means a submitted empty checkbox (whose value must be set to unchecked regardless of its default) or a newly added checkbox (whose value must be set to its default). Some other controls (multiple select) also have this annoyance.
• The solution is to distinguish $input that is explicitly NULL from $input that's missing from $form_state['input']. Comments in patch explain this in more detail.
• This patch (same as #145) also includes the clearing of $form_state['input'] from node and field forms, because that's what these forms expect, and we're changing drupal_rebuild_form() to not do it any more. After this patch lands, whether node and field forms get refactored to take advantage of the change to drupal_rebuild_form() can be discussed in #367006: [meta] Field attach API integration for entity forms is ungrokable.

Thoughts?

This is a small improvement to #163. As I was writing the summary, I was somewhat bothered by requiring value callbacks to interpret FALSE as NULL. This patch fixes that by managing the NULL/FALSE conversion entirely within _form_builder_handle_input_element(). This means the API of a value callback is unchanged by this patch relative to HEAD. Within a value callback, $input=NULL means intentionally NULL input, and we never pass $input=FALSE to a value callback. This could mean that we can revert the "value callback" / "default value callback" split, and just pass $input=FALSE to a value callback to signify we want it to return a default value. But I kind of like the split, so left it in.

Re: #164: "But I kind of like the split, so left it in."

I changed my mind. I do like the split, but let's leave it for a separate issue and keep this one focused.

Agh. Sorry for all the spam, folks. This removes a couple more irrelevant hunks. If this passes bot, I'm done with it until there's feedback. I promise. For anyone joining here, summary is on #163.

Fixed a silly line of code being in the wrong place.

I couldn't put it down. The use of FALSE to mean a different NULL than NULL just seemed wrong. Here's a patch that uses array_key_exists() to distinguish the two kinds of NULLs.

Tiny comment improvement. Once again, I re-promise this to be the last patch I upload tonight, as long as it passes bot. If you're joining here, issue summary is on #163.

Changes look good and it comes with thoroughly tests for the checkbox case. Good work!

minor:
form_test_storage_element_validate_value_cached sets $form_state['cache'] which isn't needed as the constructor already does it when $_REQUEST['cache'] is set.

cleanliness:
+ if (!$input_entry_exists && empty($form_state['rebuild']) && empty($form_state['programmed'])) {
You use empty() for checking 'rebuild' and 'programmed', which have default values so the checks should be abbreviated by using a simple "!".

Unbelievable great work! I have rewritten the comment for the form_state[input] handling. What an awesome idea to reuse _form_set_value here.

+++ includes/form.inc	2009-11-30 14:43:33 +0000
@@ -317,6 +317,12 @@ function form_state_defaults() {
+  // AJAX and other contexts may call drupal_rebuild_form() even if the flag
+  // isn't set, but form_builder() and its helper functions need to execute
+  // special code during a rebuild, and potentially, so do form constructors,
+  // and other form processing functions.
+  $form_state['rebuild'] = TRUE;

"special code"? We need to be more explicit here.

+++ includes/form.inc	2009-11-30 14:43:33 +0000
@@ -1207,13 +1213,37 @@ function _form_builder_handle_input_elem
+      // controls. We would prefer the entries to exist and be set to NULL,
+      // so that in a rebuild or a programmatically submitted form, we can
+      // distinguish elements submitted as NULL from elements not submitted at
+      // all, for which the default needs to be used.

The message is clear after reading it twice, so we need to tweak the wording. Currently sounds like a "I wish" statement.

+++ includes/form.inc	2009-11-30 14:43:33 +0000
@@ -1207,13 +1213,37 @@ function _form_builder_handle_input_elem
+      // appear in POST. However, subsequent rebuilds need to know that the user
+      // actually have seen the element and submitted "empty". To achieve this
+      // NULL is stored back in $form_state['input']. On rebuilding, new
+      // elements not yet seen by the user and not existing in
+      // $form_state['input'] will be able to use their defaults, while these
+      // special elements will have their NULL marking them "empty".

Grammar needs work here.

+++ includes/form.inc	2009-11-30 14:43:33 +0000
@@ -1207,13 +1213,37 @@ function _form_builder_handle_input_elem
+      if (!$input_entry_exists && empty($form_state['rebuild']) && empty($form_state['programmed'])) {
+++ modules/field/field.form.inc	2009-11-30 14:32:27 +0000
@@ -131,6 +131,11 @@ function field_default_form($obj_type, $
+  if (!empty($form_state['rebuild'])) {
+++ modules/node/node.pages.inc	2009-11-30 14:32:27 +0000
@@ -109,8 +109,13 @@ function node_object_prepare($node) {
+  if (!empty($form_state['rebuild'])) {
+++ modules/simpletest/tests/form_test.module	2009-11-30 14:32:27 +0000
@@ -370,9 +378,12 @@ function form_test_mock_form_submit($for
+  if (!empty($form_state['rebuild'])) {

$form_state['rebuild'] is always defined.

+++ includes/form.inc	2009-11-30 14:43:33 +0000
@@ -1207,13 +1213,37 @@ function _form_builder_handle_input_elem
+        _form_set_value($form_state['input'], NULL, $element['#parents'], NULL);
+        $input_entry_exists = TRUE;

Can we explain what's happening here?

+++ modules/node/node.pages.inc	2009-11-30 14:32:27 +0000
@@ -109,8 +109,13 @@ function node_object_prepare($node) {
 function node_form($form, &$form_state, $node) {
...
+  // This form has its own multistep persistance.
...
+    // Node has its own multistep persistance.

Do we need to repeat this? If so, why?

+++ modules/simpletest/tests/form.test	2009-11-30 14:32:27 +0000
@@ -505,12 +499,54 @@ class FormsFormStorageTestCase extends D
+  function testCachedFormStorageValidation() {
...
+    $form_build_id = (string) $fields[0]['value'];
...
+    #$this->assertFieldByName('form_build_id', $form_build_id);
...
+    #$this->assertFieldByName('form_build_id', $form_build_id);
...
+    #$this->assertFieldByName('form_build_id', $form_build_id);

I guess we should remove those assertions from this patch and move this topic into a separate issue that I want to create anyway.

+++ modules/simpletest/tests/form_test.module	2009-11-30 14:32:27 +0000
@@ -426,9 +441,27 @@ function form_storage_test_form($form, &
+    // @todo This really shouldn't be necessary. Should Form API cache that a
+    //   form can be cached? See http://drupal.org/node/641356
+    $form_state['cache'] = TRUE;

This equally doesn't really belong to this issue/patch. It was required for my patch in the other issue and needs to move into the issue I'll create (regarding $form_build_id should stay the same throughout rebuilds). Not to be discussed in this issue.

I'm on crack. Are you, too?

Two quick notes: I missed the "Browsers do not submit $_POST entries " when writing mine, the two likely can and should be merged and grammarfixed.

Do we need to repeat this? No, it just got there in the many rerolls..

With feedback from #171 and #173.

Oops. Fixed small typo.

Because at suggestion of #171, changed code to assume 'rebuild' key exists in $form_state, had to add $form_state = form_state_defaults() to a couple of tests. Also, had to add $form_state['cache'] = TRUE; back into form_test_storage_element_validate_value_cached() to fix test failure. Since a re-roll was needed, I cleaned up some form.inc comments too.

Still more minor comment tweaking. This one's final though, until there's more feedback.

#178 and #179 are identical with respect to field.form.inc, so I don't understand why #178 passed tests while #179 claims a syntax error. Anyway, this one is re-rolled for latest HEAD, so hopefully, bot won't choke on it.

+++ includes/form.inc	30 Nov 2009 20:47:00 -0000
@@ -1205,13 +1212,37 @@ function _form_builder_handle_input_elem
+      // For web-submitted forms, intentionally NULL input for some elements
+      // (empty multiple select, unchecked checkbox) does not exist in $_POST.

wuhuh? Had to read that three times to understand ;)

+++ modules/simpletest/tests/form.test	30 Nov 2009 20:47:01 -0000
@@ -505,12 +500,50 @@ class FormsFormStorageTestCase extends D
+    // Also retrieve the form_build_id to assert that it keeps identical
+    // throughout this entire test.
+    $fields = $this->xpath($this->constructFieldXpath('name', 'form_build_id'));

+++ modules/simpletest/tests/form_test.module	30 Nov 2009 20:47:01 -0000
@@ -426,9 +441,27 @@ function form_storage_test_form($form, &
+    // @todo Fix FAPI to make it unnecessary to explicitly set the cache flag in
+    //   this situation. @see http://drupal.org/node/641356.
+    $form_state['cache'] = TRUE;

This belonged to the other patch as well, not used in here.

I'll re-roll to fix those docs, then you review, and then you optionally tell me what I didn't get right. Deal?

@sun: deal! To save yourself some time, run the form storage test when you remove $form_state['cache'] = TRUE. It will fail unless you change the test.

Hm, I see why it fails. The form gets cached so the constructor isn't run the next time, thus the flag doesn't get set. Well this changed since we don't have $form['#cache'] anymore, what was cached. Thus this might be a bug dependent on we want constructors being able to set the flag. If we don't, it's fine. Anyway it's another issue and shouldn't hold up this one.

So let's yell + kill me. :)

+++ includes/form.inc	30 Nov 2009 23:37:25 -0000
@@ -1205,13 +1209,40 @@ function _form_builder_handle_input_elem
+      // elements in $form_state['input']. When rebuilding a form, we can
+      // distinguish elements having NULL input from elements that have new
+      // values for the rebuilt form and can therefore use default values for

"elements that have new values" is either unclear or flat out incorrect. How about "When rebuilding a form, we can distinguish elements having NULL input from elements that were not part of the initially submitted form, and can therefore..."

This review is powered by Dreditor.

Thanks!

Awesome. Thanks to everyone for exploring and exposing flaws in much poorer solutions, until we finally reached one that makes sense. Since rfay, fago, and chx have already given their approval (#170-#172) except for minor issues that have all been addressed, I'm marking this RTBC.

Dries / webchick: issue summary is on #163.

So nice. Behold the power of working together, this is much better than any single of us including me could come up with. Oh joy.

Spent 20 minutes with this patch, and it looked good. Committed to CVS HEAD.

Awesome, thanks all!

So, now that we solved one more thing that was making AJAX confusing, I have a recommendation for another one: #649628: Make it easier to write AJAX-enabled forms that fully work with JavaScript disabled too. I think that issue will really help developers understand that submit logic and form rebuilding logic need to live within the appropriate form function, and not within #ajax['callback'], and I would really appreciate a review from anyone who's interested in that. Thanks!

Any chance this changed the behavior of #disabled elements ?
I'm confused by the behavior I see in #687572: When disabled textfields are submitted, their values are forced to empty string instead of the default value