FormAPI 3: Ready to rock

subscribing!! don't know when I'll have a chance to test, but I'm on it. Awesome work you guys!

Here's the new form loading workflow.

Here's what happens inside drupal_process_form().

Wow, great write-up. :)

1. The word 'package' is somewhat peculiar -- wouldn't 'context' or 'state' to be a better word for this?

2. Can you write some upgrade instructions? We need these anyway for use in the handbook, and it would help me understand some of the proposed changes.

3. How does this help client-side and server-side validation in sync with AHAH and AJAX-built forms? (IMO, in the future this will be done by XForms. We need to make sure that the form API maps properly onto XForms.)

4. "#validate (with two wildly different use cases) is now #validate and #element_validate" -- care to explain this a bit more? Maybe with an example? When you say #element_validate, do you mean a form element definition rather than a form element value? The word 'element' is particularly vague and I'd prefer to use something more self-explanatory (widget?) if we can't think of something. (The code comment for both are identical and do not properly explain the difference.)

5. Are there places in core where we can take advantage of the new /package/ stuff? The patch doesn't try to take advantage of it, but it would be nice if it would. Helps us validate its usefulness.

6. Why is the caching important? Retrieving the cache from the database might actually be slower (in certain scenarios) than rebuilding the form.

7. Are we sure we can use the $user->uid to compute the cache id? Won't anonymous users accidentically see another anonymous user's private data? Wouldn't it be better to use sessions for this? It seems less prone to errors when another module overwrite the global $user object -- yes, we've seen this happen several times. The entire uid handling (i.e. uid_old) is a bit weird, and poorly documented in the code.

8. Good stuff! :)

subscribing!! any examples please - looks very promising

subscribe

subscribing

1. The word 'package' is somewhat peculiar -- wouldn't 'context' or 'state' to be a better word for this?

Sure. :-) chx and I both went around on this for a while trying to figure out the best word for it and decided that we'd just do the patch and let other people decide. ;-) $form_state might make sense. Functions would receive $form, $form_values, and $form_state.

2. Can you write some upgrade instructions? We need these anyway for use in the handbook, and it would help me understand some of the proposed changes.

Absolutely. I'm writing those up this morning. The nice thing is that while form.inc has changed, and the syntax for form related functions has changed slightly, the changes are also simple enough that a regex can make them. ~60K of the 78K patch -- all the non-form.inc stuff -- is actually just the work of the regex changing function arguments and changing return statements to use the $package variable.

3. How does this help client-side and server-side validation in sync with AHAH and AJAX-built forms? (IMO, in the future this will be done by XForms. We need to make sure that the form API maps properly onto XForms.)

An excellent question! We've been talking with Tao Starbow, author of the AHA Forms module, about the problems that he's faced working with FormAPI currently. Right now, AHAH/AJAX code works by altering a form and setting it to #multistep, sending a message to the server every time it adds or removes a form element on the client side, rebuilding and rendering THE ENTIRE form, extracting just the element it needs, retrieving that HTML, inserting that rendered piece of the form back into the client side version of the form, AND then saving descriptive information about the changes that were made in either the session object or a private cache table. That last step is necessary, because once the form is submitted, the AHAH/AJAX module has to intercept the form building call and alter in those fields that were added AGAIN, so the form can validate properly. Over the course of your average AJAX/AHAH form handling process, this can easily lead to dozens of full form lifecycles just to add a few new fields. Are you shuddering in horror? I know I am. ;-) The only other real solution is to turn on the horrible #DANGEROUS_SKIP_CHECK property and bypass almost all validation handling. And that, as we know, is not ideal at all. ;-)

How does caching the form change this? A client-side form has the #build_id of the form -- the unique identifier that is used to cache that particular instance of that particular form. It can send back a message to the server that says, "please add this element to the cached copy of the form, because I just added it on the client side." When the form is submitted, that cached copy is retrieved -- and lo, things validate properly. Writing the secure version of that 'remote form_altering' patch is one of the steps that follows this initial patch, but our extensive discussions with Tao Starbow and a number of other AHAH/AJAX developers have made it clear that if we don't do this in FAPI, they are going to have to do it by hand themselves anyways, and it will have to bypass FAPI security. Standardizing it and centralizing it lets us leverage the system in other ways, like traditional multisteps.

4. "#validate (with two wildly different use cases) is now #validate and #element_validate" -- care to explain this a bit more? Maybe with an example? When you say #element_validate, do you mean a form element definition rather than a form element value? The word 'element' is particularly vague and I'd prefer to use something more self-explanatory (widget?) if we can't think of something. (The code comment for both are identical and do not properly explain the difference.)

In the current formAPI, top-level #validate functions placed at the top level of the form receive $form_values, as one would expect. #validate functions placed below there, on specific sub-elements, DO NOT receive $form_values, but a full copy of the $form. The reason? form elements (like password_confirm) need to be able to check their own fields, but if they're deeply nested in a form structure, they may not even appear in a consistent location because of how #tree operates. One solution would be to make every #validate function receive both $form_values and $form, and leave it up to the developer to figure out when #tree has messed up the values collection for them. Instead, we felt it would be better to make the separation in use cases explicit, reserving #validate for the entire form, and #element_validate (with its special needs) for individual form elements. That also makes #validate consistent with #submit, which can also only be set at the top level of the form.

Explaining that #validate and #submit can be set for an entire form in 'normal' form-building situations, and that programmers implementing custom form elements can specify element-specific validation code with #element_validate, is easier IMO than trying to explain why a validate function receives totally different arguments depending on where it's placed in the form.

5. Are there places in core where we can take advantage of the new /package/ stuff? The patch doesn't try to take advantage of it, but it would be nice if it would. Helps us validate its usefulness.

Absolutely, without question. The first and most obvious places are existing #multistep forms: poll.module and system.module's system settings form. Those are actually the two 'to-dos' for this patch, mentioned in the original post but probably easy to miss considering how spammy it was. ;-)

