button broken due to fix various problems when using form storage
| Project: | Drupal |
| Version: | 6.15 |
| Component: | forms system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | sun |
| Status: | reviewed & tested by the community |
| Issue tags: | D7 Form API challenge, drupal-6.16-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.
#58
.
#59
also we can't just remove the check for 'submitted' as else the form might rebuild even when it's loaded initially. I think checking "$form_state['process_input']" instead should do it as we only want to rebuild when the input has been processed beforehand.
#60
The previous step button will not work any longer on multistep forms (D6.14).
Please rollback or point me to a an example how to build a multistep form with previous step buttons.
Cheers!
#61
@JoepH: This is a D7 issue, and nothing has been committed yet. I'm not familiar with a 6.14 regression on this, but you may find one. But this isn't the place for the discussion.
When you do pursue this, you'll have to give a very specific example of your problem or nobody will be able to help you out.
#62
@rfay Just a reminder, this is also a D6 issue to as of #30 when the changes were committed into the D6 branch.
This change is preventing a couple of our sites from being upgrading to 6.14, although the change itself is required by two of our other D6 sites.
#63
I apologize for the misdirection and error. I am so used to things going into 7 and then working their way into 6.
#64
subscribe (coming from #638758: Fixes and improvements to emthumb).
#65
Would it be a good idea to create a new issue for D6?
Please read #61.
Thanks
#66
JoepH: The D6 fix will be in this issue, indicated by the issue tag drupal-6.15-blocker.
#67
@rfay re: #61: Uhh, please read this issue, starting at comment #31. This is absolutely a D6 regression in 6.14, and we definitely need to fix it. In fact, the only reason this issue has version "7.x-dev" is the policy of fixing in HEAD first and then backporting.
#68
subscribing
#69
So let's fix it in d7 now! I've setup a working dev-environment in a virtual machine, so I gave this a try.
@#42: The form shouldn't rebuild if there is a validation error, this was broken before this patch landed initially.
@proposed fix:
The fix was broken as 1) it triggered form rebuilds even when the form was shown the first time, 2) it tried to invoke a #submit handler for a button of type 'button', however this is the only distinction of 'button' buttons to 'submit' buttons - they don't submit the form. 1) was the cause for the wrong construction counts as I noted in #55.
So to fix I changed it to check for "$form_state['process_input']" instead 'submitted', that way the form doesn't build twice for the first page load, but also rebuilds for buttons of type button.
I also overhauled the test case to test this, however this wasn't so easy as in d7 it's not so easy to trigger form rebuilding for buttons of type button because
* just setting the storage doesn't trigger rebuilding any more
* enabling $form_state['rebuild'] during the constructor doesn't work right yet once caching is turned on, see #648170: Form constructors cannot enable form caching or form rebuilding
* submit callbacks aren't invoked for buttons of type button.
So I used a validation handler to set $form_state['rebuild']. For that I added a "Reset" button to the test, which just rebuilds the form with the previous values from storage.
However if rebuild isn't set the form would just re-load - so the same form would be presented again. This might be useful just to run the validation handlers. So I tested this case by implementing a simple "Reload" button.
Thus the test covers now
* rebuilding through a submit button
* rebuilding with a button button
* not rebuilding with a button button
#70
#71
+++ modules/simpletest/tests/form.test@@ -589,13 +621,10 @@ class FormsFormStorageTestCase extends DrupalWebTestCase {
// At this point, the form storage should contain updated values, but we do
// not see them, because the form has not been rebuilt yet due to the
- // validation error. Post again with an arbitrary 'title' (which is only
- // updated in form storage in step 1) and verify that the rebuilt form
- // contains the values of the updated form storage.
- $edit = array('title' => 'foo', 'value' => '');
- $this->drupalPost(NULL, $edit, 'Save');
- $this->assertFieldByName('title', 'title_changed', t('The altered form storage value was updated in cache and taken over.'));
- $this->assertText('Title: title_changed', t('The form storage has stored the values.'));
+ // validation error. Post again and verify that the rebuilt form contains
+ // the values of the updated form storage.
+ $this->drupalPost(NULL, array('title' => 'foo', 'value' => 'bar'), 'Save');
+ $this->assertText("The thing has been changed.", 'The altered form storage value was updated in cache and taken over.');
I really hope that this is 100% the same result, 'cause I need this to function properly in D7. It looks like it is, so I'm going to trust you. ;)
This review is powered by Dreditor.
#72
Yep, it's the same.
I had to adapt it as I changed the test-form to not 'reload' the form once it's finished, because else we would get another "form construction message" which might generate a false positive for the other tests. Also the title output reflects now the actual form value and not the storage, anyway a dedicated output ("The thing has been changed.") is easier to grasp.
#73
Given this turned out to be more complex then to be possible for D6.15, it did not actually block that release. I'd very welcome a D6 fix for this soon, right after D7.
#74
Committed to CVS HEAD. Thanks! Changing version to D6.
#75
It is actually a critical issue for Drupal 6 as well (requires followup to original patch), not at all fixed there as far as I understand.
#76
Subscribing
#77
subscribing
#78
When can we have a fix for 6.x? Since now months we have to life with a patched core...
#79
Also OpenID Provider relied on the pre 6.14 behavior. What's interesting is that a coincidentally set $_POST global concealed the bug. When $_POST was accidentally set (from a different form submission), the new form was eventually rebuilt and $form_state - on which the OpenID Provider form in question relied - was cached.
Detailed writeup here: #621956-7: Redirecting fails when not logged in on the provider site
Haven't had the time to look at #69.
#80
Here is a backport of the d7 fix (#69). As for d7, this re-enables form rebuilds for buttons of #type 'button'.
#81
Looks good - and looks like an API change at first sight, but we'd have to duplicate a very long + awkward condition otherwise.
#82
Yep, it sets the 'process_input' flag known from d7, so we can avoid this long + awkward condition. However a new flag won't break anything, so it should be fine that way.
#83
#80 still breaks a multi-step form that has been working fine on
6.146.13.I will investigate further.
edit:
on 6.14 the previous step functionality is not working when required fields are not filled in.
for 6.13 we created an ugly workaround by clearing the messages in the session. From 6.14 this not enough anymore, we will also need to set the $form_state['submitted'] = true and clear the messages.
see http://drupal.org/node/472566
off topic: is there a clean way of implementing previous step functionality that bypasses the internal required fields validation?
#84
subscribing
#85
@#83: Well clearing the messages shouldn't be enough, when it was, then because you triggered the "form rebuilds even when there are validation errors" bug fixed by this issue.
#86
Subscribing,
greetings, Martijn
#87
When comment previews are configured to be required, no one is currently able to submit a comment in D6. This patch also fixes that bug.