FormAPI: Enable programmaticaly submitted forms via the pull model

I am going to test this the moment I have a chance (probably this weekend).

This is a really important patch, as it allows us to more easily programmatically create nodes and submit forms.
The manual code change is essentially a copy paste operation, into another function.

I got curious.

admin/themes works , as do most of the settings forms (apart from things like filter.module).

How is that achieved, what is the drupal_get_form('form_id', array($form),'some_callback') format about ?
In my view, all forms should be registered, whether they use callbacks or not.

I like this a lot more.

But what I thought about .. was perhaps we could use a form_ namespace , to reduce the size of the stuff needed in the _forms array.

function drupal_get_defined_forms($reset = FALSE) {
  static $forms = array;

  if (!sizeof($forms) && !$reset) {
    $functions = get_defined_functions();
    foreach ($functions['user'] as $function) {
      if (strpos($function, 'form_') === 0) {
        $form_id = substr($form, 5);
        $forms[$form_id] = array(); #anything that doesn't specify anything in the _forms array, automatically detected.
      }
    }
    $forms = module_invoke_all('forms') + $forms[$form_id]; // much smaller dataset
  }
  return $forms;
}

This would also be very similar to how we do theme('func'). I'm still not 100% sure on the difference
between form_handler and form_id. system_settings_form is now correctly handled imo though =)

The latest patch also doesn't have arguments, i noticed.

i am interested in how you think we should handle node_form. I kind of think it should be something like

  function node_forms() {
     $node_types = // whatever is needed to get this.
     foreach ($node_types as $type) {
       $forms[$type . '_node'] = array('handler' => 'node'); // ie: for form_event_node , use form_node instead.
     }
  }

hence the node form menu item for some node type would be like :

    $items[] = array(
       'path' => 'node/add/nodetype',
       'title' => t('nodetype'),
       'description' => t('Create a new node of nodetype'),
       'callback' => 'drupal_get_form',
       'callback arguments' => array('nodetype_node')
    }

Me and chx have decided that the form_ prefix is a great idea, but not for this release.
The idea is now part of the designs for fapi 2.0.

The new patch is going onto a very non invasive upgrade, and i approve of it so far. It should also be very fast, since there's not very much overhead.

This is huge.

It imposes a requirement on modules: forms arrays should be built by dedicated functions, not in the middle of other logic.

What do we get in return for that one requirement?

The ability to programmatically pull a form, populate it, validate it, and submit it without user interaction. (This is huge, and a big boon for 'remixing' existing forms)
The ability to cleanly control form workflow for multi-step forms and wizards (why? with this patch, Drupal easily pull the old form for validation, but display a new one)
Cleaner separation of form definition code from form handling logic. This is a big win.

Most modules are already 90% of the way there after the 4.7 conversion. Those that aren't (because they mix form building in with other logic) are either easy to convert, or doing very dark formapi voodoo already. The ones that are doing voodoo will be made much simpler by the flexibility this patch brings.

My only other thought is that drupal_validate_form() and drupal_submit_form() should accept an optional array of $form_values to be used instead of $_POST['edit']/global $form_values. That would allow much more straightforward use of those functions during programmatic node creation, etc.

There are minor issues -- install.php and update.php pages need to be converted, stuff like that -- but those are polishing/finishing steps. This is a huge leap in form functionality, and solves lots of architectural snafus we've encountered since 4.7 shipped.

A big, big +1.

This is a minor re-roll of the patch, converting update.php and install.php to the new mechanism. It's dirt simple. Really.

Whoops. That last patch was against the wrong directory. Please ignore, thanks! ;)

aggregator administration and block editing are both working now.

Does install_select_profile() still has to be converted?, Apache abends after going into a recursive loop if multiple install profiles exist.
I'll convert it myself but don't fully understand these form changes yet.

You're right -- I missed that when doing the conversion late last night. Doing the conversion is pretty straightforward.
The Old Way

function mymodule_page() {
  // build my form
  $output = drupal_get_form('mymodule_form', $form);
  return $output;
}

