button broken due to fix various problems when using form storage
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | forms system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | sun |
| Status: | needs work |
| Issue tags: | drupal-6.15-blocker |
Currently $form_state['storage'] is intended to be set in a #submit callback. So when creating a multistep form you can put your stuff into $form_state during the form build, then populate the storage during #submit.
This works fine, but when you start to add some #ahah stuff to the form, #cache will be set and the form will be cached when it's loaded the first time. As an affect the #submit callback is called
* without an initialized $form_state, as the form got cached through #cache
* without a form storage, as it wasn't set yet
So there is no way to make use of the $form_state apart from setting $form_state['storage'] during the form build. However currently this isn't intended and doesn't work well, because when you do it
* the form is immediately rebuilt even when it's loaded the very first time
* $form is written two times into the cache, once due to the storage and once due to #cache
To solve this I'd propose support setting the form storage during form build and fix the two mentioned issues. If there is an agreement that this is the way to solve it, I'd do a patch.
Note: This issue applies to 6.x too.
#1
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.
#2
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?
#3
I just created a book page about our mock modules. Please see http://drupal.org/node/302577
#4
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.
#5
Test patch and issue patch apply file, and the FormsAPI simpletest runs correctly (88 passes, 0 fails, 0 exceptions).
Robin
#6
@Robin - please slow down. Significant FAPI changes need review from Eaton or chx.
#7
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 :)
#8
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.
#9
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.
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?
#10
>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).
#11
Subscribing.
#12
Can we add some inline comments explaining the how's and why's of the enhanced condition?
#13
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.
#14
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.
#15
The last submitted patch failed testing.
#16
See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
#17
Re-test.
#18
Subscribing.
#19
Subscribing.
#20
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).
#21
Subscribing.
#22
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.
#23
Some feedback:
- We use dashes instead of underscores for menu/url paths.
form_test/form_storageshould beform-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!
#24
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.
#25
I forgot to rename the counting "builds" variable too, fixed that.
#26
Code looks good, fixes a real problem, comes with tests and documentation. Excellent work. Committed to CVS HEAD. Thanks fago.
#27
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.
#28
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?
#29
#13 still applies and still works for me on 6.x-dev.
#30
Committed to Drupal 6 too, thanks.
#31
Not sure if it's because project_issue's comment handling is doing something unholy and relying on the bug that this fixed, but deploying this change on d.o broke issue comment previews. See #579900: Previewing comments is broken by FAPI change in Drupal core 6.14. I don't have time to dig much deeper (I've got a massive deadline this weekend), other than narrowing it down to this API change as the source of the problem. I haven't even read this issue yet or studied the patch. But, if anyone familiar with this change is willing/able to take a look at issue comment previews, that'd be great... Thanks!
#32
subscribe
#33
!empty($form_state['submitted']) && !form_get_errors() <= why is form_state['submitted'] there? the !form_get_errors yeah but the submitted check broke buttons as dww shows.
#34
To clarify, by "buttons" we mean FAPI form elements of #type 'button', like core's "Preview" button on comment forms...
#35
yeah, looks wrong.
However, comment previews are working in HEAD, so this doesn't seem to be covered by tests.
#36
comment previews are working in D6, too. Core comment previews do *not* require rebuilding the form (and in fact, you break the preview if you rebuild the form). what's broken are forms that use #type 'button' to trigger something that requires the form to rebuild. But right, that might not be tested in D7, either...
#37
Like that? Not sure.
#38
#type = button.
#39
In terms of the new test in #38 -- no, I think we really want to test both #type == 'submit' and 'button', not one or the other. So long as both exist in FAPI (and API bug IMHO, but whatever), we should test both properly for this case.
#40
p.s. I know you love to clean up every file you touch, but all the whitespace and code style fixes make this patch harder to read. ;) Can't we do that stuff in separate issues? *shrug* Either way, thanks for fixing them. ;)
#41
This change also broke multistep forms in Drupal 6.14. If '#button' is used, $form_state['values'] is empty.
#42
Also created: #591696: 6.14 broke AHAH with form validation
#43
This is really serious for 6.14! E.g. the biblio module stopped working after upgrading from 6.13 to 6.14. The patch in http://drupal.org/node/591696 makes it working again, so I can get around this, but I guess there should be an official patch for 6.14.
#44
Tagging
#45
+
#46
Which patch should I apply to 6.14 to fix this?
#47
The one from #43 works for our 6.14.
#48
Does this fix the bug where button D was pressed but the function for button A was called instead? For 6.14
#49
This multistep form bug also effects a custom module that we have for a customer site. Looking for ways of working around it currently.
#50
#51
The last submitted patch failed testing.
#52
Fixed the tests.
#53
ouch, um, my fault :( I'm sry for that.
@patch:
The fix looks good. However the number of form constructions should be 1 less for the version with cache activated. I think you are testing it both times without having #cache activated as you are missing the query parameter for the GET call.
#54
+++ modules/simpletest/tests/form_test.module 17 Nov 2009 18:17:55 -0000
@@ -297,8 +297,10 @@ function form_storage_test_form($form, &
$form_state['storage'] += array('step' => 1);
}
+ // Output how often this form has been constructed.
+ drupal_set_message('Form constructions: ' . $_SESSION['constructions']);
- // Count how often the form is constructed
+ // Count how often the form is constructed.
$_SESSION['constructions']++;
uhm - are you sure?
It counts the number of constructions in a session variable - not the number of submitted steps or cached rebuilds or whatever.
This review is powered by Dreditor.
#55
Right, it counts how often the form constructor is called. But this depends whether caching is on or not.
Workflow without $form_state['cache']:
Get page -> Form construction #1
Post step1 ->
* Form is constructed (#2) and the form submitted.
* Form is rebuilt -> Construction (#3) + Form gets cached.
Post step2 ->
* Form is loaded from cache and the form submitted.
* Form is rebuilt -> Construction (#4)
Workflow with $form_state['cache'] (e.g. due to #ajax):
Get page -> Form construction #1 + Form gets cached.
Post step1 ->
* Form is loaded from cache and the form submitted.
* Form is rebuilt -> Construction (#2) + Form gets cached.
Post step2 ->
* Form is loaded from cache and the form submitted.
* Form is rebuilt -> Construction (#3)
Thus the we should have one construction less with caching. As said I think you are missing
<?phparray('query' => array('cache' => 1))
?>
#56
rfay requested that failed test be re-tested.
#57
The patch in #52 will have to be re-rolled due to #634440: Remove auto-rebuilding magic for $form_state['storage'] landing. I was about to do it, but since this is the most sensitive area of all our current undertakings, I thought I'd leave it to the principals on this issue.