Allow to specify a 'wrapper callback' before executing a form builder function

I'm not really happy with this.

If those multi-step forms already use different arguments, and they already presume that there are certain form elements in the passed $form, couldn't we just pass on the 'wrapper_form' generated by the 'wrapper callback' inside of $form_state and let the form builder functions figure out how to use it? i.e. they'd start with

  $form = $form_state['wrapper_form'];

Leveraging the $form_state['build_info'] from #367567: Use AJAX framework for "Add more" / load include file containing form / introduce $form_state['build_info'] would restore that wrapper form information properly from cache when a form is being rebuilt from cache.

Hm.

I wonder why wizard.inc can't route all multi-step form builds through itself, prebuilding any form elements upfront, calling the (special) respective multi-step form builder callback (using those different arguments), and also auto-apply any #validate + #submit handlers (if existent) for that multi-step form builder callback?

At least, the concept sounds similar to node forms (http://api.drupal.org/api/function/node_form/7), where one form builder callback wraps an arbitrary other form. Leveraging hook_forms() (http://api.drupal.org/api/function/node_forms/7) to auto-register/route all multi-step form builder callbacks to a wizard_form().

?

This one should at least get "somewhere".

Additional benefit: This patch brings consistency into form builder functions and form validate/submit handlers. All of them take ($form, &$form_state) as arguments now. Great for DX.

1) Converting all core forms alone to take $form as the first argument is a monster job. I had to manually fix a couple of pass-through functions like http://api.drupal.org/api/function/user_user_form/7 to account for that change, but I obviously didn't catch all of them.

2) As there is no form builder function in core that uses a wrapper callback/form, we are safe to keep those $form re-initializations in the form builder functions. Definitely a novice issue to wander through all of core and change/remove those accordingly.

3) Adding $form as the first argument brings consistency into the function signature of all form build/validate/submit handlers. For me, that is the only reason for accepting the 'wrapper callback' addition in D7. In D8, we should re-consider this addition along with re-considering implementations like aforementioned user_user_form() as well as http://api.drupal.org/api/function/node_form/7, which all kinda tackle a similar issue: Invoking a specific $form_id, but wrapping that form builder into another form builder. Currently, both user forms and node forms use custom API callbacks to pass on a prefilled $form.

4) Tests for this can certainly be added, but before that happens, the testbot should pass on the current code. Fixing that will require quite some work, so it would be good have some early core committer feedback on 1) - 3).

This one should get at least beyond the installer....

oh boy, this one should get a bit further.

This one should pass.

Final patch?

Latest patch is still green. I can't mark my own patch as RTBC...

Summary:

1) Adds $form as the first argument to all form builder functions in Drupal. This means that the primary form handling functions now share the same arguments:

function mymodule_awesome_form($form, &$form_state) {
  ...
}

function mymodule_awesome_form_validate($form, &$form_state) {
  ...
}

function mymodule_awesome_form_submit($form, &$form_state) {
  ...
}

2) Adds the originally requested facility of a wrapping form builder function defined in $form_state['wrapper callback']. When defined, drupal_retrieve_form() invokes that function first to allow a form to be prepopulated with a form structure, which is then passed on as $form to the actual form builder (usually $form_id). Documentation for this is added to the existing $form_state documentation in the PHPDoc of drupal_build_form().

3) Adds tests to ensure the 'wrapper callback' functionality.

I think chx confirmed in IRC that he's ok with this patch.

Attached patch clarifies the API docs in drupal_retrieve_form(), where the 'wrapper callback' is actually invoked. The added test is just ensuring that functionality. In case anyone ever wants to refactor that functionality, then he/she really should be able to learn about what exactly 'wrapper callback' is for right within drupal_retrieve_form(), and not within an arbitrary test that just disapproves the killing concept he/she tried to apply - i.e. when it's too late, after people thought that they could revamp that API.

Replacing all $form = array(); initializations (where possible).

Let's see whether this comes back green. ;)

Fixing that failing Color module code. Should come back green now.

chx mentioned a few more instances of $form = array();, which could be converted. Attached patch converts those.

Note that there are still some more left, but those cannot be converted, since they belong to hook implementations that apparently do not get a $form passed.

The altered color_scheme_form() in this patch is basically also one of those implementations, but since $form_state was already passed to that "implementation", my earlier patch added $form there as well. That, however, broke tests, because the passed in $form was duplicated into the original $form, so I simply reset $form previously. This patch changes the passed in $form to $complete_form to clarify that.

Overall, however, this leads to an interesting point: We also have implementations like user_user_form(), which also do not get the original, complete form passed. Like color_scheme_form(), hook_user_form() implementations are only supposed to add to the user edit form, without having a clue about the rest of the form. They neither get $form nor $form_state. So to actually do something funky, modules have to implement hook_form_alter() to alter that form. The same applies to other instances, where this pattern is applied, such as node forms. This really sounds like something worth to explore, but I already know that webchick will kill me if I apply the "API clean-up" tag to that... 8)

