Previewing multiselect taxonomy fields lose value on multistep node forms

Actually, I can only reproduce this bug if drupalorg.module is enabled. Moving this to the right queue. Also, this is a regression from the d.o upgrade -- tagging appropriately.

Turns out this is a core bug. Attached is a small module, "twostepnode", that implements a trivial two-step node form (title on page 1, body and everything else on page 2) which exhibits the same bug. To reproduce:

1. Download these files to sites/all/modules/twostepnode, rename appropriately, and enable
2. Configure a vocabulary that uses multiselect and associate it with this node type
3. visit node/add/twostepnode
4. Enter a title and press 'Next'
5. Select some terms from your vocabulary, but don't type anything in the 'Required body' field
6. Try to preview or save -- notice that the terms you selected are now gone when the form is rendered for the validation error

chx promised that if I wrote and posted such a module for the test case, that he'd fix the bug. ;)

forgive me, webchick, for i have sinned. ;)

On validate, the node form submit handler does not run, form_state[node] is not populated and the taxonomy gets lost. title stays only because the module reads from form_state[storage]. We might need to move form_state[node] to form_state[storage][node].

The first hunk in there (the only one that matters) applies to D6, and indeed fixes the bug, both via twostepnode.module and via project_release.module + cvs.module. It makes sense why this now works -- since we don't rebuild the form on a validation error, we don't destroy $form_state['node'] and we save the values. However, I won't pretend like I understand every far-flung corner of FAPI well enough to know if there are going to be places that are broken if forms don't rebuild on validation errors. This still needs more review and testing.

Lest anyone freak out about the patch "failing to apply", here's a reroll with the actual code change and a reasonable patch name.

Tested this out on the Quick Tabs admin form, adding elements via ahah, failing validation - everything worked as it should.

Bot, what do you think?

The full monty: the fix and dww's module pimped out with hook_perm and access and turned into a test module and added a test. I checked, without the form.inc fix we get 17 passes and 1 fail, with the test we get 18 passes. The only question, is this a node test or a form.inc test ? I voted on node...

As per chx's request I'll be taking a closer look at this, hopefully tomorrow. In the meantime, an attachment here captures some useful data.

Some observations. Your form can either be cached or you are building it based on form_state. If it's cached, the patch does not change anything if I am not mistaken (Eaton, please reassure me -- I am very wary of this patch). If you are building then before the patch you needed to re-build once the form for the submit handler. Now you need to be able to build N times because of validation errors. Should you deplete some resource (like some remote API call quota) then this patch will cause failure but then why you are not caching your form? I do not feel a tremendous change going from 2 to N.

Subscribe. Will take a look at this later. Eaton, could you answer chx's question?

We discussed this with Eaton and I am marking this as RTBC as noone reviews it but there is a nice and big test.