The second is any place that we do a preview of the form before persisting. Node previewing in particular is currently hellish. If we simply built up a node object in the $package['storage'] bin, all the submission handlers would have a chance to 'do their thing' and it could be displayed cleanly at the beginning of the form building process. Right now, we have two baffling form flag: #pre_render and #after_build -- that were added JUST to make 'sticking a preview of the node at the beginning of the form' possible. Rework node_previews to use the new system, and those two form properties (and the need to document them) vanishes. Mmmmmmm.

chx and I have also explored the possibility of adding standard $package['errors'] and $package['messages'] bins to it that could be used by validate and submit handlers instead of form_set_error() and drupal_set_message(). In a normal form submission process, we would handle displaying those messages after processing is completed. when called via drupal_execute() however, they would just be passed back as the results of the drupal_execute() call. Right now, anyone using drupal_execute has to parse $_SESSION['messages'] and loop through form_get_errors() to see what happened during the form's processing life cycle.

There are others -- anywhere we're currently jacking around with the session in strange ways (like storing the node filter settings on admin/content/node) can easily be done with this system instead. The huge gains come for complex contrib modules like views, voting_actions, webform, and others. They have forms that essentially build up a complex object (like a view), adding data to it each trip through a multistep form until the 'submit' button is finally pressed and the complex object is persisted. Currently those modules have to jam things into hidden fields, or use their own caching system, to achieve this behavior. Now, they can simply build up the object in $package['storage'] and FormAPI handles the lifecycle issues for them.

We don't face that in core very often because we rarely try to build things that complex. ;-)

6. Why is the caching important? Retrieving the cache from the database might actually be slower (in certain scenarios) than rebuilding the form.

Most of this was covered above in the AJAX/AHAH question, but it's also worth noting that we're doing this to some extent already whenever a multistep form is generated. Now, though, we're using the session rather than a cache_table, which GREATLY limits our ability to clean up old and crusty data. the cache system already has great support for cache lifetimes, etc. this lets us leverage that to avoid long-term bloat.

Also, it's important to note forms are not always cached . They are only cached if 1) they have #cache => TRUE set, as is necessary for an AJAX/AHAH scenerio, or 2) they start looping, as in a traditional multistep scenerio like poll.module. We can tell a form is beginning to loop back on itself when its submit handler populates $package['storage'] -- if it doesn't get populated, we never bother with the caching system and forms execute as they do now. That actually makes it more efficient than the current #multistep system, which starts blasting data into the session as soon as a #multistep=>TRUE form is viewed, even if it's only opened and viewed and never submitted.

The speed difference between a single DB call and a full form-building (and form-altering) lifecycle is arguable, but the improvement to the workflow in #multistep scenerios is immense -- people will no longer have to carefully debug two build-cycles in the same function in each page-view, with slightly different parameters, one building a snapshot of 'the old form', the other building the new version of it. The old copy, for validation, is pulled from the cache and all is good.

7. Are we sure we can use the $user->uid to compute the cache id? Won't anonymous users accidentically see another anonymous user's private data? Wouldn't it be better to use sessions for this? It seems less prone to errors when another module overwrite the global $user object -- yes, we've seen this happen several times. The entire uid handling (i.e. uid_old) is a bit weird, and poorly documented in the code.

Yes. It's important to note that the uid is prepended to the build_id rather than used to calculate it. For anonymous users, the build_id will still be unique. Chx might want to explain the rationale behind prepending this information -- he explained some potential security implications, and he is most likely the best person to detail them. ;-) If the security issues would be better solved in a different way, though, we can remove the uid from that key generation, and the caching/retrieval code would work just fine.

Also, a slightly nicer version of the form-loading flowchart.

subscribe.

Re-rolled to keep up with HEAD, and way more commenting in form.inc.

Also, this version removes the unecessary use of $uid when creating the cache keys for forms AND renames $package to $form_state, which is what we're using it for. Also dropped the unecessary vestigial session_clearing function that was needed in Drupal-5.

Note that there's a database update involved: the addition of the cache_form table.

Here's the non-scripted portion of the patch -- it should be easier to skim.

And here's the PHP script that updates all the modules.

Previous patch had a bug wherein some forms would improperly redirect in the middle of multi-step operations. Fixed. And now, even more comments! Woo.

...And the stripped version of the patch, for use with fapi3.php.

Dependency checking on admin/build/modules is working again!

I looked at the code and it looks good. Good code comments too.

Feel free to mark RTBC after a good night of testing. ;-) I've only done very basic testing.

Dries -- Right now, everything BUT poll.module is working nicely. If we're willing to accept that the 'next step' is a second patch that focuses on reworking the node submit form for cleaner multi-step scenerios and cleaner handling of the 'preview' case, including poll.module's special requirements, I would say that this patch is ready to rock.

I do lean towards that solution, since poll.module always turns into its own special adventure, and the stuff with node.module that ties into that is a second-phase project that builds on the work in this patch.

Words cannot even begin to describe how much I want to rewrite poll.module from scratch. ;-)

Heh.

But... but... poll module is our regression test! ;)

J/K. I'd support this. I mean, menu module is/has been broken for 2+ months in HEAD. I think if it opens the door for these other improvements to simplfy the FAPI, that's worth it.

So... Karoly and I are going to be tackling that 'phase 2' portion, with the rework of how 'preview' mode works and how poll.module does its magic. In the meantime, this patch just needs some more smoke testing by other folks in the community.

Calling all testers! Calling all testers! Click on every form you can think of! ;-)

if you are cleaning up node submission, consider taking the code from Clean up node submit hooks which happens before nodeapi(insert/update). some good cleanup happenned there, but got mixed up with some bad company - namely deprecating most nodeapi hooks.

or just focus on node preview and poll

Moshe, that's certainly on the table. I think the biggest candidate for cleanup is nodeapi op 'submit', whose ONLY purpose is to turn $form_values data into the format expected by nodeapi op 'insert' and 'update'. Just migrating that 'submit' op into the formapi submit handlers would make quite a difference in code flow, I think.