The New Way

function mymodule_page() {
  $output = drupal_get_form('mymodule_form');
  return $output;
}

function mymodule_form() {
  // build my form
  return $form;
}

If your form building function requires conditional logic, or requires some contextual information (like a particular database record), the drupal_get_form() call can have additional parameters appended to it like so:

function mymodule_page() {
  $stuff = mymodule_get_stuff();
  $output = drupal_get_form('mymodule_form', $stuff);
  return $output;
}

function mymodule_form($stuff) {
  // build my form, based on the contents of $stuff
  return $form;
}

Fix made to the install screen, should now work with multiple profiles.

I lied. now it works with multiple profiles.

A new version, rolled collaboratively with chx. Comment posting and administration now works. Some style/syntax changes are coming, but this version has the guts.

This patch fixes one of the 2 largest problems with FAPI, and opens the path to implementing many interesting pieces of functionality such as the command pattern (http://en.wikipedia.org/wiki/Command_pattern) , which will allow us to make a function which records all form submissions, and can replay them as part of macros or workflows.

Outstanding patch. I tested lots and lots of forms and only found a tiny bug - the 'i need more choices' checkbox on node/add/poll does not work.

I did not find an implementation of hook_forms() in this patch. Is that for Contrib? When do you imagine it will be used? If you can't think of a place, we may not need it.

Poll problem fixed.

Looks okay, but lets remove the hook_forms() thing and save that for a different patch since no core module needs it.

We need more PHPdoc code. I tried to review this patch without reading up on the issue discussion.

No where is explain why things work the way they work, and the paramater descriptions are bad. Here is just one example:

+ * @param $function_prefix
+ *   An optional function prefix that will be used in place of $form_id.

It doesn't explain why, how, when.

+ * Retrieves a form from a handler function, then passes it on for
+ * processing and rendering.

It describes what the function does, but not when or how you want to use it.

What is the difference between a form handler and a form id? Some stuff uses form IDs, other stuff uses form handlers, and it is unclear why. How do you know the form's form_handler, and how does it differ from the form's ID?

+  $form_id = isset($form['#form_id']) ? $form['#form_id'] : $form_handler;

For starters, we need more high-level information in the code. :)

Dries: After extensive discussions with chx, I'm working on re-rolling this patch to address some of the issues both you and drumm brought up. I think it will smooth things considerably and eliminate a lot of the confusion about the present version. Briefly, once the patch is in place, form generation/handling will follow this workflow:

1. drupal_get_form($form_id) is called
2. drupal_retrieve_form() is called, and obtains a fresh unaltered copy of the form definition array. It does this by first checking to see if $form_id exists as a function. If so, it calls it directly. If not, it looks through the data provided by hook_forms() to determine what form IDs correspond to what builder-functions. Any parameters that were passed into drupal_get_form() after the $form_id are also passed into the builder function.
3. drupal_build_form() preps the form for use by the API. (building, altering adding necessary form elements, etc.) This should really be renamed drupal_prepare_form(), to eliminate confusion.
   1. if we're accepting posted data, drupal_validate_form() is called
      1. If validation passes, drupal_submit_form() is called
      2. drupal_redirect_form() is called
4. drupal_render_form() is called.

This allows simple form building code to just call drupal_get_form($form_id), while more complex code can use the hook_forms() code to handle custom situations, like the node_form scenerios.

Further posts as the patch approaches completion.

Well, the stuff that I was doing on it is currently locked half-done on my non-responsive server, so chx has saved the day.

Thanks, Site5!

I think this patch is a huge jump over the last round that was rolled. hook_forms() IS needed, to avoid a lot of the same confusion that was present in earlier versions of the patch. I'm looking closer at the code now and will test it when my server is back online.

i tried a whole lot more forms with good success. here is a reroll with a few more comments. things i'd like to see before RTBC:

- documentation for #function_prefix
- documentation for #form_post_key
- the 'add new choices' checkbox on node/add/poll not fixed
- a php snippet gets posted here so that the CVS review team sees the benefit of programmatic form submit. until someone sees this, i think this looks like a code pushing exercise. i have one in waiting but there is a bug somewhere. eaton is chasing now.

