By mikeschinkel on

I'm working with v5.7 and a simple use-case for a #multistep form but am running into a strange behavior and I'm wondering if I'm doing something wrong or if there is some reason that it should work this way that I'm just not realizing.

The use-case is a simple magazine subscription form with three steps:

1.) Delivery info & Credit card info and Continue Button,
2.) Confirmation Page and Confirm Button, and
3.) Thanks Page

The behavior that I am questioning is what happens during a page refresh. After submitting information on step #1 and the step #2 Confirmation page is displayed, if I tell the browser to refresh the confirmation page (using F5) and tell it to "Retry" it reloads the page #1 (the Delivery info & Credit card info page) instead of page #2 (the Confirmation page.)

From a user's perspective, this behavior could be quite frustrating and certainly not what I would expect as a use. I've attempted to debug it but setting up the test case is tedious and the form process is complex so I haven't been able to figure out if it is something I'm doing wrong, if it is a bug in the form API, or if it for some reason is supposed to work that way.

I'd really appreciate any insight you could give me on this.

⋅ Categories: Drupal 5.x

Comments

mikeschinkel’s picture

mikeschinkel commented

Okay...if I comment out the "unset()" on line 90 of form.inc, it works as expected; i.e. I press F5 to refresh on the Continue page, and the Continue page is reloaded. Why?

I've copied the drupal_get_form() function from form.inc below with that unset() line commented out to illustrate the line that is causing the behaviour I'm trying to avoid:

function drupal_get_form($form_id) {
	// In multi-step form scenarios, the incoming $_POST values are not
	// necessarily intended for the current form. We need to build
	// a copy of the previously built form for validation and processing,
	// then go on to the one that was requested if everything works.

	$form_build_id = md5(mt_rand());
	if (isset($_POST['form_build_id']) && isset($_SESSION['form'][$_POST['form_build_id']]['args']) && $_POST['form_id'] == $form_id) {
		// There's a previously stored multi-step form. We should handle
		// IT first.
		$stored = TRUE;
		$args = $_SESSION['form'][$_POST['form_build_id']]['args'];
		$form = call_user_func_array('drupal_retrieve_form', $args);
		$form['#build_id'] = $_POST['form_build_id'];
	}
	else {
		// We're coming in fresh; build things as they would be. If the
		// form's #multistep flag is set, store the build parameters so
		// the same form can be reconstituted for validation.
		$args = func_get_args();
		$form = call_user_func_array('drupal_retrieve_form', $args);
		if (isset($form['#multistep']) && $form['#multistep']) {
			// Clean up old multistep form session data.
			_drupal_clean_form_sessions();
			$_SESSION['form'][$form_build_id] = array('timestamp' => time(), 'args' => $args);
			$form['#build_id'] = $form_build_id;
		}
		$stored = FALSE;
	}

	// Process the form, submit it, and store any errors if necessary.
	drupal_process_form($args[0], $form);

	if ($stored && !form_get_errors()) {
		// If it's a stored form and there were no errors, we processed the
		// stored form successfully. Now we need to build the form that was
		// actually requested. We always pass in the current $_POST values
		// to the builder function, as values from one stage of a multistep
		// form can determine how subsequent steps are displayed.
		$args = func_get_args();
		$args[] = $_POST;
		$form = call_user_func_array('drupal_retrieve_form', $args);
//		unset($_SESSION['form'][$_POST['form_build_id']]);
		if (isset($form['#multistep']) && $form['#multistep']) {
			$_SESSION['form'][$form_build_id] = array('timestamp' => time(), 'args' => $args);
			$form['#build_id'] = $form_build_id;
		}
		drupal_prepare_form($args[0], $form);
	}

	return drupal_render_form($args[0], $form);
}

What is the reason for this unset()? Is it to force the form back to the initial state, or is it a bug? I'm hoping it's a bug but I'm fearing it might be some sort of security feature.

Help?

criznach’s picture

criznach commented

Multistep forms are definitely tricky. After using multistep for a couple of projects, I'd recommend just using several single step forms and storing the data in $_SESSION. That way each step can be a unique path and the back button and refresh work. Just test that each step came from the previous step and unset your session variables after the final submit.

http://www.trailheadinteractive.com

mikeschinkel’s picture

mikeschinkel commented

Thanks for the reply.

If it weren't for this one problem, I'd say that the #multistep forms have been pretty easy. But after posting this I started thinking about doing it exactly as you suggest, with multiple paths, i.e. /subscribe, /subscribe/confirm, and /subscribe/thanks; my code even already has the different forms broken out into their own functions, so why not?

Still, it would be nice to learn if this is actually the designed behavior for some security reason that escapes me, or if in-fact I have found a bug.

OTOH, as for using $_SESSION, I really don't like the fact that $form uses $_SESSION as it's not very scalable. Much better IMO to store the info in a text file as that can scale far greater, and wouldn't require scanning through the entire $_SESSION['form'] array to remove expired forms; that task could be delegated to the file handling of the operating system.

For a high traffic site with lots of commonly used #multistep forms, $_SESSION['form'] could have tens of thousands of elements after 24 hours and IMO that's not a great use of server memory when the same would take up a trivial amount of disk space as test files.