When multistep was first written, we had an unset($_POST['edit']) and that stopped running validate and submit on the new form. As this should not happen, we now explicitely tell drupal_process_form that this is a multistep form and how to act on it.

This is critical because at certain circumstances this can cause submit to be called without validating. Thanks to neclimdul for catching this.

CommentFileSizeAuthor
#6 multistep_2.patch382 byteschx
#3 multistep_1.patch2.08 KBchx
multistep_0.patch1.65 KBchx

Comments

neclimdul’s picture

neclimdul
He/Him
commented

Thanks, this fixes my problem.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs review » Reviewed & tested by the community

This does exatly as it says and fixes the problem. Looks good so RTBC.

chx’s picture

chx commented
StatusFileSize
new multistep_1.patch2.08 KB

On Steven's advice, the variable is renamed better doxygen is used and I found out some defines, too. Functionality is the same, just it's easier to understand.

AjK’s picture

AjK commented

Applied and tested, looks good. I'd mark critical and RTBC but it already is.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

Code looks screwy (line-breaks). Looks a bit hack-ish to me.

chx’s picture

chx commented
Status: Needs work » Needs review
StatusFileSize
new multistep_2.patch382 bytes

Dries, In your wisdom we are humbled -- this was a hack indeed. Let's see this version.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

adding to my review queue.

eaton’s picture

eaton commented

Quite the simple fix -- and it seems to work well on my 5.0 test site. I've run it htrough several multistep and single-step forms and all works as expected. Further testing to follow..

eaton’s picture

eaton commented
Status: Needs review » Reviewed & tested by the community

That includes *commenting on a poll* btw, the case the most obviously broke theform handling code. I'm RTBC'ing this.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Needs review

I think this is a good idea, but am having trouble understanding it.

What exact problem is reproducible? The original post only mentions "certain circumstances." The last update mentions "commenting on a poll," but I couldn't find any issues with this. Do I need to change my commenting options?

The original post says "can cause submit to be called without validating," but the actual patch seems to remove a validation step.

eaton’s picture

eaton commented

drumm,

My comment about comments was a bit of a red herring; it was instead a reference to some behaviour that we were trying to avoid (problems with multiple forms on a page simultaneously...) Sorry for the confusion.

Here's a quick overview of the problem:

1) I create a multistep form, and pop it open in my browser. I fill out all the fields and click SUBMIT.
2) drupal_get_form() reconstructs the first step of the form and calls drupal_process_form(), validating and submitting the data.
3) drupal_get_form() now builds the second step of the form -- and calls drupal_process_form() again. Because both steps share a form_id, the validation code thinks that this step has already been validated. It skips validation, and hops directly to the submit function. Whoops.

In reality, we should never be doing the validate/submit cycle for that second display-only form case. Thus, we can replace drupal_process_form() with drupal_prepare_form() in that portion of drupal_get_form(). That preps it for rendering, but does NOT validate or submit any of its fields.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

drupal_process_form() again. Because both steps share a form_id, the validation code thinks that this step has already been validated. It skips validation, and hops directly to the submit function. Whoops.

In reality, we should never be doing the validate/submit cycle for that second display-only form case.

Note that it is not a solution to not skip validation the solution is not to run validation/submit at all

chx’s picture

chx commented

More, because my explanation does not say "why".There is excellent explanation at http://jeff.viapositiva.net/drupal/dynamic-forms

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)