i'm handing this issue off for now.

settings.php snuck in.

Something to make clear: I have a strong desire to refactor the flow of drupal_process_form, and to rework the $form_values handling process so that it's more straightforward to programmatically sling forms and their data around. I'd like to figure out a way to avoid the need to build #function_prefix into the form itself. And all of this code is only inches from clean core support for multi-step multi-form wizards.

But two of the other major patches that have gone in this version -- install and pre-CCK -- have been criticized for being too 'monolithic' when they could've been broken up. The reasons for this major refactoring are many, but those benefits can't be realized without it.

noticing some breakage in a few forms like content-types. new patch forthcoming.

improved the docs a bit.

chx will investigate the last bug that i can find: user register and request new password both redirect to home page without a confirm message.

thanks chx. RTBC. the attached patch is same as last time, without my settings.php.

Why are we using globals in chx' example?

What I meant with my last comment is this: having to use global variables in such a way sounds like a significant shortcoming of the APIs.

chx, You are my hero for ever and ever and ever. That solution is simpler and more beautiful than anything I'd done to try to avoid the globals.

You rock.

good progress ... this version improves the doc at the top of file.inc and removes a stray $form_post_key.

What is the easiest way to test this, and detect these bugs? How can contrib module authors identify broken or non-compatible forms?

I think we need to work on this some more. Parts of the code are not clear, and the design decisions do not seem to be documented. For example, why the menu system integration? I think we need to do a better job explaining various of the high level concepts.

For example, I can't make sense of this comment:

+  // A menu callback that returns an array always consists of a form definition. Then we push $return to 
+  // drupal_process_form(). Modules may submit forms programmatically using an analogous "push". 

It doesn't explain 'why' we use the menu system ... is it just to save code? Either way, it would be good to document that a little bit better. Isn't this going the complicate the menu system development, which is already quite difficult.

The documentation makes many assumptions about people's knowledge of Drupal, and the internal working of the forms system.

Eg.

 ... or use the regular submit model.

This is not clear and I'd like to see that be made a little more verbose. What does that mean in practical terms?

 hook_forms() is called to build a system-wide array of forms,

What is a 'system-wide array of forms'? Is that a list of form IDs? A list of pre-rendered forms?

And why do we have to mechanisms to "register" (?) a form? Why don't we always use the form_id? Why do we need the _forms hook to begin with? It would be great to see this explained better because it is hard to grok from just reading the patch' code.

+ * @param $base
+ *   An optional function prefix that will be used in place of $form_id.

This comment doesn't explain why or when you'd want to use this. If possible, provide a use-case so it becomes easier to understand. Why isn't the $form_id appropriate at all times? Is that a shortcoming of the system?

As I said earlier, and as Dries has explained, I'm not sure that we need hook_form. All it seems to be used for is adding an extra layer of abstraction that changes what set of callbacks are called.

drumm:
currently all forms are built, then pushed to the drupal_get_form() function along with a unique ID. Moving to a pull model (which allows other code to use the same forms for automated workflow, etc) means moving the form-building code to dedicated functions. hook_forms is just a registry of those, and (more importantly) it allows us to request a form simply by asking for the ID. It allows us to map multiple form ids to one builder-function with different callback parameters. For some modules it's not necessary; they can simply call drupal_get_form($form_id) and provide a function $form_id() in their code. For others, like node creation, it simplifies things immensely.

changes to contact.module broke things.

This version of the patch includes much-clarified PHPDoc comments, an expanded and more accurate header for all of form.inc, and some minor code changes that make the general form processing workflow more intuitive.

I'll quote directly from that to explain things at a high level:

The drupal_get_form() function handles retrieving, processing, and displaying a rendered HTML form for modules automatically. For example:

  // display the user registration form
  $output = drupal_get_form('user_register');

Forms can also be built and submitted programmatically without any user input using by populating $form['#post']['edit'] with values to be submitted. For example:

// register a new user
$form = drupal_retrieve_form('user_register');
$form['#post']['edit']['name'] = 'robo-user';
$form['#post']['edit']['mail'] = 'robouser@example.com';
drupal_prepare_form($form);
drupal_process_form('user_register', $form);

Calling form_get_errors() will list any validation errors that prevented the form from being submitted successfully.

The full life cycle for a form is:

1. Retrieval. In this stage, $form_id is either either called as a function to get a form array, or used to look up the proper builder-function from the results of hook_forms(). See the explanation in #58 for why hook_forms is used.
2. Preparation. The form definition array has various required properties added to it, is modified by any form_alter functions, and has either incoming $_POST data or the contents of $form['#post'] merged in correctly by the form building function.
3. Processing. If $_POST or #post data is present, it's processed. This stage is an atomic combination of:
   1. Validation. Loop through all the values to be submitted, calling defined validation functions and so on.
   2. Submission. If no validation errors are generated, call functions to persist or act on the submitted values. This step MAY also return a URL to redirect to when processing is finished.
4. Redirecting. If the processing stage returns a path to redirect to, go to it.
5. Rendering. If we get to this stage, we turn a form definition array into HTML suitable for display to the end user in a web browser.

Whew.

Regarding earlier questions, the changes to the menu system are really a convenience issue. MANY pages, especially in administrative sections, are nothing more than a wrapper function for a single drupal_get_form() question. This makes it possible to leave off the wrapper function. As forms and structured arrays become more integrated in Drupal's arhcitecture, this means less cruft.

Whoops. left out $form_id in the example reference to drupal_prepare_form()

Yeah, that all would've been great if I hadn't completely broken everything. ignore 23 and 24, they're worthless save some of the phpdoc additions.

chx is correct. with patch #35, 'Step 2, preparation' becomes a substep of 'Step 3, preparation' before the validation and submission.

I'd still like to see redirection and rendering split out and made explicit parts of drupal_get_form rather than drupal_process_form, but the patch works well again, has the workflow I explained above, has much more detailed comments, etc.

It is important to make the form API (and its raw power) accessible to developers; proper documentation with practical advice and examples is the way to go.

The documentation in the last patch is already significantly better.

Maybe change:

+ * // display the user registration form
+ * $output = drupal_get_form('user_register');

to

+ * // display the user registration form
+ * print drupal_get_form('user_register');

The latter can be copy-pasted and it will work.

The following information (first chunk) sounds wrong:

             ... without any user input
+ * using by populating ...

hook_forms() still feels a little weird:

+    $forms[$type .'_node_form']['callback'] = 'node_form';

Why the ['callback'] part? Why not $forms[$type .'_node_form'] = 'node_form';? Personally, I wouldn't mind to remove the _forms hook. We could define functions like:

 function mynode_node_form(...) {
  return node_form(...);
}

However, that probably complicates matters with the CCK in mind. Probably a good enough reason to keep it around. However, to improve readability and discoverability of the code, I'd advise against using it (and if possible, would like to see that functionality go).

In the user.module's case it looks a bit weird:

+function user_forms() {
+  $forms['user_admin_access_add_form']['callback'] = 'user_admin_access_form';+   
+  $forms['user_admin_access_edit_form']['callback'] = 'user_admin_access_form';
+  $forms['user_admin_new_role']['callback'] = 'user_admin_role';
+  return $forms;
+}

Can't user_admin_access_add_form and user_admin_access_edit_form by merged? Why do we need two separate form IDs? Do the forms act differently based on the form_id? That's probably good information to add somewhere as it motivates the need for mapping multiple form_ids onto the same callback. The documentation mentions that it is possible, but doesn't really explain why you'd want to do that, or what good use cases are.

Also note that there is a stray '+' in the first line.

Good to see many optional $callback arguments being removed. There are still some of them around, and the are a little hard to grok:

+  return call_user_func_array(isset($callback) ? $callback : $form_id, $args);

Is that a left-over or is it still needed, actually?