On a very cursory overview (i'd still like someone who is not chx to give this a thorough technical review), there are some minor things:

* TwoStepNodeTestCase missing PHPDoc
* all testXX functions missing PHPDoc
* + * Implementation of hook_perm. and + * Implementation of hook_access. are missing parentheses.

Added doxygen for function testTwoStepNode and fixed the doxygen for hook_perm and access (which i wrote. the rest is dww's and those do not need fixing.)

The class was not! As a penitence I have written a small novel about it.

D'oh, new files left out.

Still needs review. Peter said he would look tomorrow.

That was another patch. Noone will look at this. There is a test that should be enough.

It was this patch - I'll give it another look. I did look at the patch once before based on dww's comments re: the bug on d.o

fwiw:

This is the only actual change to core:

--- includes/form.inc	2009-04-11 22:19:44 +0000
+++ includes/form.inc	2009-04-16 02:32:21 +0000
@@ -208,7 +208,7 @@ function drupal_build_form($form_id, &$f
   // the form, passing in the latest $form_state in addition to any
   // other variables passed into drupal_get_form().
 
-  if (!empty($form_state['rebuild']) || !empty($form_state['storage'])) {
+  if (!form_get_errors() && (!empty($form_state['rebuild']) || !empty($form_state['storage']))) {
     $form = drupal_rebuild_form($form_id, $form_state);
   }

Basically, we do not completely rebuild the form from scratch if there are validation errors. The reason rebuilding the form from scratch on validation errors is a problem is that the pseudo $node object passed in to node forms ends up being lost during the rebuild, and that's why multi-page node forms break. Myself, chx, eaton, and merlinofchaos have all considered this, tried to break it or find things this would break, and we all failed. The patch is still passing all core tests, further evidence that the above is a safe change to FAPI.

The rest of this patch is a test to ensure that the above change fixes the actual bug. 90% of the code for the test is the twostepnode.module which I wrote. So, I'm not really qualified to RTBC it, but yes, that code is great. ;)

I'm happy for others to consider the above change to FAPI, but I don't think you're going to do much better than chx, eaton, merlinofchaos and myself for people who know about the internals of FAPI...

Then could you guys please post that to the issue? There are no replies from merlinofchaos, and Eaton's only reply is "I'll look at this later." The last reply from you was "here's a module to reproduce the problem." It makes it appear as though chx is the only one who has reviewed the patch, and I do not feel comfortable committing any patch to form.inc, regardless of how many characters it is, with only one set of eyes on it.

Also, could you clarify whether the patch you've all agreed to is the latest one or the one from #8?

Edit Nevermind, I see what you're saying. The one-liner fix is the same, the test is different.

The change looks reasonable, however, the big comment on top of the rebuild condition has *not* been updated. It should capture what dww described in #25.

Subscribing - I would like to see this fix in Drupal 6

The patch from #25 no longer applies, and -- even more interesting -- the test now runs properly without the changed line of code.

It appears that #322344 may have changed the code already, adding a check for form_get_errors(). Can anyone else confirm?

Subscribing - I would like to see this fix in D6, as well.

@chx says this solves the form validation problem we have at #428744: Make the main page content a real block.
@eaton: I tried to find this directly and indirectly in your mentioned patch at #322344: (notice) Form improvements from Views, but via just textual patch review, I did not find it. It could still very well be there.

@chx suggested me to roll this change into my patch at #428644: Problem in email system, but when looking at current code (versus the patch above), I see this change seems to have been already applied:

  if ((!empty($form_state['storage']) || !empty($form_state['rebuild'])) && !empty($form_state['submitted']) && !form_get_errors()) {
    $form = drupal_rebuild_form($form_id, $form_state);
  }

vs. the patch particle mentioned above by dww:

--- includes/form.inc	2009-04-11 22:19:44 +0000
+++ includes/form.inc	2009-04-16 02:32:21 +0000
@@ -208,7 +208,7 @@ function drupal_build_form($form_id, &$f
   // the form, passing in the latest $form_state in addition to any
   // other variables passed into drupal_get_form().

-  if (!empty($form_state['rebuild']) || !empty($form_state['storage'])) {
+  if (!form_get_errors() && (!empty($form_state['rebuild']) || !empty($form_state['storage']))) {
     $form = drupal_rebuild_form($form_id, $form_state);
   }

Looks like the current code has even more conditions then this issue suggested originally. Also, this did not solve my problem there.

Hi,

Has anyone any tips for rolling this back to D6, as we'd like to see it fixed there. I can offer a bounty if someone is prepared to do this.

Thanks

subscribing

#12: twostepnode_test.patch queued for re-testing.

Is this queued for automated testing, or do you guys need someone to test this? If you outline what exactly needs to be tested and you just need someone besides yourself to make it RTBC post the instructions here and I can test this for you. I'm an Architect looking to get more involved but just need a nudge.

Thanks,

Barnettech

I recreated the sample module in Drupal 8 to test for this bug in comment #2 above (http://drupal.org/node/382634#comment-1306916), I'll post my code when I have a chance. In Drupal 8 I was not able to reproduce the bug. I enter the title, then hit next in the multistep form, then via cck I had added a multi select field. I choose 2 items in the multi select which leaving the required body blank. I get the validation error but do not loose the title or the multi select choices I entered. So this does not seem to be a bug in Drupal 8.

I'll post my code soon and will have a fellow Boston Drupal Meetup member review my work next week.

Attached is the drupal 8 version of the module from comment #2 above which was made to demonstrate the problem in this ticket. You'll see that the problem no longer exists in drupal 8 (which at this point is almost identical to drupal 7). At the Boston Coding Initiative on Thursday led by bryanhirsch and webchick I'll verify I can close this.

I can verify that this bug seems fixed in Drupal 8 when tested using barnettech's Drupal 8 version of the twostepnode module.

Creating title, clicking on "Next", selecting 2 items in multiselect but leaving body field blank, then clicking on either "Save" or "Preview" presents the validation error but the title and the multiselect choices remain in the form.

Thanks for the testing @ronaldmulero. So if I'm understanding correctly, the bug does not exist in Drupal 8 nor Drupal 7?

If so, we can close this now.

Yes that is correct. If you load the module that I updated in step #39 you'll see in Drupal 8 this is no longer a problem. In our Boston Meetups we're taking on Core bugs and were ordered to just test in Drupal 8, which is almost identical to Drupal 7 at this early stage.

My guess is the module I updated in step #39 is loadable in Drupal 7 as well but just change the .info file to list it as a Drupal 7 module rather than a drupal 8 module.

So the bug no longer exists in these later versons of Drupal. @ronaldmulero helped me test this to confirm it's all set.