After some additional testing I ran into some issues -- namely, flurries of notices when previewing or deleting nodes. It may be simple issues but I'm concerned that we may have missed some edge cases in the node submit forms, and we need to do a bit more investigating.

Further updates as events warrant.

Extra bonus: last night chx discovered that the only reason poll.module EVER worked in version 5.0 was because of a bug in FAPI 2. ;-) We were accidentally passing the full, unfiltered $_POST to form constructors rather than $form_values, which was a minor but definite security weakness. (not a hole, but certainly a weakness).

When that hole is plugged, poll.module breaks completely. Truly, poll.module is FAPI's regression test.

subscribe

Several notices were being triggered on the node preview and delete forms due to some vars not being set -- that's been fixed. The big remaining issue now is the fact that the installer no longer works. ;-)

There's a simple function_exists() call in form.inc now, to avoid checking for cached copies of the form when cache.inc hasn't even been included yet, but there is other weirdness still present in install.php that hasn't yet been sorted out. For the time being this patch should only be tested on *already installed* copies of Drupal HEAD.

The latest round of progress.

This patch adds the final step in getting the node preview stuff working again: button-specific callback functions. In other words, you can assign a different set of #submit and #validate handlers to your 'preview' button, and it can execute completely different code, without having to build goofy if $form_values['op'] == 'foo' switch logic all through your validate and submit code.

Along the way we broke up the monstrous 200+ line form_builder function into more bite-sized pieces.

And here's the patch itself!

Also worth noting: the new restructuring of form_builder() completely isolates the internet explorer-specific hacks we have to do to properly recognize submission in certain scenarios. In addition, it eliminates the need for several globals and paves the way (brace yourselves) for the removal of the $form_values global entirely. The end result? form.inc no longer uses secret sneaky globals to communicate between its various steps. Instead, it uses the $form_state array as a single, consistent one-stop shop. We still pass the $form_values array in to validate and submit handlers, but internally we use $form_state['values'] when building them, $form_state['submitted'] to indicate that the form should be processed, $form_state['storage'] to hold persistent storage between requests, and so on.

Results of testing:
* During install, I get: Fatal error: Call to undefined function cache_get() in /Applications/MAMP/htdocs/head/includes/form.inc on line 56 after entering my database credentials.
* Update path looks like it works!
* Login block is broken... if I leave either field blan, I get "Username/Password is required." However, if I enter anything in the boxes, whether valid login credentials or not, the page just refreshes and blanks my values in the textfields.
* When attempting to reset password, I get the following notices. I don't get these when the patch is reverse-applied:

    * notice: Undefined index: account in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1193.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1197.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1274.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1274.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1274.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1197.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1197.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1200.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1203.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1203.

* user/1/edit gives the following notices that don't appear when I reverse-apply the patch:

    * notice: Uninitialized string offset: 0 in /Applications/MAMP/htdocs/head/includes/form.inc on line 1201.
    * notice: Uninitialized string offset: 0 in /Applications/MAMP/htdocs/head/includes/form.inc on line 1206.

* jQuery teaser splitter is broken. The value in the teaser area gets removed, replaced with only the value in the body area.
* Preview is also broken everywhere; same behaviour as the login block -- just redirects back to page with no visible changes.
* When attaching a file with upload module, I get the following notices:

notice: Undefined index:  file_previews in /Applications/MAMP/htdocs/head/modules/upload/upload.module on line 329.
warning: Missing argument 3 for form_builder(), called in /Applications/MAMP/htdocs/head/modules/upload/upload.module on line 911 and defined in /Applications/MAMP/htdocs/head/includes/form.inc on line 703.
notice: Undefined index:  submitted in /Applications/MAMP/htdocs/head/includes/form.inc on line 883.

(there's also one about locale.module, and some notices on viewing a node with an attached file, but that happens even without this patch.)

Sorry, that's all I have time for atm.

* During install, I get: Fatal error: Call to undefined function cache_get() in /Applications/MAMP/htdocs/head/includes/form.inc on line 56 after entering my database credentials.

Yes, the installer page is currently in the process of being fixed. ;-)

* Login block is broken... if I leave either field blan, I get "Username/Password is required." However, if I enter anything in the boxes, whether valid login credentials or not, the page just refreshes and blanks my values in the textfields.
* When attempting to reset password, I get the following notices. I don't get these when the patch is reverse-applied:

Silly error -- I'm about to reroll the patch to fix the typo that broke that. Thanks :)

* jQuery teaser splitter is broken. The value in the teaser area gets removed, replaced with only the value in the body area.
* Preview is also broken everywhere; same behaviour as the login block -- just redirects back to page with no visible changes.
* When attaching a file with upload module, I get the following notices:

These three are all connected to the last change in how node-preview is handled. The next set of changes will fix preview-related stuff.

Thanks!

It sounds like a lot of broken stuff, but all boil down to 1) installer-related stuff or 2) node preview related stuff, the two portions that are still remaining. Further updates as events warrant... :)

--Jeff

Oops, sorry. I had misread #34, and thought this patch had fixed previews. :)

No worries.

