| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | forms system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (works as designed) |
Issue Summary
The new FAPI feature of $form_state['storage'] is supposed to provide developers with a free-form place to store data between form views and submits. So far, it seems to be doing this rather well, except when first displaying the form for the first time. This problem is mainly a problem when dealing with multi-step forms that allow something to be edited.
I'm working on a testcase, but that's likely to be complicated and take a bit of time. However, imagine this scenario:
1) User enters multi-step form for the first time, and fills in page 1
2) User submits, so the module stores step 1 data in $form_state['storage'] before displaying step 2
3) User fills in step 2
4) User submits, so module adds the received form values with those stored in (2). This is then saved to the database.
At some later point, the user wants to edit the thing saved in step (4). To do this, the module loads the data stored in step (4) above. This is then used to "seed" the form, so that the user can modify their original selections. The pseudo-code of the module's form function would be something like:
function mymodule_form($&form_state, $filename) {
if is_first_call then {
$step = 1;
if isset($filename) then {
$data = load_form_config($filename);
$form_state['storage'] = $data;
}
} else {
$step = get_step_from_form($form_state);
}
$form = get_form($form_state, $step);
return $form;
}...the problem is that this doesn't work - the data needs to be loaded twice - once for the first display of the form, and then again at some later point (most probably during a submit, or possibly on next page load).
It seems that if your function is called by drupal_get_form(), it should pass you a $form_state array, and honor the 'storage' sub-array at the very least. Currently that doesn't seem to be the case.
Comments
#1
I've attached a testcase that demonstrates the problem.
The testcase is a multi-step form. The form builder function, and the submit handler set values in $form_state['storage'] as follows:
Step 1 (first view): sets fruit => banana
Submit: sets drink => beer
Step 2 just displays some stuff, but doesn't actually set anything
Submit: (re)sets drink => beer
Step 1 (second view): sets food => sandwich
(and so on)
The form displays what is set or unset in $form_values['storage']. The only time it ever shows "banana" is on first view of step 1. It shows "beer" on every page after the first submit, and shows "sandwich" on every page after it's set on second view of step 1.
In short: Storage set when a form builder is first called is lost. All subsequent usage of storage (in validate, submit or form builders) is correctly stored. Looks like a bug to me ;-)
#2
This problem is due to call_user_func_array() NOT passing by reference as you might expect (see about half way down on http://uk2.php.net/call_user_func_array for details)
For example, this DOES NOT WORK:
<?phparray_shift($args);
array_unshift($args, $form_state);
array_unshift($args, $form_id);
$form = call_user_func_array('drupal_retrieve_form', $args);
?>
...and must instead be written as:
<?phparray_shift($args);
$form = call_user_func_array('drupal_retrieve_form', array($form_id, &$form_state) + $args);
?>
This mistaken use of call_user_func_array() is all through includes/form.inc, and sometimes it's resolution is more involved than I really understand. For example, in drupal_get_form(), there are two places where the form can be fetched from a builder function. The first is used on first call, and the second is only called if $form_state['storage'] is empty. Clearly, if that first call puts things in storage, then this breaks.
In short, this is looking like it may be a fairly involved bit of form.inc hacking. Also see my comments towards the bottom of http://drupal.org/node/146667 for related issues in form.inc. The testcase I attached earlier might help work things out.
I'm looking at a patch, but in truth, I think it's a bit beyond my understanding of FAPI. I need some help from someone who knows this stuff properly.
--
If it matters, here's some of my system info:
Linux 2.6.20-1.2952.fc6 #1 SMP Wed May 16 18:59:18 EDT 2007 i686
Apache/2.2.4 (Fedora)
MySQL 5.0.27
PHP Version 5.1.6
#3
Title changed as the problem is actually more prevalent than just first form call. Most circumstances where the problem occurs are a little more obscure than "normal" people might find due to the call_user_func_array() problem.
#4
I found a bug that leads to those same lines of code. The comment controls don't remember the state since the first argument here ends up getting converted into an array when the form is called up. It should be a string or integer.
<?phpfunction comment_controls($mode = COMMENT_MODE_THREADED_EXPANDED, $order = COMMENT_ORDER_NEWEST_FIRST, $comments_per_page = 50);
?>
$order, and $comments_per_page end up with the right values. It's just that first one.
I wouldn't know how to fix this myself.
#5
I'm not sure if that comment issue is actually related or not. I haven't looked in proper detail, but I'm wondering if it's more related to FAPI's argument ordering (see http://drupal.org/node/146667) than FAPI failing to call functions with references.
In truth, I don't know, so I should probably just keep quiet ;-)
#6
form_state['storage'] is ought to be changed in submit handler not in builder.
#7
Whilst one may be able to argue that $form_state['storage'] should only be usable after the first submit of the form, this does not address the scenario described - modules have to load multi-step form seeding data twice, which is (a) pointless and (b) unnecessary if form.inc uses call_user_func_array() properly. Also, form builders are passed a $form_state structure, so FAPI really ought to honour it's use (or not pass it in the first place).
Everywhere form.inc uses call_user_func_array(), expecting to pass $form_state by reference is actually a bug. It DOES NOT pass by reference (see http://uk2.php.net/call_user_func_array) without special handling. This causes a demonstrable problem with the "double load" issue, and doubtless other subtle, hard to find, possibly obscure problems, most likely with multi-step forms (which by their nature are hard to fully test).
#8
Whilst one may be able to argue that $form_state['storage'] should only be usable after the first submit of the form, this does not address the scenario described - modules have to load multi-step form seeding data twice, which is (a) pointless and (b) unnecessary if form.inc uses call_user_func_array() properly. Also, form builders are passed a $form_state structure, so FAPI really ought to honour it's use (or not pass it in the first place).
Everywhere form.inc uses call_user_func_array(), expecting to pass $form_state by reference is actually a bug. It DOES NOT pass by reference (see http://uk2.php.net/call_user_func_array) without special handling. This causes a demonstrable problem with the "double load" issue, and doubtless other subtle, hard to find, possibly obscure problems, most likely with multi-step forms (which by their nature are hard to fully test).
#9
Form builders are passed the form state because that is what they use to determine what they should build. Think long and hard over how they would determine the current step if they were passed no state information.
#10
It is still by design that the builder should not change the state.
#11
There are still bugs in form.inc (wherever call_user_func_array() is used), but it seems I'm not going to get anywhere in fixing them (not being qualified to patch them myself).
As for the other, I've opened a Feature Request for v7.x as it seems we've got as far as we're going to with this in v6.x. http://drupal.org/node/179566
#12
I'm newbie to all this but as I was tracing the 6.8 code I noticed this:
// If $form_state['storage'] or $form_state['rebuild'] have been
// set by any submit or validate handlers, however, we know that
// we're in a complex 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 (!empty($form_state['rebuild']) || !empty($form_state['storage'])) {
$form = drupal_rebuild_form($form_id, $form_state, $args);
}
I would question the logic (probably because I don't understand) of systematically rebuilding a form if empty($form_state['storage'] is non empty. I would rather the form programmer explicitly set the rebuild flag. If I want to store any data in $form_state['storage'] when my form is first build, why should it be rebuild automatically? I just want to use storage instead of hidden fields. My form would not be rebuilt if I had used hidden fields.