This patch is still in progress, but I've attached it here so we have a place to work and so people can analyze the path and consider any weaknesses.

What this patch does:

  1. It introduces a new function, drupal_build_form() which takes over the original drupal_get_form(). The key difference is that drupal_build_form() doesn't take the arguments to the form builder form the command line, but instead it takes a pre-populated $form_state array which contains the $args as $form_state['args'] and allows for a few other switches to control form behavior. The advantage here is that by sending the $form_state in as a reference, the $form_state still exists allowing the calling function to do things with the data the form manipulated.
  2. drupal_get_form() is then made a very small wrapper around drupal_build_form() because in a lot of cases, $form_state won't be necessary, so there's no reason to force people to build one. It retains the original syntax.
  3. Because args are now stored on the $form_state, drupal_retrieve_form() has been simplified.
  4. $form_state['method'] will allow fapi to sort of cleanly use $_GET as a form method.
  5. Allowances for a form to declare it really really really does not want to be cached.
  6. One of the things this will open up is the ability to finally fix node previews. Currently node previews try to generate a preview based upon form data that was simply turned into an object. This fails in a number of horrifying ways. What instead should happen is this: fapi renders the form; user clicks preview; form is submitted. $node object is constructed in node_form_submit (plus other submit functions). node_page_edit() gets this $node object off the $form_state and 1) if preview was clicked, renders it, 2) if submit was clicked saves it.

    The above will require a lot of tuning since it's a major change in the location of where node save happens; it requires node_save to do proper CRUD (save a $node object rather than a $form_state['values'] array which is different) and all of the fun that goes with that.

    Problems to solve:

    1. There are probably issues with form caching; many of the settings on $form_state will need to be cached with a form.
    2. $form['#post'] needs to be removed entirely. To do this, $form_state will need to be accessible in more places (particularly the theme layer and a few other places).
    3. A couple calls to drupal_retrieve_form() still need to be converted.
    4. $form['#parameters'] seems like it should be deprecated with this, need to make sure it fully is.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eaton’s picture

I'll be weighing in on this one with more detail later, but I think it's very important. with more AHAH and AJAX based sites springing up, we need these sorts of robust support functions in a bad way. The restructuring that Merlinofchaos proposes brings a lot of consistency to FAPI.

Pasqualle’s picture

subscribe

webchick’s picture

Subscribing.

This is enabler patch #0 for views in core. It'd be lovely to get folks with some FAPI chops on this.

sun’s picture

Marking for later review.

webchick’s picture

Issue tags: +Views in Core
timmillwood’s picture

Subscribing.

Very interested in helping with getting views in core.

drewish’s picture

subscribing

Frando’s picture

FileSize
13.86 KB

This is a great patch. Removing as much of $_POST and ['#post'] for $form_state['input'] sounds great.

Attached is a simple reroll, no other changes. Merging was quite easy thanks to git, I hope I didn't break it somewhere. But from just looking at the code, poll.module must be broken.

Frando’s picture

FileSize
14.88 KB

OK, so attached patch 1) changes all cases where $form_state['post'] was used to $form_state['input'] and removes $form_state['post'] completely, and 2) makes poll module work somehow. The choices are not added in the right order somehow, but at least it works without errors.

Earl: In your patch you removed $form['#parameters'] completely - was this on purpose? If it was on purpose, how is e.g. poll_choice_js supposed to access the arguments to set $form_state['args'] accordingly?

I re-added $form['#parameters'] in the attached patch but renamed it to #args to have in congruent to $form_state['args'] and make poll.module work.

Frando’s picture

Status: Needs work » Needs review

Oh, and setting to CNR to make the testbot run.. let's see where else it fails (apart from poll.module, which is still not completely correct).

Frando’s picture

FileSize
19.17 KB

Oh my, attached a wrong patch file in #9. This one is better.

Status: Needs review » Needs work

The last submitted patch failed testing.

Frando’s picture

Status: Needs work » Needs review
FileSize
30.27 KB