Right now, node previews work by using a non-functional button (one that validates but doesn't submit) to trigger a reload of the form. Then, the raw $_POST values are inspected and the t('Preview') string is checked. If it's there, we trigger a bunch of custom goofy FormAPI callbacks like #after_build that were added JUST to make previewing possible.

With this new system, we'll be able to simply have a separate 'preview' button with its own set of callbacks. It can use the $form_state collection to build up the 'preview' version of the node, then set the $form_state['rebuild'] = TRUE flag to indicate that the form should be rebuilt from scratch using the newly updated 'preview node.'

It's a subtle difference, but it means fewer strange bits buried around the system in globals and special callback functions like #after_build, and more behavior centralized around the $form_state[] collection. The nice thing is that you'll always be able to take a look at THAT variable when you're processing, or debugging, to look at what the current workflow is up to...

Here's a copy of the patch with just the login/account/validation issue fixed. Previews and Installation are still messed up, but doing a clean HEAD install, followed by th patch, followed by update.php, will work.

Further posts later today as the preview issue is hammered out, followed by install.php. (The reason it's being saved for last is simple: it currently short-circuits quite a bit of FormAPI in order to work before things are bootstrapped fully. We want to make sure the other issues are ironed out, and FAPI is clean, before trying to rework install.php's tricks...

Subscribing.

Some more stuff to add to webchick's results:

* admin/content/node -> Content filtering is broken :

    * notice: Undefined index: op in D:\var\htdocs\drupal_head\modules\node\node.module on line 1514.

* admin/content/node -> "Update options" is broken :

    * notice: Undefined index: #submit in D:\var\htdocs\drupal_head\includes\form.inc on line 773.
    * notice: Undefined index: #validate in D:\var\htdocs\drupal_head\includes\form.inc on line 774.

* admin/user/roles -> "add role" and "edit role" don't work :

    * notice: Undefined index: op in D:\var\htdocs\drupal_head\modules\user\user.module on line 2105.
    * notice: Undefined index: op in D:\var\htdocs\drupal_head\modules\user\user.module on line 2109.
    * notice: Undefined index: op in D:\var\htdocs\drupal_head\modules\user\user.module on line 2117.

* admin/user/user -> User filtering is broken :

    * notice: Undefined index: op in D:\var\htdocs\drupal_head\modules\user\user.module on line 2797. 

Thanks,
Riccardo

Ricardo, are you by any chance using Internet Explorer? Two of those appear to be related to custom IE button-click handling code.

Jeff, I'm using FF 2.0.0.3

This version of the patch fixes a problem that kept button ops from firing, threw a couple of the notices, and... blocked all validation. And all logins. ;-) The errors found by riccardo are now fixed.

chx and I are about to dive into fixing node previews, then install.php. further updates as events warrant.

Huzzah!

Node previews are working (and are now implemented in a much simpler and more flexible fashion). Install.php is working again thanks to chx's craftiness. And poll.module is working again as well.

And did I mention? form.inc no longer uses globals. For anything.

There is some ugliness still going on in upload.module: although the first file uploads properly, subsequent ones overwrite it. I'm tempted to say that we should treat it as a second-phase cleanup patch, since it's just a matter of digging in and rewriting some of the module to avoid the old-style ugliness, but I'll defer to others on that one. The patch is 140K now, and I'm hoping that we can get it in and clean up some of the details. ;-)

I'll be going back through and updating the comments for all of form.inc, now that some of the workflow details have been pinned down. I've also got a handbook file written up on the 'standard' elements of $form_state that control form workflow and rendering.

So, another version of the patch with updated comments is coming, but it's definitely ready for review.

One other bit of known broken-ness: user admin operations and node admin operations (the filter forms, etc). Those are both scheduled for cleanup: they are PRIME candidates for the new form building techniques because of the way they loop around and do goofy things. But they're also known to be wonky right now.

Poll.module is our regression test, and it works. ;-)

cool, i've read the whole thread and this sounds fantastic. i'm too tired for a thorough review/test now, but i might get a prize for the first person beyond chx and eaton to port anything to FAPI3 ;) ... my latest patch for dblog filtering in HEAD:

http://drupal.org/files/issues/dblog-filtering-multiselect_fapi3.patch.txt (comment #54).

i'm happy to report the porting task was easy and everything made sense (even without real upgrade docs) and took all of about 10 minutes (including patching my HEAD test site and running update.php, all of which worked without a hitch), so that's a clear sign this patch is getting very close to RTBC. porting something more insane like project*, dba.module, etc, would be the real test, but that'll have to wait for another time. ;)

once again, chx and eaton thoroughly rock. ;) thanks, y'all!

Here's the slightly updated version, with tweaked code comments and a handful of fixes. (drupal_execute wouldn't have properly handled a custom $form_state being passed in, etc.)

In addition, there are some extra bonuses. When submitting nodes, users, taxonomy terms, and taxonomy vocabularies programmatically via drupal_execute, the $form_state will contain actual useful information after processing is finished.

$form_state['nid'], $form_state['tid'], $form_state['vid'] will all contain the respective ids of submitted records, and when the user_register form is submitted via drupal_execute(), $form_state['user'] will be populated with the resulting user account -- including its password, uid, and so on. This means that pro grammatically setting up a new site with dummy accounts and dummy content gets a bit easier: you can actually make note of the generated IDs without ugly string parsing and so on.

There is one funky addition: a small implementation of cache.inc called 'cache-install.inc'. It's used to stub out cache functions without triggering any database hits during initial site boostrapping. It ensures that functions that call cache_get() won't accidentally blow up in flames. It's documented and commented in the code, but I figured it would be better to note it explicitly rather than raise eyebrows. ;-)

There are upload.module problems, as noted in earlier posts, and the node administration and user administration screens need to be reworked. Those cases, though, are all areas in need of refactoring for other reasons. If there are no objections, I'd love to see those three issues treated as follow-up patches... This is a monster of a refactoring, and while there's not a lot of code that needs to change for modules that use FormAPI in normal ways, the sooner it's "in the wild," the better....

Here's what I can find at the moment:

* user/1/edit:

    * notice: Uninitialized string offset: 0 in /Applications/MAMP/htdocs/head/includes/form.inc on line 1157.
    * notice: Uninitialized string offset: 0 in /Applications/MAMP/htdocs/head/includes/form.inc on line 1162.

* admin/build/modules/uninstall is broken; just redirects back to the form.
* jQuery teaser splitter is still broken. Retains only one of the values.
* Saving the form at admin/build/locale/language/edit/en yields a white screen of death.

The cache-install.inc is a bit weird. I'd rather see that fixed differently. Maybe put a function_exist() in it somewhere? I'll have to give it some thought.

Webchick, thanks! I'm getting those fixed today.

