Remove auto-rebuilding magic for $form_state['storage']

In light of #583730: How many ways are there to cache a form?, I'd really prefer to remove the _entire_ caching + rebuilding magic around 'storage'. Not just rebuilding.

I love you. :)

+++ includes/form.inc
@@ -767,8 +765,8 @@ function drupal_redirect_form($form_state) {
-  // Skip redirection for multi-step forms.
...
+  // Skip redirection for explictely set form rebuilds.
+  if (!empty($form_state['rebuild'])) {

This comment needs work.

This review is powered by Dreditor.

Better title.

Can we also add the two properties to the PHPDoc of drupal_build_form() ? Neither 'storage' nor 'rebuild' is mentioned there currently.

Seems like this one is a real road-block for #367006: [meta] Field attach API integration for entity forms is ungrokable. Since we really need to quickly move forward, bumping to critical.

Yes, yes, yes!! Thank you for this issue! I'm completely in favor. The whole concept of form rebuilding and storage is complex and takes a while for a new developer to understand, and by decoupling the two concepts, it removes a lot of WTFs. The only downside I can see is people used to building multistep forms by just setting $form_state['storage'] will now need to also set $form_state['rebuild']. But, they can stop unsetting $form_state['storage'] on the last step, so it's trading one extra line of code needed with one line saved, so no net change to overall difficulty. And, this way makes more sense. I'm sure webchick would have preferred it if this change got committed and documented before API freeze, because now, module developers who've begun porting their modules to D7 will need to be notified about this. However, this only affects module developers who build multistep forms, and the level of Drupal expertise needed to do that is high enough, that these same developers will quickly understand the change, and even if they missed the notification, they'll have the debugging skills to figure it out.

This patch includes the documentation requested in #10, changes a documentation line from If $form_state['rebuild'] is TRUE is populated to If $form_state['rebuild'] is TRUE, and replaces all instances of "multi-step" with "multistep", because let's set a standard on how that's spelled that we can use when documenting how multistep forms work.

+++ includes/form.inc	18 Nov 2009 00:12:26 -0000
@@ -123,7 +140,7 @@ function drupal_get_form($form_id) {
- *     common elements, such as back/next/save buttons in multi-step form
+ *     common elements, such as back/next/save buttons in multistep form
@@ -218,13 +235,12 @@ function drupal_build_form($form_id, &$f
+  // know that we're in a multi-part process of some sort and the form's
@@ -302,7 +318,7 @@ function drupal_rebuild_form($form_id, &
-    // be used to resume complex multi-step processes.
+    // be used to resume complex multistep processes.
@@ -499,7 +515,7 @@ function drupal_retrieve_form($form_id, 
-  // back/next/save buttons in multi-step form wizards.
+  // back/next/save buttons in multistep form wizards.
@@ -529,7 +545,7 @@ function drupal_retrieve_form($form_id, 
- *   multi-step form. Additional information, like the sanitized $_POST
+ *   multistep form. Additional information, like the sanitized $_POST

Please revert to "multi-step", and also rename "multi-part" to "multi-step" ;)

+++ includes/form.inc	18 Nov 2009 00:12:26 -0000
@@ -218,13 +235,12 @@ function drupal_build_form($form_id, &$f
+  // workflow is NOT complete. We need to construct a fresh copy of the form,

Not sure why we are yelling "not" here. So while being there, let's also fix that ;)

I'm on crack. Are you, too?

IMO it is important to get this simplified so more module devs grok multistep and we can stop calling upon eaton to write a blog post about how to do multistep in Drupal x. I can't imagine this change will affect more than one or two modules (if that) so +1 for a code freeze exception here.

Note that the changes around $form_state['storage'] and ['rebuild'] that are currently in the air might break some edge cases around batch API and multistep forms. This can be fixed afterwards and shouldn't hold this patch IMO, but we'd need the batch API tests added in #629794: Scaling issues with batch API sooner rather than later ;-).

Just to clarify in advance: This is no code freeze exception. It's a critical bug fix and we need to do this API change to make D7 work. #635552: [meta issue] Major Form API/Field API problems contains an overview and discussion of all related issues around this topic.

Review in #13 is minor stuff related to comments, so doesn't affect RTBC state of this issue, but @sun: there's a bunch of other places within core that spell it "multistep". Are you suggesting they all be changed to use "multi-step", or are you suggesting to just remove hunks from this patch and let the inconsistency remain?

I was under the assumption that "multi-step" is the proper term, or at least, being used everywhere else.

Anyway, totally unimportant here. No reason to hold off this patch.

I committed #583730: How many ways are there to cache a form? which might require this patch to be rerolled. I asked for a re-test.

HEAD is broken - I got those two failures on a local HEAD, and so does http://drupal.org/node/629794#comment-2280216

HEAD fixed - requiring re-test

So, this is RTBC then, yes?

I apologize for poor clarity in #25. It's not a question. Yes, this is RTBC, as it was in #18. Everything from #19 to #24 was noise due to HEAD breakage unrelated to this issue or patch. It would be very helpful for this to land or have concerns raised quickly, as many other FAPI issues await it (#635552: [meta issue] Major Form API/Field API problems).

I'd like to see an eaton or a chx on this issue.

Makes sense to have either or both of them weigh in.

@chx: In #635552-8: [meta issue] Major Form API/Field API problems, you ask:

form_state['storage'] triggers caching because you only want to use form_state['storage'] to persist state from one step to another. So if you have storage then you surely wanted to rebuild. I am not exactly sure whether this is the magic you want to remove and if yes then what we would achieve.

Please see this issue's description and comment #3 as well as the conversation in #635552: [meta issue] Major Form API/Field API problems. A summary of some of the reasons:

• You don't want a rebuild on the last step of a multistep form. Who's responsibility is it then to unset $form_state['storage'] on that last step? A submit handler? How does it know that there aren't additional submit handlers running after it that might still want to inspect $form_state['storage']?
• Say a module wants to prevent a rebuild of some other form for some reason. It implements an alter hook or a submit handler that sets $form_state['rebuild'] to FALSE and assumes that's enough. But no, it also needs to unset $form_state['storage']? WTF?
• To solve some of the Field API integration problems, it may make sense to have fields use $form_state['storage'] in case the fieldable entity uses a multistep form. But, if the fieldable entity doesn't use a multistep form, then just because Field API added something to the storage shouldn't automatically trigger a rebuild.

Point 3 is a little misphrased.
The 'add more' button de-facto makes any form where it appears multistep - i.e needs to support incoming form values from a previous state. That's to avoid "change node title, hit 'add more' for field_foo, the title input is reset" in non-JS mode.
"if the fieldable entity doesn't use a multistep form", then it's bugged. #367006: [meta] Field attach API integration for entity forms is ungrokable is about making the job of fieldable entities easier. And that involves using $form_state['storage'], which shouldn't *necessarily* mean the form is going to be rebuilt (if I just change node title and Save).

Well, OK. We wanted to make things easier but no magic is better I must admit.

We wanted to make things easier but no magic is better I must admit. I tried to parse that sentence but I can't really make sense of it -- probably because I'm less versed in English than you are. chx, are you suggesting this patch adds magic? If you have a concern, could you please articulate it? We were holding this patch up to get your opinion, so I'd like to make sure I understand it. Thanks!

- Reworked the $form_state documentation. (which are quite interesting, btw! Thanks, effulgentsia!)

- Removed unrelated changes.

This makes so much sense!

@Dries: No, this patch only removes ugly magic :)

Ah, I understand chx's comment now -- sorry. I agree that this is an improvement so committed to CVS HEAD.

The commit also contained all or parts of #601584: setUp() function for unit and web test cases should reset all resettable statics

@yched: I can only guess you posted to the wrong issue?

Also marking needs work for module upgrade documentation.

sun: no no, correct issue. http://drupal.org/cvs?commit=292080 contains this patch + bits of #601584: setUp() function for unit and web test cases should reset all resettable statics
Anyway, fixed now, Dries committed the other missing bits of that other patch.

That's correct. And the latest patch would have failed, but was committed too early.

Came to the same conclusion, and approx the same patch.

This patch still needs work. I get 9 exceptions in the Module => Enable/Disable Modules test.

Undefined index: uninstall	Notice	system.admin.inc	1154	system_modules_uninstall_confirm_form()	Exception
array_filter(): The first argument should be an array	Warning	system.admin.inc	1154	system_modules_uninstall_confirm_form()	Exception
Invalid argument supplied for foreach()	Warning	system.admin.inc	1154	system_modules_uninstall_confirm_form()	Exception

times 3.

Ok, committed #45 to HEAD. Yay for allowing us to enable modules without Drush. :P

This should be the proper fix.

Committed #47 as well.

.

I think this still needs some documentation (and the "Needs Documentation" tag agrees), as the Form API handbook explicitly states that setting $form_state['storage'] will also set $form_state['rebuild'] to TRUE. In my opinion, the Converting 6.x modules to 7.x should at least have a short line about this change(s).

Even though the Form API documentation does mention that $form_state['storage'] is no longer treated in a special way, this might as well imply that setting any other non-special key in $form_state would also rebuild the form.

Downgrading from critical.

Sounds like this needs both Upgrade Guide documentation and a mention in the Forms API topic/group page on api.drupal.org

I did a search on d.o documentation, and the only page I found that looked at all relevant to forms with the words 'storage' and 'rebuild' on it was this one:
http://drupal.org/node/717746
It doesn't look like it implies that setting $form_state['storage'] automatically sets $form_state['rebuild'] to me, except right down at the bottom:

// Commenting out the line with the unset() function and
// then adding a new set of name fields and submitting the form,
// causes the form to no longer clear itself. The reason is that when
// the 'storage' value is set, the $form_state['rebuild'] value will get
// set to true  causing the form fields to get rebuilt with the
// values found in $form_state['values'].
function my_module_my_form_submit($form, &$form_state) {
    unset($form_state['storage']);
    drupal_set_message(t('The form has been submitted.'));
}

This page is marked as Drupal 6, but if someone who understands this issue could fix the last example or its comment (by adding in a note about Drupal 7), that would be good, and then mark the page as Drupal 6/7.

Meanwhile, I added this to the 6/7 module update guide, so I think the update doc tag is no longer needed.
http://drupal.org/update/modules/6/7#form-auto-rebuild