And so I went ahead and killed $form['#post']!
Completely.
Everyone now is just using $form_state['input'].
So instead of $form['#post'] on each and every element plus $form_state['post'], we now have one single place from where form input values are available.

Node preview is working, there's one failure though which I can't explain right now (about the value of the body field after preview, it works fine when doing it manually and I actually don't understand why that test even works in HEAD).
Some other failures removed.
Poll.module is still having issues with assigning the right values to the right fields when adding more choices. I started looking into this but got lost somewhere in form_builder and _form_builder_handle_input_element and poll_form after an hour or so without any results.. any poll.module fapi gurus around?

Does the theme layer have to access #post or $form_state['input'] at all? Shouldn't by the time it comes to theming #value be set accordingly?

Frando’s picture

FileSize
30.32 KB

Now, finally found the bug that made poll choice addition not work. In drupal_rebuild_form the input values weren't cleared. Fixed that, so poll tests should pass now.

catch’s picture

Status: Needs review » Needs work

Nitpicks:

+  array_shift($args); // remove the form id

Inline comments should be on a separate line, and "A sentence ending with a full stop."

+ * Retrieves a form from a constructor function, or from the cache if
+ * the form was built in a previous page-load. The form is then passesed
+ * on for processing, after and rendered for display if necessary.

Summary should be one line

"Retrieves a form from a constructor function or from the cache." maybe, then the rest:

+ * Retrieves a form from a constructor function, or from the cache if
+ * the form was built in a previous page-load. The form is then passed
+ * on for processing and rendering if necessary.

+ *   what the form did when the form is complete.

eh?

not to rerender
re-render needs a hyphen

+ *   - must_validate: Ordinarily a form is only validated once but there are times,
+ *     when a form is resubmitted internally and should be validated again. Setting
+ *     this to true will force that to happen.

re-submitted needs a hyphen
TRUE should be upper case.

+      // If the form method is set to get in the form_state but not
+      // the form, or vice versa, fix that.

Should be $form_state for clarity

+* @param $form_state
+ *   A keyed array containing the current state of the form.

spacing issues.

-  db_query("DELETE FROM {authmap} WHERE uid = %d AND aid = %d AND module = 'openid'", $form_state['#parameters'][2]->uid, $form_state['#parameters'][3]);
+  db_query("DELETE FROM {authmap} WHERE uid = %d AND aid = %d AND module = 'openid'", $form_state['#args'][0]->uid, $form_state['#args'][1]);

Should really be converted to db_delete()

merlinofchaos’s picture

Status: Needs work » Needs review
FileSize
32.78 KB

Rerolled with catch's comments, minus two items:

'resubmit' is a valid word by itself, no hyphen necessary. rerender isn't so I added the hyphen there.

I didn't convert the DELETE query to DBTNG. Partly I feel it's out of scope for the patch, partly I'm not up on DBTNG yet. That whole system should probably be converted as needed rather than converting just one query because we had to touch it.

catch’s picture

Re-submit turns out to be UK English, sorry about that. db_delete() - yeah it doesn't really matter either way in terms of query conversion. I'll re-roll with that if webchick requires it though.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
32.78 KB

Re-rolled, converted the one delete query that's touched to dbtng while I was in there.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Needs a re-roll for the tableselect changes it seems.

Frando’s picture

Status: Needs work » Needs review
FileSize
32.99 KB

Reroll attached. I also moved $form['#programmed'] to $form_state['programmed'] as it is similarly to #post not at all a $form property but a part of the form state. Previously we copied the value of $form['#programmed'] to every single element which was just ugly. No more!

No other changes apart from fixing tableselect and the better AHAH callback implementation in #360128: Security fix for simplified AHAH callbacks that went in today or yesterday (Great changes, btw!).

I have some more simplifications to FAPI in mind once this gets in. This one gotta be first, though.

Status: Needs review » Needs work

The last submitted patch failed testing.

Frando’s picture

Status: Needs work » Needs review
FileSize
34.04 KB

Take this, testbot!
Apart from fixing a broken test, I also moved the default values for an "empty" form_state array into a helper function to not have to manually specify it wherever e.g. drupal_process_form is called without using drupal_get_form / drupal_build_form. No other changes to the last patch.

chx’s picture

Status: Needs review » Reviewed & tested by the community

I am happy to see form API simplfiications done by someone not called Eaton or chx. Really. It was high time. That said, the patch is frickin' awesome. Lots of simplifications (like what kind of property is #cache where only the value FALSE counts? #nocache FTW), really but with simplifications come the possibility to extend. I love how GET got folded into FAPI at last.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

I find the documentation lacking:

  1. Retrieves a form from a constructor function. -- this function description does not imply that the form will be build. Because of that, the follow-up paragraph is confusing.
  2. retrieved from from the cache -- 2x from
  3. From the documentation it is unclear when you'd want to use the 'get' method.
  4. From the documentation it is unclear why you might not want to re-render the form; i.e. what is the purpose of 'rerender' and when to use it.
  5. From the documentation it is not clear why and when I'd want to use 'always_process'.
  6. For 'must_validate' the documentation write: there are times when a form is resubmitted internally and should be validated again but it is not at all clear when these 'times' are.

I think we should make an attempt at making the documentation a lot more helpful for those that want to make sense of the API.

merlinofchaos’s picture

For 'must_validate' the documentation write: there are times when a form is resubmitted internally and should be validated again but it is not at all clear when these 'times' are.

In this case it's a rare edge case of having the same form presented on the page multiple times for different purposes. I'm not sure that it's the right strategy to document every edge case when listing the options that make those edge cases happen.

Also, I'm not sure FAPI should document $_POST vs $_GET -- that's basic HTTP, HTML and PHP's interpretation -- I should think most people using FAPI need to have at least some familiarity with the
concept, even if it's a lot less used.

Dries’s picture

I'm OK with omitting why 'get' would be useful, if you feel strongly about it.

merlinofchaos’s picture

While we're discussing doc improvements, drupal_get_form() needs an @see to drupal_build_form() and drupal_build_form() doc compares itself to drupal_get_form; this is historical, however, and not appropriate for API doc (it would be good to pull out and use in the upgrade docs).

starbow’s picture

Ok, I see how you can turn off the redirect ($form_state['no_redirect']=TRUE), and I see how you can keep the form from being reoutputed ($form_state['rerender']=FALSE). Is there a way to output something else?

Frando’s picture

I think if 'rerender' is FALSE and 'no_redirect' is TRUE, you'll just be back in the menu callback once form processing is done, so you can return whatever you want, I think.

starbow’s picture

Unless I am missing something, that approach works if you have separate menu items/paths for the normal form submission and the ajax form submission. But I am not sure if this thread is the right place to dig into the question of ajax form submission - I was just checking if it was already part of this patch.

merlinofchaos’s picture

starbow: Yes, the caller can now output whatever it likes. The submit cannot, though, the caller of drupal_build_form has to do it.

Rerolling patch, will post in a few moments.

merlinofchaos’s picture

Status: Needs work » Needs review
FileSize
36.75 KB

This should be a documentation-only improvement.

starbow’s picture

@merlinofchaos: Looking at the patch again, I think I could do the ajax stuff I have in mind, if the new drupal_get_form function has hook calls before and after drupal_build_form. The pre hook could set the form_state args, and the post hook could output json. Is that something you would consider including in this patch, or should I wait until have this gets in?

merlinofchaos’s picture

Let's do that separately. Also, let's talk elsewhere. I have a couple of followups that I need to do and they may or may not coincide with what you want to do. IRC or email?

Frando’s picture

Status: Needs review » Reviewed & tested by the community

merlinofchaos, the documentation improvements are great. With the patch, there is now much more docs on $form_state and its properties than we have in HEAD now - awesome!
I reread the whole patch file and I think the docs are good for now.

As stated the patch is an important door opener towards more FAPI simplifications and less ugly AJAX/AHAH. No downsides at all. Better docs than in HEAD and 100% passes - RTBC.

sun’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
37.15 KB

Improved some of the PHPDoc. RTBC again if the bot passes.

wretched sinner - saved by grace’s picture

Status: Needs review » Reviewed & tested by the community

RTBC based on sun's comment #38

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

Frando’s picture

Status: Needs work » Needs review

The fail is "Image GD manipulation". I don't believe it's an issue with this patch, it gotta be problem with GD by the slave. Requesting retest, afterwards I'll set back to RTBC.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Bot #7 had issues with GD. Disabled now.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

I gave this another review, and it looked good. Thanks for the reviews, and incorporating the feedback (including mine). Committed!

c960657’s picture

Status: Fixed » Active

This introduced a regression. When registering an account via OpenID (i.e. log in using an OpenID identity that is unknown to Drupal), I now get the following errors:

  • Notice: Undefined index: args in drupal_retrieve_form() (line 412 of /home/chsc/www/drupal7/includes/form.inc).
  • Warning: array_merge(): Argument #2 is not an array in drupal_retrieve_form() (line 441 of /home/chsc/www/drupal7/includes/form.inc).
  • Notice: Undefined index: args in drupal_retrieve_form() (line 447 of /home/chsc/www/drupal7/includes/form.inc).

This was discovered using the tests awaiting review in #251245: openid.module needs tests.

c960657’s picture

The regression can be fixed by adding $form_state['args'] = array(); to openid_authentication(), but I'm not sure this is the proper way to fix this.

The Doxygen comment for drupal_retrieve_form() mentions @param ... Any additional arguments needed by the unique form constructor function, but AFAICT these arguments are no longer supported by the function.

desbeers’s picture

Status: Active » Needs review
FileSize
892 bytes

When outlining book-pages with javascript enabled; the form (node-edit and outline forms) gives notices on submission. With javascript disabled, all works well so that's why it passes all the book-test now I think. One-line patch attached.

desbeers’s picture

Category: task » bug

This is actually a bug-report as result of the task. Changed the status.

Dries’s picture

desbeers: care to elaborate on the fix. I'm not sure I understand the proposed patch.

desbeers’s picture

With pleasure Dries,

The notice I got when submitting a node to a new book was:

Notice: Undefined index: programmed in _form_builder_handle_input_element() (line 1108 of /Users/Desbeers/Sites/devel/drupal-7-orig/includes/form.inc).

  • I disabled JavaScript and tried again. This time there was no notice so the fault must be in function book_form_update() because that's the AJAX callback.
  • I had a look in the committed patch from #38 and compared function upload_js() with function book_form_update() as they are both AJAX callbacks.
  • Function upload_js() has 'programmed' set to FALSE in the $form_state array and function book_form_update() was lacking that, so I added it.
  • The notice was gone :-)

