button broken due to fix various problems when using form storage

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

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 :)

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?

Subscribing.

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

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.

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.

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!

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

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?

#13 still applies and still works for me on 6.x-dev.

Committed to Drupal 6 too, thanks.

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!

subscribe

!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.

To clarify, by "buttons" we mean FAPI form elements of #type 'button', like core's "Preview" button on comment forms...

yeah, looks wrong.

However, comment previews are working in HEAD, so this doesn't seem to be covered by tests.

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...

Like that? Not sure.

#type = button.

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.

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. ;)

This change also broke multistep forms in Drupal 6.14. If '#button' is used, $form_state['values'] is empty.

Also created: #591696: Problem with various AHAH forms and validation

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.

Tagging

+

Which patch should I apply to 6.14 to fix this?

The one from #43 works for our 6.14.

Does this fix the bug where button D was pressed but the function for button A was called instead? For 6.14

This multistep form bug also effects a custom module that we have for a customer site. Looking for ways of working around it currently.

Fixed the tests.

+++ 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.

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.

.

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!

@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.

@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.

I apologize for the misdirection and error. I am so used to things going into 7 and then working their way into 6.

subscribe (coming from #638758: Fixes and improvements to emthumb).

Would it be a good idea to create a new issue for D6?
Please read #61.

Thanks

JoepH: The D6 fix will be in this issue, indicated by the issue tag drupal-6.15-blocker.

@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.

subscribing

+++ 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.

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.

Committed to CVS HEAD. Thanks! Changing version to D6.

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.

Subscribing

subscribing

When can we have a fix for 6.x? Since now months we have to life with a patched core...

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.

Looks good - and looks like an API change at first sight, but we'd have to duplicate a very long + awkward condition otherwise.

#80 still breaks a multi-step form that has been working fine on 6.14 6.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?

subscribing

Subscribing,
greetings, Martijn

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.

Subscribing.

Thanks, committed to Drupal 6.

Reopening. There is a discrepancy between the code and the intention expressed in the comment:

  // If $form_state['rebuild'] has been set and input has been processed, we
  // know that we're in a multi-part process of some sort and the form's
  // workflow is not complete. We need to construct a fresh copy of the form,
  // passing in the latest $form_state in addition to any other variables passed
  // into drupal_get_form().
  if ($form_state['rebuild'] && $form_state['process_input'] && !form_get_errors()) {
    $form = drupal_rebuild_form($form_id, $form_state);
  }

The intent here is to rebuild only if the form has been processed and passed validation. But !form_get_errors() means something set a form error. Especially, it means that you cannot set a non-rebuild-preventing error in a submit handler, just before rebuilding the form. I suggest we store the information "we executed form_execute_handlers('submit', $form, $form_state);" somewhere in the form state, and use that here.

it means that you cannot set a non-rebuild-preventing error in a submit handler

Can you elaborate on that? It doesn't make sense to me - form errors need to be set during form validation, not in submit handlers. We are only allowed to rebuild after successful submit. Any validation errors need to be handled already.

What if I want to display an error message, without preventing the form from being submitted/rebuilt?

As of now, we don't have a concept for pseudo-error, warnings, and recommendation messages. That's a very dusty area, as we've learned from #limit_validation_errors and all the related trouble with entities built off from in-edit data in $form_state. I'm not sure what the use-case at hand is, but you can always invoke drupal_set_message('Message', 'error') manually -- in the end, for a pseudo error message, it would be the same as calling form_set_error(), but just without preventing the form from successfully validating and submitting.