Dries, chx and I went round and round about that very thing. I'd like to see it fixed a different way too, but I couldn't think of anything cleaner. If we put function_exists() all throughout form.inc before checking cache it seems a bit ugly. On the flip side, if we define some sort of stubbed out versions of the functions in install.php iself, it'll fail once we try to bootstrap the *actual* full cache system. This lets us take advantage of the swappable caching mechanism to put in the stub until the DB is ready.

If you can think of better solution (or are willing to wrap the form.inc cache references in function_exists even though the installer is the only place where that would occur), I'm OK with that.

Fixed the jQuery teaser splitter, and the notices when editing any form with a password_confirm element. Locale and uninstall fixes coming later today hopefully.

Sadly, it appears that we can't completely get rid of the old #after_build flag. Some things, like the jQuery teaser splitter, really do need that explicit step to bridge the gap between jQuery managed elements and composite input elements. So, #after_build remains in place.

you rock! this is really great work!

While reading through the whole issue, some suggestions come up in my head like putting the form_values in the $form_state. reading further you had already implemented that - amazing!
this will make implementing multistep and ahah forms real fun!

I'd also really love to see $form_state['errors'] and $form_state['messages'] - so that $form_state consistently holds the whole state of the form. However that could be tackled after this is in..

I'm going testing now..

The batch patch has landed - I'll try to update the FAPI3 patch accordingly, unfortunately I'll be away from home (but connected) for the next few days...

subscribing

subscribing

Attached patch is a first pass at updating the FAPI3 patch for latest HEAD and the new batch API. Not completely there yet, but I'll be AFK for a few days, so I post what I have so far, in case you want to follow-up.

Patch is rerolled against latest HEAD, I tried to update the new forms code that went in locale.inc a few days ago. You should probably double check I did not miss anything or left out existing bits from the previous patch...

Batches integrate in the form processing workflow in two places :
- form_execute_handlers($type = 'submit') : store submit callbacks for execution at the right time during batch processing
- drupal_process_form(), after form_execute_handlers : redirect to the batch processing page.

Things seem to work rather well, a few points should be noted (I left some comments as 'TODO's in the code at the relevant places) :
- my implementation might be, er, naive. Please check I'm not doing anything stupid :-)
- batches are not multistep friendly yet. I still did not completely wrapped my head around how multistep work, and how we could 'redirect to step 2' after the batch for step 1 has finished.
- the amount of data stored in the batch is sub-optimal (one copy of the entire $form per submit callback), and batches take longer to process than with current HEAD. Can obvioulsy be made better, still trying to figure out the best way.

A few remarks / question regarding the patch itself :
- it seems validate / submit callbacks are allowed to alter the $form itself : is that intended ?
- drupal_execute trigger the following warnings :
* notice: Undefined index: submitted in includes\form.inc on line 850.
* notice: Undefined index: form_build_id in includes\form.inc on line 272.
* notice: Undefined index: form_build_id in includes\form.inc on line 273.
- Shouldn't the {cache_form}update be in update.php instead of system.update ?
- the patch removes these lines in node_submit :

-  // Do node-type-specific validation checks.
-  node_invoke($node, 'submit');
-  node_invoke_nodeapi($node, 'submit');

Is that correct ? (don't bother to explain if it is, I just found that surprising...)

My little batch_test.module, updated for FAPI3-style forms.
Provides tests for batches in 'simple' (single submit callback), 'complex' (multiple submit callbacks), multistep and programmatic forms.

Additional remark :
the following code in _form_builder_handle_input_element looks surprising

if (isset($form['#validate'])) {
  $form_state['validate_handlers'] = $form['#submit'];
}
if (isset($form['#submit'])) {
  $form_state['submit_handlers'] = $form['#submit'];
}

Is that a copy-paste error ?

That absolutely IS a copy-paste error, yched. THANKS! Your update is definitely a big chunk of work, I'll take a close look over it tomorrow and try to see if I can wrap my head around batching. One of the important things to remember is that as long as any given step receives the proper bundle of information in the $form_state array, it cna process properly. That means that if the 'intermediate' batch processing pages hijack things in some way, then hand off the form state in its "proper" form to the next step, things should work fine...

Actually I just managed to hack multistep / batches together.
Not very clean code yet, needs some polishing and code commenting, but definitely works.
I thought I'd post this before taking my train :-)

This version fixes the problems with upload.module, and the problems with locale editing. The big remaining issue is the uninstall screen, still have to hit that.

There are several notices during preview/saving of attached files, but those appear to be due to deep-seated E_ALL issues in file.inc, and I don't want to creep the scope of this patch any more than it *already* is. ;-)

Testing greatly appreciated, everyone!

Some feedback:
* Running install.php yields a white screen of death because the new file includes/cache-install.inc is no longer created by the patch.
I manually created cache-install.inc by extracting it from patch #66 and have been able to perform a clean install.

* When trying to enable "normal caching" I got the following messages:

    * notice: Undefined variable: user in D:\var\htdocs\drupal_head\includes\form.inc on line 290.
    * notice: Trying to get property of non-object in D:\var\htdocs\drupal_head\includes\form.inc on line 290.

* The copy-paste error reported by yched on #64 is still present.

Subscribing, just read the entire thread. This is very exciting :)

RiccardoR,

Thanks for those catches. I'd rolled the patch with the wrong option and it didn't include the additional file. Here's a new version, with the added file, the #submit typo, and the notice messages that you spotted all fixed.

jQuery teaser splitter working great now!

admin/content/forum, add container (or forum):

notice: Undefined variable: form_id in /Applications/MAMP/htdocs/head/modules/forum/forum.module on line 572.

Doesn't appear when patch is reverted.

user/password, enter username and submit 'reset password' form:

    * warning: Missing argument 3 for form_set_value(), called in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1183 and defined in /Applications/MAMP/htdocs/head/includes/form.inc on line 932.
    * notice: Undefined index: account in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1193.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1197.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1274.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1274.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1274.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1197.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1197.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1200.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1203.
    * notice: Trying to get property of non-object in /Applications/MAMP/htdocs/head/modules/user/user.module on line 1203.