Why the ['callback'] part? Why not $forms[$type .'_node_form'] = 'node_form';? Personally, I wouldn't mind to remove the _forms hook. We could define functions like:

Dries, it might make more sense to name that property 'form_builder' or something along those lines. The reason it's part of a nested structure is because it's also possible to define additional callback parameters for the form_builder function using hook_forms.

mymodule_forms() {
  global $user;

  $forms['my_current_user_form'] = array(
    'builder' => 'mymodule_form_builder',
    'arguments' => array($user->uid),
  );
  $forms['my_specified_user_form'] = array(
    'builder' => 'mymodule_form_builder',
    'arguments' => array(arg(1)),
  );
  return $forms;
}

In the above examples, calling drupal_get_form('my_current_user_form') builds a form based on the currently logged in user account, while drupal_get_form('my_specified_user_form') builds one based on the uid in www.example.com/user/uid. It's a very simple demonstration. It also means that we can begin defining optional meta-data for forms (things like access restrictions on a form, or workflow information) in the hook_forms() array. Right now it is only used for the arguments and the function name, but it provides a very important point of future API expandability. And, as noted, it's not something that people HAVE to implement right now; it's only needed by the very complex cases, and developers can move to it as they need the more complex handling that it enables.

I agree this should be documented better. I'm scratching my head, though, to figure out where is the best place for it -- hooks have no clear 'home', unless I'm misunderstanding things.

er, I botched up the names of the keys in that sample array. You get the picture ;)

added a bit more documentation per dries' comments. i omitted his first suggestion to: print drupal_get_form() since i think printing this by itself is extremely rare.

i tested lots of forms and all worked just fine.

After much discussion with chx, and a bit of tinkering, I took a stab at eliminating the trickery in menu.inc.

The results are exactly the same -- no form processing code has been changed -- but now, pages that are nothing but a form use drupal_get_form as their menu callback and the $form_id as their menu callback argument. menu.inc is now untouched by the patch.

There are a couple of typos in the documentation fixed, and the use of drupal_get_form for the callbacks allows us to put the call to drupal_render_form() back in drupal_get_form() where it belongs. (Processing and rendering are really separate steps). I've spent the last couple of hours testing each form with every module enabled, and I'm very confident that nothing has broken since moshe's tested version of the patch.

If the trickery in menu.inc, and the resulting obscurity in various modules' hook_menu functions, was a barrier to the patch's acceptance, consider those barriers removed. :)

Also, let it be known that I now owe chx one pony.

chx: sorry to be a pain.

Tested the last patch and it seems to work.

Did several benchmarks with a ?q=node/x page featuring a search box in the header and a comment form below the last comment. With this patch, the page is 4% slower.

I don't think the 4% performance drop is a show stopper, but maybe something is out of whack, or maybe we missed an optimization opportunity. (Is it due to the extra _forms() hook that is being called? When a page has two forms, do we duplicate any efforts?)

I think this is ready to be committed, but I'd appreciate a quick look with regard to the performance issue.

Quick question, Dries: how are you doing these benchmarks? I'd like to try to duplicate them so unecessary optimization paths aren't chased while ignoring real issues...

Using ab2 -n 20 -c 1000 with page caching disabled on a dedicated benchmark machine. I repeat the benchmarks 3 times, and I compute the average number of requests per second.

I used a page with 3 forms:
- The search form (header)
- The login form (sidebar)
- The 'add comment' form (body)

ab2 -n 20 -c 1000 doesn't sound correct

If I want to estimate the influence the impact of a patch on the generation time for a single page I always run with -c1. We don't want to stress the test the server after all. I ususally use higher n values, though.

I meant -n 1000 -c 20.

Cool, that's how I do it too, albeit with killes' modification (only one concurrent request), and the lack of a dedicated machine. :P I think that latter fact is one of the reasons I'm unable to benchmark too fine-grain of changes.

I think that was meant to be review...

With the last patch, there is still a 4% slowdown.

got commited by dries: http://drupal.org/cvs?commit=38635

Docs posted to http://drupal.org/node/64279