But why? I don't know for sure, but it looks like the default values from function form_state_defaults() are not available in the callback functions. Maybe somebody with more FAPI knowledge can tell if that's the normal behavior or if the callbacks should load the defaults instead of declaring themselves.

chx’s picture

FileSize
552 bytes
chx’s picture

So both functions that calls form_builder set up defaults. Those that call from outside of FAPI don't. This is a quick fix. Eventually we need to change form_builder to _form_builder too.

catch’s picture

Title: Form improvements from Views » (notice) Form improvements from Views

How come #251245: openid.module needs tests was able to go in without this?

c960657’s picture

The OpenID patch included the fix mentioned in #45.

catch’s picture

Status: Needs review » Needs work

As far as I can see this patch should either revert the change from the openid patch, or it's not needed at all - but I'm not sure which is the better fix.

sun’s picture

Status: Needs work » Needs review
FileSize
513 bytes

How about that instead?

Modules calling any FAPI internals are supposed to use form_state_defaults() anyway. Currently, you get ugly notices, because 'args' isn't setup for you.

If this won't fly, let's revert this issue to fixed and deal with the rest elsewhere.

merlinofchaos’s picture

Status: Needs review » Reviewed & tested by the community

I agree with sun. That should just be a default setting.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

Status: Fixed » Closed (fixed)
Issue tags: -Views in Core

Automatically closed -- issue fixed for 2 weeks with no activity.