I imagine the first one is the cause of the latter ones.

Other than the module uninstall (which you already mentioned), I'm having to try real hard to come up with issues. ;) yay!

Reset password form and other misc. notices solved, and the module uninstall page is working.

There are still a number of 'TODO' style comments in the batch-processing code that I'm not sure I understand. I need to read over them more thoroughly to figure out how to best document some of what's going on there, but everything that *I* understand (ie, not batch) is working.

Setting this back to 'needs review' as all the major issues I was aware of have been corrected. Let's pound on it. :-)

Reset password form and other misc. notices solved, and the module uninstall page is working.

There are still a number of 'TODO' style comments in the batch-processing code that I'm not sure I understand. I need to read over them more thoroughly to figure out how to best document some of what's going on there, but everything that *I* understand (ie, not batch) is working.

Setting this back to 'needs review' as all the major issues I was aware of have been corrected. Let's pound on it. :-)

About the remaining (batch-related) TODOs - some of them are not related to the FAPI3 patch, and are more small batch bugs spamming your patch :-).

Taking them in the order of appearance in the latest patch (#73) :
- 1st one (_batch_process) is for me alone, and is not FAPI3-related.
- 3rd one (drupal_get_form) is actually not a TODO anymore. It's about batches with multistep forms, which now seem to work well together. I should probably polish the code and update the code comments, though.
- 4th one (drupal_process_form) can be removed - this _is_ the right place for batch redirection...
- 8th one (batch_process / $user->sid) is not FAPI3-related.

For the other ones, I'll try to grab you on IRC when I get back home on thursday.

Jeff, I am sorry to tell you that cache-install.inc has disappeared again.

Two small issues concerning taxonomy:

admin/content/taxonomy, edit an existing vocabulary :

    * notice: Undefined index: op in D:\var\htdocs\drupal_head\modules\taxonomy\taxonomy.module on line 1441.
    * notice: Undefined index: confirm in D:\var\htdocs\drupal_head\modules\taxonomy\taxonomy.module on line 1441.

(reason: _$POST[] is empty)

When trying to edit an existing term :

    * notice: Undefined index: op in D:\var\htdocs\drupal_head\modules\taxonomy\taxonomy.module on line 1451.
    * notice: Undefined index: confirm in D:\var\htdocs\drupal_head\modules\taxonomy\taxonomy.module on line 1451.
    * notice: Trying to get property of non-object in D:\var\htdocs\drupal_head\modules\taxonomy\taxonomy.module on line 425.
    * notice: Trying to get property of non-object in D:\var\htdocs\drupal_head\modules\taxonomy\taxonomy.module on line 443.
    * notice: Undefined variable: synonyms in D:\var\htdocs\drupal_head\modules\taxonomy\taxonomy.module on line 1038.
    * notice: Trying to get property of non-object in D:\var\htdocs\drupal_head\modules\taxonomy\taxonomy.module on line 459.

(initial reason: _$POST[] is empty)

Thanks.
This is almost ready to rock n' roll ;)

After taking a quick look at both of those sets of warnings, they don't seem to have anything to do with this patch in particular, just with some funky things Taxonomy module does in its own forms. Given the existing scope of the patch, I'm going to hold off on trying to fix those for now as they break nothing and are better handled in a dedicated 'fix taxonomy module's E_ALLs' or 'refactor taxonomy's forms' patch.

Right now, the big question for me is the stuff related to batch-processing. The TODO code scattered throughout the patch concerns me. Would it make sense to pull out the commented-out bits and the TODOs, and treat them as a separate batch-related patch entirely? yched, I'm not familiar enough with the state of batch stuff to know whether the code that's currently in this patch works, or whether it is 'half-updated'.

Also, thanks for noticing the missing cache file. That's rolled in again. ;-)

Eaton, it seems the patch does not apply cleanly on latest HEAD's (conflicts in instal.php, node.module).
Maybe it's just offsets, but I'd prefer not to mess anything.

Then I can reroll a 'cleaned' patch without the TODO comments (which I intend to tackle tomorrow).

Forgot to add : the FAPI3 related TODOs are mainly details / optimizations / code style.
The current code is largely functionnal wrt batches, so it's probably ok to be zen about the todos

As per yched's comments, I've rolled a fresh version against HEAD without the TODOs in form.inc. Those were mostly workflow/optimization concepts for batch processing, and can certainly be handled in a separate patch.

Let's test this baby!

When posting a comment to a page, I get this warning on the initial preview:

* warning: Missing argument 3 for drupal_validate_form() in .../includes/form.inc on line 407.
* Validation error, please try again. If this error persists, please contact the site administrator.

I then proceed to post the comment but it does not actually post.

subscribing

- Re-rolled against latest HEAD (including FAPI3 adaptation of the recent install.php update)
- batch integration is cleaner and better commented. The TODOs from one or two patches ago are now officially done :-)
- I did not try to fix the comment bugs reported in #80, though.

I wonder if we need to settle with the following layout:

 cache.dummy.inc
 cache.db.inc
 cache.memory.inc

I looked at the code once more and it looks nice and clean. From a code pov, this patch is RTBC.

However, from a stability pov, this patch might need a bit more testing. I leave that up to Eaton to decide. Of course we can fix up stupid mistakes after this patch landed.

Either way, I'd like to commit this patch ASAP.

However, from a stability pov, this patch might need a bit more testing. I leave that up to Eaton to decide. Of course we can fix up stupid mistakes after this patch landed.

I definitely agree. The problems we are finding are simple to fix (mostly missed forms that need some tweaks to their params), but there are a lot of forms in core. The biggest difficulty right now is that the patch, at ~170K, touches almost everything and currently breaks and requires a tweak and re-roll anytime anything hits core. :-)

However, from a stability pov, this patch might need a bit more testing. I leave that up to Eaton to decide. Of course we can fix up stupid mistakes after this patch landed.

