fix various problems when using form storage

Furthermore there is another related issue: When the storage is already set and there is a validation error, the form is rebuilt nevertheless! So when one uses a multistep form and saves stuff into the storage with the submit callback, then in this case the submit callbacks are skipped due to the validation error and the form is rebuilt without an updated storage, so any changes are lost.

So I've done a patch which fixes these problems:
* It makes sure the form storage lets the form rebuild only when the form has been submitted and there are no validation errors.
* It also saves the form storage to the cache table when #cache is activate by #ahah properties or something else.

These simple fixes now make it possible to
* put your thingie to edit into the form storage during the initial form build
* to update your thingie after each step out of a #submit callback
- which I'd consider as typical usage.

Furthermore now it's no problem to use the form storage together with #cache.

Note that I've tested the changes with a complex case - the rules admin UI for d6. It's a multistep form using storage which might have some #ahah properties in there - with this patch the form runs fine!

It applies to d7 and d6.

I think it would make sense to write a test for a multistep form using storage and #cache in d7 - however I'm not sure how to do it. Is there a way to add a testcase-module containing such a form just for testing?

I just created a book page about our mock modules. Please see http://drupal.org/node/302577

ah thanks!

ok, so I've written some tests using a mock module. The module provides a multistep form using storage and optionally #cache. Then the tests ensure that
* the storage properly stores the values
* the form builders aren't called too often
* the storage keeps working, when #cache is activated
* the form values are kept when an validation error occurs.

attached is a patch containing *only* the tests. Make sure to have the patch from #1 included or some tests will fail.

Test patch and issue patch apply file, and the FormsAPI simpletest runs correctly (88 passes, 0 fails, 0 exceptions).

Robin

@Robin - please slow down. Significant FAPI changes need review from Eaton or chx.

The second hunk is a bugfix -- you can have storage populated from a previous step and then a validate error will get the form rebuilt still. Whether we want the builder to be able to populate storage I am less sure. I am open to what Eaton says :)

thanks chx.

>Whether we want the builder to be able to populate storage I am less sure. I am open to what Eaton says :)
I'd call this a bug making $form_state unusable, as it requires one to use $form for storing values instead when caching by #cache should work too.

Anyway when putting stuff into the storage during build is not supported, the first hunk makes no difference either. It's unnecessary to pass NULL to form_set_cache when there is no storage.

I had posted a similar patch in other issue #291939: Multistep forms should not be rebuilt if there are validation errors. because I did not found this one.

Making sure the form storage lets the form rebuild only when there are not validation errors fixes the bugs reported and work Ok form me.

Anyway when putting stuff into the storage during build is not supported

Could you explain why this is not supported. I have some use case where is required to put stuff into the storage during the form builder function. Is there an alternative way to do this?

>Anyway when putting stuff into the storage during build is not supported, the first hunk makes no difference either. It's unnecessary to pass NULL to form_set_cache when there is no storage.

Anyway, I think putting stuff into storage during build *should* be supported. Currently it's not, but nobody knows. It's not documented and as it's useful people do - yes, I have seen several cases where people do so. Then some days ago I held a copy of the latest pro drupal development book in my hands - even the multistep form example in there sets $the storage in $form_state during the form build!

@dropcube: What happens when you populate the storage during form build currently?
* the form is immediately rebuilt even when it's loaded the very first time. So the builder runs 2x when the form is shown the first time.
* When #cache gets set, $form is written two times into the cache, once due to the storage and once due to #cache.

To summarize this simple patch
1. It fixes validation for multistep forms (prevent the rebuild!)
2. It fixes issues when populating form storage during form build

Furthermore thanks to the fix of point 2, it makes it possible to use the form storage in a clean way even when #cache gets activated. See post #1.

Imo the patch from #1 is an important patch which should get into d7 and d6! For d7 there is also a test (#4).

Subscribing.

Can we add some inline comments explaining the how's and why's of the enhanced condition?

ok, I've updated the patch.

Furthermore I've improved it a bit: When $form_state['rebuild'] is explicitly set, now it's too checked whether the form has been submitted and validation errors occured. It doesn't make sense to allow people skipping this check, so I've improved the patch to check it for both cases. As an affect you can set $form_state['rebuild'] in your form builder now too.

Updated and improved patch from #1 attached, applies to d6 & d7. The test for d7 (#4) still works fine once the patch is applied.

The patch looks good for me. I tested and it prevents that forms that contains validation errors get rebuild during a multi-step process. The Form API docs should be updated to clearly explains the workflow.

The last submitted patch failed testing.

See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476

Re-test.

Subscribing.

Subscribing.

Those two fixes looks reasonable at first sight. The test in #4 should be rolled in the patch (so that it can be tested by the test bot). There is some code style convention issues there (please see the Test writers guidelines for details). Plus I would like to be sure that the test cover both hunks (ie. it should break when any of them is not applied).

Subscribing.

so, next try.

* I've updated the patch, as the form api received some improvements in the meanwhile.
* I took a look at the test writer guidelines (thanks damien) and overhauled the test. The test already covers both hunks.
* Attached is the updated patch for d7, containing the both, the changes and test.

Some feedback:

- We use dashes instead of underscores for menu/url paths. form_test/form_storage should be form-test/form-storage.

- The test case looks OK but took me some time figuring out. Some additional documentation would have helped. I wonder if the test could be simplified.

- In testFormCached() why don't we pass array('query' => 'cache=1') to both calls to drupalPost()? It seems like that could make for a better test.

Let's get this patch in!

thanks.

The exiting menu items of the already existing form-API test use the path "form_test" - so I stayed with form_test for consistency, but fixed it to use form_test/form-storage. So probably form_test should be fixed there to be form-test for all those menu items, but that's another issue..

>In testFormCached() why don't we pass array('query' => 'cache=1') to both calls to drupalPost()? It seems like that could make for a better test.

It doesn't matter the second time, as the form is already cached and won't be constructed again. But yes, it's more logical to add it to the second call too, so I've done so.

>Some additional documentation would have helped. I wonder if the test could be simplified.

Unfortunately I can't think of a way simplifying the test. Multistep forms with form storage aren't that simple ;) However I added some more comments to the form and the tests. I also improved the language to talk about "form constructions" not "form builds", as form builds could be easily confused with the build of form_builder(), which is happening for cached forms too and isn't counted.

I forgot to rename the counting "builds" variable too, fixed that.

Code looks good, fixes a real problem, comes with tests and documentation. Excellent work. Committed to CVS HEAD. Thanks fago.

yeah, thanks!

So let's backport this to 6.x? The patch from #13 still applies fine to d6 and it's the same fix as got committed in d7.

Thanks fago, the patch from #13 is working nicely on my local Drupal 6.12 install.

Is this still on the cards for a back port?