FAPI3 fixes: correct builder argument ordering, eliminate redundant arguments

That's just plain bizarre. We need to 1) find out why !empty() is returning TRUE, and 2) find out what's passing a TRUE into that function to begin with. I suspect that our real solution will be found in 2).

Well, as pwolanin showed, $form_state is a string. Strings may be accessed as _if_ they were arrays (they are not, however). Now, PHP tries to convert the key to an numeric index, which is, for 'storage' is 0, and returns the first character of the string (1).

The function passing this string is drupal_get_form('system_modules', [URL arg1, arg2, ...]) thanks to the menu system.

Well, that's forehead-slappingly simple. Thanks, Heine.

The question now is what's passing '1' into the function. Let's track that down -- it's a legit bug.

Heine and chx pointed out what's actually going on -- I'd missed the '1' at the end of the url. This appears to have flushed out a conceptual problem: since the menu handler passes in all path bits as arguments, it arbitrarily pushes $form_state out to the second parameter if you put in dummy URL segments. That's... bad, to say the least.

My proposed solution is to rearrange the arguments so that $form_state is always the FIRST argument passed to builder functions. Since form_state has become pretty central to the form workflow, I think it's justifiable, and it would result in the param ordering always staying predictable. Thoughts? Anyone?

Here's a patch that reshuffles things so that $form_state is always the first parameter in any form builder function. An exception is any implementation of hook_form() for node forms. Since that's handled through nodeapi rather than formAPI, the params are still $node, $form_state. Can't be helped, AFAIK.

This definitely needs testing, but I agree it makes things more consistent.

As a quick note, what this means is:

EVERY form's builder function will receive $form_state. Whether it has any data in it or not determines whether you're starting out fresh or in a middle-step of a multistep scenario.
If you pass ad hoc parameters into drupal_get_form('my_form_id', $like, $this)... those params will show up like this: my_builder_function($form_state, $adhoc1, $adhoc2)
If you use hook_forms() to add additional default parameters, you'll get them as: my_builder_function($form_state, $default1, $default2, $adhoc1, $adhoc2)

The patch did indeed fix the problem for the path: admin/build/modules/1
but it generated the following errors when going to admin/build/themes/settings:

# warning: Illegal offset type in /home/flo/workspace/Drupal-head/modules/system/system.module on line 2164.
# notice: Trying to get property of non-object in /home/flo/workspace/Drupal-head/modules/system/system.module on line 2164.
# warning: in_array() [function.in-array]: Wrong datatype for second argument in /home/flo/workspace/Drupal-head/modules/system/system.module on line 2230.

I'm not entirely clear on what this is due to though.

Thanks! A function signature had been missed. This is why testing is important on this one ;-)

Fixes the previously mentioned missing forms, corrects a number of other missed forms, and also eliminates the redundant $form_values variable in favor of $form_state['values'].

An FYI: the reason for eliminating form_values is simple. form_state contains it, and form_values was already just a reference to form_state['values']. If people want to save keystrokes in large functions, they can always $fv &= $form_state['values'].

This has been requested by almost every dev who's actually tackled porting stuff, as there is confusion over whether to use $form_state or $form_values.

Eaton : I can't roll a patch right now, but 'delayed' submit handler execution in batch.inc needs to be updated as well to reflect new submit handler arguments.
Something like : (batch.inc - line 235)

    if (isset($current_set['form_submit']) && ($function = $current_set['form_submit']) && function_exists($function)) {
      // We use our stored copies of $form and $form_state, to account for
      // possible alteration by the submit handlers.
-      $function($batch['form'], $batch['form_state'], $batch['form_state']['values']);
+      $function($batch['form'], $batch['form_state']);
    }
    return TRUE;

Thanks for the heads up. Re-rolled. :-)

typo in _form_set_value() flushed out by frando. thanks!

The installer's site config page is still not working - lots of notices from user.module (mail, name, pass missing indexes) and it doesn't redirect. Eaton is fixing it.

there's a bug in the book.module: function book_submit(&$form_state['values']) {

...Aaaaaand fixed. Thanks, guys. :) Apologies for the book related issue as well, that was in one of the earlier patches but a file was lost from one of the re-rolls due to an editor crash.

Fixed some remaining issues in the locale forms, an input filter configuration form, and search forms.

....Aaaaand a PHP4 compatability issue. Woo!

As far as I can see we are ready.

Eeeexcept for one issue with the installer I introduced. Missing function param. Thanks, hunmonk!

Color module issue fixed. Right now we're down to straggler forms that bypass the normal FAPI processing (by manually calling their own builder functions in hook_form_alter(), stuff like that).

And finally, changing the title of this patch to more accurately reflect its current state.

Committed to CVS HEAD. Marking this as 'code needs work' until this the documentation has been updated to reflect this.

Thanks Eaton. :)

Missing one-liner in batch.inc - see my comment in #14 (apparently Eaton was supposed to include it in #15 but seems to have left it out, and I did not actually check...)

Whoops. Missed the taxonomy editing form and the... um.

User login form.

That was working earlier. Honest. ;)

There are still some issues with confirm form that hunmonk and I are tracking down, but these two fixes are pretty critical.

This fixes user login, taxonomy, and the assorted confirm forms that were missed around the site.

Handful of files missed that cvs diff. This has it all.

....Aaaaand an issue with comment previews.

Committed! Thanks Jeff.

Upgrade documentation and code snippets at http://drupal.org/node/144132 have been updated to match the latest and greatest. Thanks!

Is this really fixed? I've attached a small testcase module that shows some odd parameter duplication.

In my testing, the second page of a multi-step form seems to have a weirdly duplicated $form_state passed to it as a parameter. In short, it's like this:

First Page: form_builder(&$form_state)
Second Page: form_builder(&$form_state, &$wierd_state)
Third Page: form_builder(&$form_state)

I think $wierd_state is actually the previous $form_state, but I might be wrong about that.

Clearly, this problem presents some challenges when dealing with multistep forms that have extra parameters passed to them!

About the testcase:
It's a small Drupal 6 module that asks a few things over two pages of a multi-step form. It uses drupal_set_message to tell you what the form builder function received as arguments. On the first page, it only reports form_state. On the second page, the form builder is called once to validate the submitted data (so reports as before), but is called again to display page 2 of the form. It's here that it reports the extra parameter.

Yep - there's a bug in the way FAPI calls form builders in multi-step forms. I've attached a patch that fixes the problem (more talented devs than me may prefer to solve the problem another way, I don't know...?).

The bug is actually quite a simple one. On submit, FAPI looks to see if it's got the form cached, if not, it has to create it from scratch. To do this, it calls the form builder using the same arguments it was called with when it first generated the page the user's just filled in. In so doing, it manipulates the $args array (created when the drupal_get_form() function starts). It then correctly calls the form builder and moves on.

The next stage is to deliver the second page of the form to the user. Again, drupal_get_form() manipulates $args, but this time they're not as they were when the function was called (because of the above). This is where the bug can be seen in action.

On third or later calls, the form is cached, so the first manipulation of $args does not take place. As such, the next stage is unaffected, and so the bug is not visible.

My patch just makes a copy of the $args array before doing the manipulations on it. That way, $args itself is left as it intended for later use.

Just ran this through, and existing complex forms (like module uninstallation, poll, etc) work smoothly. This was a very good catch -- thanks, coofercat!

Committed to CVS HEAD. Thanks.