I definitely agree. The problems we are finding are simple to fix (mostly missed forms that need some tweaks to their params), but there are a lot of forms in core. The biggest difficulty right now is that the patch, at ~170K, touches almost everything and currently breaks and requires a tweak and re-roll anytime anything hits core. :-)

Re-rolled against HEAD.

The installer is now broken (surprise, surprise... ;)). It won't progress any past the initial screen where it's prompting for db information.

Yay, the installer's working again!

Note to anyone testing: quite a bit seems messed up ATM, for example there are assorted errors and an access denied message at node/add/story, and content types don't list when visiting node/add. Those problems exist in clean HEAD at the moment, so they shouldn't be reported in this thread. ;-)

yched, just curious: why were the 200 lines or so of batch processing code put into form.inc..? isn't there a batch.inc for that purpose? I haven't been following that patch closely, so I'm not sure about the details.

I'm a little concerned about the complexity that it adds to drupal_get_form(); it seems that a huge amount of work goes into trying to streamline it a bit more for each version of Drupal, and it quickly gets overtaken by more special-casing logic. I need to go back and re-read the issue to get some insights into what it's doing.

For anyone testing this patch, the following command will "reverse" apply the changes it makes:

patch -p0 -R < patch.patch

(note the -R ... to re-apply again, just do the normal patch command)

So please only report bugs that are NOT found when the patch is reverse-applied.

It kind of seems to me like this batch stuff has repeatedly held back and inflated this critically important patch. There's no way we could strip that out for now and add it back in post-codefreeze is there?

For anyone testing this patch, the following command will "reverse" apply the changes it makes:

patch -p0 -R < patch.patch

(note the -R ... to re-apply again, just do the normal patch command)

So please only report bugs that are NOT found when the patch is reverse-applied.

It kind of seems to me like this batch stuff has repeatedly held back and inflated this critically important patch. There's no way we could strip that out for now and add it back in post-codefreeze is there?

@eaton (form.inc / batch.inc) :
batch.inc contains the batch processing engine, conditionally included only when actually processing a batch (this was per chx request).
The functions that live in form.inc are the outward-facing API, and thus need to be loaded for every page. I put them in form.inc because batches are highly FAPI-related, and Dries and Gabor seemed OK with that.
Moving them somewhere else is fine by me. For instance we could have batch_api.inc (always included) and batch_engine.inc (conditionally included). Whose decision is this ?

@webchick (neglect batches for now ?) :
Well, this would mean leaving a large part of the batch feature (temporarily ?) broken. Update.php and - i think - the new installer process should not be affected, though, so I guess this can be considered.
Not my call either, obviously :-).

A soon as the batch patch got committed, I did my best to minimize the delay on the FAPI3 work by providing the code to adapt to the new state of HEAD. Batches / forms work hand in hand quite well in the latest patches, and the impact code-wise is relatively clean and well delimited : a few lines in 3 functions (although I have to agree with eaton that drupal_get_form is not the neatest part - not sure what we can do about it, but I'm ready to work on this with him - IRC ?)

As eaton stated in #84, I think the main thing that's holding this patch back now is its mere size, and the fact that every new stuff that gets committed requires some work and a reroll...

The two last patches by webchick and eaton merge this FAPI3 patch with the 'multiple menu' patch in http://drupal.org/node/137767.

Attached is the same patch as #88 (installer fixed), without the menu patch...
Eaton, you _might_ want to check nothing's missing...

new patch fixes comment submission - comment preview still broken.

(damn, no .patch extension allowed on attachment ??)

(damn, no .patch extension allowed on attachment ??)

sorry, that was a temporary accident: http://drupal.org/node/143586

Comment previews and submission are working happily again.

It's also imperative that anyone testing pay attention to webchick's earlier posts: there are quite a few bizarre and somewhat subtle bugs floating around in core right now. Before reporting any issues with this patch be sure to reverse the patch and test a clean copy as well.

Er, sorry, that was a patch from the wrong directory -- the previous roll that included menu related stuff. Here's the correct one.

new patch fixes warnings on single comment deletion (and reworks the code to be more fapi3-style : no $_POST stuff)

I'm officially hitting the big green button.

I found a couple other minor things (notices in forum module, etc.) but nothing so critical that it's worth holding up this patch any longer. Let's get this sucker in!!

I've committed this patch. Not marking this 'fixed' until:

1. Documentation has been provided.
2. An update/mail is sent to development@drupal.org.
3. We probably want to add a CHANGELOG.txt entry.

Good work guys. Let's keep working on this in order to perfect it. :)

This patch re-introduces the pgsql bug fixed in http://drupal.org/node/139673.

The when schema patch gets applied (it is almost ready), this kind of problem should permanently vanish.

attached patch fixes the pgsql bug mentioned above.

Dries, it looks like you committed (parts of ?) the custom date formats patch along with this fix.

yched: yep, I already clarified that in the custom date format patch.

Yep, saw that :-)

Setting back to 'needs work' for the remaining doc tasks.

Some more bugs which may or may not be related:

Delete confirm forms broken
Search is broken: keyword is always "Array"

Attached is a fix for the search form and node delete confirmation. Thanks, webchick!

Works great. Thanks, Eaton! :D

Committed the 2 fixes. Setting back to 'code needs work'.

http://drupal.org/node/144132 contains my first stab at the upgrade guide. The main Module 5-6 upgrade page has also been updated with a link to it.

The FAPI3 patch removed in function node_submit() the lines calling hook_submit and hook_nodeapi('submit') - not sure why, my bet is it's a typo.
Unless I'm missing something, those hooks are not triggered anymore by anything in core right now.

Ok, if this is supposed to be in a new issue, sorry! After finishing my little patch that could persist submit values between submit callbacks, it got swooped by this beautiful patch just before I added an issue. What can I say, I'm not complaining! Wonderful work Eaton and chx.

The problem I attempted to solve was the fact that hook_form_alter is capable of adding new form elements to a form, and to retrieve them $form['#submit'] (which is an array) could be tacked with a new submit handler like so:

$form['#submit'][] = 'mymodule_orignial_module_submit';

One more problem remained, if the original hook_submit added a new row in a table, then the new id would most likely be essential in the subsequent submit handlers to create a foreign key relationship (exactly like nodeapi's which passes the node, with the valuable nid). Now I tested weather this was doable in this patched version and I couldn't find it. So I set on a new patch and wonderfully enough it was trivial. My new patch adds a key called $form_state['persist'] that will save whatever is returned from the submit function and passes it to the later submit handlers. I was hoping that good module developers would be instructed to return any important data in their submit functions so that subsequently called handlers would receive them.

If this functionality is already present in this patch, or if there's some other way to handle this scenario then please let me know. I've attached the patch file and changed status to "code needs review". Thanks

clarified some comments & added code to prevent this functionality from being triggered on validate.

That patch should definitely go in another issue, not in this one -- it's a new feature that deserves its own discussion, rather than being treated as a bug fix for this one.

That said, I'm not sure what the 'persistent' key would do that existing form_state keys don't do already. If one submit handler puts data into $form_state, all subsequent submit handlers can see that data as well; there's no need to add a special new set of keys...

Actually that's what I figured $form_state was for. But for some reason, any additional keys I add to $form_state simply vanish once my hook_submit call ends. I'm positive that there's nothing wrong with my PHP configuration files. Any hints?

Are you using $form_state, or &$form_state? the variable has to be taken by reference for your changes to be seen.

Also, there are two aspects of the issue: first, there is form_state data that is carried along *during a particular processing cycle* for the form. In other words, when you click submit, and the form is built and validated and submit handlers are called and then the form is optionally rendered etc, that's one cycle. The values you put in form_state during that cycle are not automatically still there when you submit a SECOND time.

There is, however, the $form_state['storage'] bin -- it gets persisted between page loads and is a 'safe' place to put data if you're doing multistep form processes.

Reading the documentation that is here http://drupal.org/node/144132 I find something that really needs to be fixed: the function prototypes have asymmetric argument ordering.

For example, for *_form_alter() we have $form_values, $form_id, $form_state, but for *_validate() we have $form_id, $form_values, $form_state. For *_submit() we have $form_id, $form_values, $form_state.

It would be best for everyone if we have a general consistent order for the arguments, for example, $form_id, $form_values followed by $form_state. This would make the learning curve less steep, as well as reduce confusion and errors.

If this needs a separate issue, let me know.

FAPI3 breaks language negociation : http://drupal.org/node/144634

I agree with kbahey: let's polish this some.

Yes, the $form_state worked to pass variables between handlers.

Eaton, in your documentation page, you have the variables to hook_submit & hook_navigate in the wrong order, what you have is:

($form, $form_values, $form_state)

What should be (according the the core call, not what I really think the order should be):

($form_values, $form, &$form_state)

I took the liberty of assuming you'd want only $form_state to be passed by reference. Although it would be helpful if you pointed out which ones should be passed by reference and which by value and why. Please disregard my patch as it's evidently not required with the introducing of $form_state.

Is this FormAPI3 patch the cause for Theme Settings cannot be saved http://drupal.org/node/144162 and why we cannot test the patch http://drupal.org/node/57676?

IMO the "Theme Settings cannot be saved" issue filed at http://drupal.org/node/144162 is actually a follow-on of FormAPI3.
I posted there a patch that seems to fix.
Of course a review from FAPI3 developer(s) would be greatly appreciated.

Thanks,
Riccardo

riccardoR, it looks like that issue related to FormAPI 3, but rather a change in HEAD from months ago (the elimination of #base). Good catch, though!

Let me preface this comment by saying that FAPI3 is very nice. You guys did a great job!

But, Eaton, I’m somewhat sure that FAPI3 is the cause of #144162. I've been actively trying to add custom settings to the D6 Theme Settings form since May 3 and the form didn’t break until FAPI3 landed (May 14).

FAPI3 modifies system_settings_form() to include these lines:

  $form['#submit'][] = 'system_settings_form_submit';
  $form['#validate'][] = 'system_settings_form_validate';

Which means system_settings_form_submit() is called instead of system_theme_settings_submit() and the Administration Theme and Performance forms show “The configuration options have been saved.” twice when submitted.

All of these issues are easily fixed. See the patch over at #144162.

I'm trying to keep Nodeorder in-step with HEAD. Here's confirmation of http://drupal.org/node/138706#comment-245319 -- when a custom #submit handler is called, the order of parameters isn't as advertised. Possibly line 614 of form.inc:

        $function($form_state['values'], $form, $form_state);

just needs to swap the first two parameters sent into $function.

Not even sure where to submit this, so I'm hoping this high traffic issue will suffice. There's a typo on the first line of:

function _form_builder_handle_input_element($form_id, &$form, &$form_state) {
  if (isset($form['#type']) && $form['#type'] == 'form' && !empty($form['#programed'])) {

#programed should be #programmed

this issue has become completely impossible to follow, due to the number of unrelated follow-up requests and bugs being reported here. so, i'm hereby closing this issue.

see http://drupal.org/node/146470 for the effort to unify the signatures for FAPI3 callbacks (suggested by kbahey, supported by dries, myself, and others). that was the only thing that arguably still belonged here, and why the status should have been "code needs work". the docs are done (and great), so there's no need to keep the status open for those, anymore.

@marcp: the right place to submit a bug report is in a new issue: http://drupal.org/node/add/project-issue/drupal -- "high traffic" issues become almost impossible to use if there are multiple threads of effort. submit a new core bug, component == "forms system", and please post a patch, not just a code snippet pointing out a typo. thanks.

@all: if anyone else finds a bug in FAPI3, please do not mention it here. Submit your own issue, so each problem can be investigated, verified, fixed, reviewed, and committed independently.

thanks again to chx, eaton, and all the people who contributed to reviewing and getting this into core. but, please, let's leave this issue to rest.

Subscribing...

Subscribing...