I'm killing hook_user_form() + hook_user_register() in #118345: Revamp hook_user_form/_register/_validate/_submit/_insert/_update currently. Almost done.

sorry for rtbc'ing my own patch, but this is a no-brainer follow-up patch.

ok, re-rolled.

But, this time, also adding a pretty interesting little change, that may have been the missing puzzle piece before:

Let's allow hook_forms() to also assign this new $form_state['wrapper_callback'] in case it is not defined already. This allows the "regular Drupal developer" to use this concept by just using the tools he knows already: hook_forms() + drupal_get_form(). Quite potentially, this really was the missing piece.

If today was not today and API freeze not "tomorrow", then I could even see Drupal core using this thingy. Specifically, I'm working on #118345: Revamp hook_user_form/_register/_validate/_submit/_insert/_update and I now realized that user_register() and user_profile_form() are exactly such wrappers around user_edit_form().

In addition to that, we also want to pass form builder arguments to the 'wrapper callback' ;)

Just updated the relevant documentation of hook_forms() accordingly.

Also created spin-off: #597108: Consistent array key syntax in $form_state (underscores vs. spaces)

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

Revamped the docs of hook_forms().

But, to be honest, we don't know the actual real-world possibilities and/or limitations of this yet. :) Using the wrapper callback to setup common form elements for wizard-alike things is the use-case of CTools, but given this tiny enhancement, I doubt that it will be the most common/simple thing you can do with this.

Experiments in contrib will tell :)

+++ modules/simpletest/tests/form.test	9 Oct 2009 17:51:10 -0000
@@ -305,11 +305,9 @@ class FormsElementsTableSelectFunctional
   private function formSubmitHelper($form_element, $edit) {
     $form_id = $this->randomName();
-
     $form_state = form_state_defaults();
-    $form = array();
+    $form = $form_element;
 
-    $form = array_merge($form, $form_element);

So why do we copy $form_element into $form, you ask?

Well, I asked me that, too.

Nonsense.

+++ modules/system/system.api.php	9 Oct 2009 18:36:37 -0000
@@ -573,9 +573,11 @@ function hook_form_FORM_ID_alter(&$form,
+ * in 'callback'. In case the code that calls drupal_get_form() passes

s/passes/also passes/

+++ modules/system/system.api.php	9 Oct 2009 18:36:37 -0000
@@ -583,17 +585,45 @@ function hook_form_FORM_ID_alter(&$form,
+ *   An array containing the original arguments provided to drupal_get_form()
+ *   for reference. These are always passed to the form builder and do not have

"for reference" sounds like "by reference", so let's drop that.

This review is powered by Dreditor.

ah, yeah, sun is right. Fixed those issues. :)

Yes. The follow-up patch is mainly about making the functionality we already added available to more developers who are just used to define 'drupal_get_form' in their menu callbacks and implement hook_forms(), because both are API constructs they know and used already.

So the idea of making 'wrapper_callback' available to hook_forms() is that many more use-cases could be possible. We don't know what's possible yet, because CTools was the only module thus far that implemented this concept, and because it used the more advanced implementation of calling drupal_build_form() with a customized $form_state - which is something that most developers will most likely not grok.

As mentioned before, we even have use-cases in core for this. user_register_form() + user_profile_form() are exactly such wrappers around user_account_form(). I doubt that I'll have the time to convert those, but it clearly shows that there are plenty of use-cases for this facility.

In short: The follow-up patch just exposes an internal Form API feature to the masses. If the docs are really insufficient, I'll try to find someone who's able to improve them.

I really asked around, and several people said they would help on the docs, but didn't show up here.

So I revamped the docs for hook_forms() once more and I now have little idea on how they could be improved further.

Next to the slight hook_forms() feature addition, this patch contains a fix for passing form builder arguments also to the wrapper callback, which is required for #367006-18: [meta] Field attach API integration for entity forms is ungrokable. Yes, I know that sounds strange, but you can read the comment I linked to grok why.

#367567: Use AJAX framework for "Add more" / load include file containing form / introduce $form_state['build_info'] should be committed first - afterwards this one needs a quick re-roll to apply again.

Guys, we need this patch to move forward in #367006: [meta] Field attach API integration for entity forms is ungrokable. If absolutely required, then I can remove the change to hook_forms() allowing to specify a wrapper callback for unknown form ids. However, I really think that this nifty piece will allow for some interesting code in contrib during D7.

We need at least the other parts of this patch, because the current flow of Form API + Field Attach API integration is totally ungrokable for everyone. The important piece of this patch is the passing of form builder arguments to the wrapper callback.

Please note that this patch contains conflicting changes with #367567: Use AJAX framework for "Add more" / load include file containing form / introduce $form_state['build_info']. Both are highly critical at the moment, but either one can be re-rolled easily after the other one went in.

.