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:
- 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. drupal_get_form()
is then made a very small wrapper arounddrupal_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.- Because args are now stored on the $form_state,
drupal_retrieve_form()
has been simplified. $form_state['method']
will allow fapi to sort of cleanly use $_GET as a form method.- Allowances for a form to declare it really really really does not want to be cached.
- There are probably issues with form caching; many of the settings on $form_state will need to be cached with a form.
- $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).
- A couple calls to drupal_retrieve_form() still need to be converted.
- $form['#parameters'] seems like it should be deprecated with this, need to make sure it fully is.
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:
Comment | File | Size | Author |
---|---|---|---|
#55 | drupal.form-state-defaults.patch | 513 bytes | sun |
#50 | form_state_defaults.patch | 552 bytes | chx |
#46 | book-js-1.patch | 892 bytes | desbeers |
#38 | drupal.build-form.patch | 37.15 KB | sun |
#34 | 322344-drupal-build-form.patch | 36.75 KB | merlinofchaos |
Comments
Comment #1
eaton CreditAttribution: eaton commentedI'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.
Comment #2
Pasquallesubscribe
Comment #3
webchickSubscribing.
This is enabler patch #0 for views in core. It'd be lovely to get folks with some FAPI chops on this.
Comment #4
sunMarking for later review.
Comment #5
webchickComment #6
timmillwoodSubscribing.
Very interested in helping with getting views in core.
Comment #7
drewish CreditAttribution: drewish commentedsubscribing
Comment #8
Frando CreditAttribution: Frando commentedThis 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.
Comment #9
Frando CreditAttribution: Frando commentedOK, 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.
Comment #10
Frando CreditAttribution: Frando commentedOh, 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).
Comment #11
Frando CreditAttribution: Frando commentedOh my, attached a wrong patch file in #9. This one is better.
Comment #13
Frando CreditAttribution: Frando commentedAnd 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?
Comment #14
Frando CreditAttribution: Frando commentedNow, 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.
Comment #15
catchNitpicks:
Inline comments should be on a separate line, and "A sentence ending with a full stop."
Summary should be one line
"Retrieves a form from a constructor function or from the cache." maybe, then the rest:
eh?
not to rerender
re-render needs a hyphen
re-submitted needs a hyphen
TRUE should be upper case.
Should be $form_state for clarity
spacing issues.
Should really be converted to db_delete()
Comment #16
merlinofchaos CreditAttribution: merlinofchaos commentedRerolled 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.
Comment #17
catchRe-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.
Comment #19
catchRe-rolled, converted the one delete query that's touched to dbtng while I was in there.
Comment #21
catchNeeds a re-roll for the tableselect changes it seems.
Comment #22
Frando CreditAttribution: Frando commentedReroll 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.
Comment #24
Frando CreditAttribution: Frando commentedTake 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.
Comment #25
chx CreditAttribution: chx commentedI 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.
Comment #26
Dries CreditAttribution: Dries commentedI find the documentation lacking:
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.retrieved from from the cache
-- 2x fromthere 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.
Comment #27
merlinofchaos CreditAttribution: merlinofchaos commentedIn 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.
Comment #28
Dries CreditAttribution: Dries commentedI'm OK with omitting why 'get' would be useful, if you feel strongly about it.
Comment #29
merlinofchaos CreditAttribution: merlinofchaos commentedWhile 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).
Comment #30
starbow CreditAttribution: starbow commentedOk, 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?
Comment #31
Frando CreditAttribution: Frando commentedI 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.
Comment #32
starbow CreditAttribution: starbow commentedUnless 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.
Comment #33
merlinofchaos CreditAttribution: merlinofchaos commentedstarbow: 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.
Comment #34
merlinofchaos CreditAttribution: merlinofchaos commentedThis should be a documentation-only improvement.
Comment #35
starbow CreditAttribution: starbow commented@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?
Comment #36
merlinofchaos CreditAttribution: merlinofchaos commentedLet'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?
Comment #37
Frando CreditAttribution: Frando commentedmerlinofchaos, 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.
Comment #38
sunImproved some of the PHPDoc. RTBC again if the bot passes.
Comment #39
wretched sinner - saved by grace CreditAttribution: wretched sinner - saved by grace commentedRTBC based on sun's comment #38
Comment #41
Frando CreditAttribution: Frando commentedThe 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.
Comment #42
chx CreditAttribution: chx commentedBot #7 had issues with GD. Disabled now.
Comment #43
Dries CreditAttribution: Dries commentedI gave this another review, and it looked good. Thanks for the reviews, and incorporating the feedback (including mine). Committed!
Comment #44
c960657 CreditAttribution: c960657 commentedThis 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:
This was discovered using the tests awaiting review in #251245: openid.module needs tests.
Comment #45
c960657 CreditAttribution: c960657 commentedThe 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.Comment #46
desbeers CreditAttribution: desbeers commentedWhen 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.
Comment #47
desbeers CreditAttribution: desbeers commentedThis is actually a bug-report as result of the task. Changed the status.
Comment #48
Dries CreditAttribution: Dries commenteddesbeers: care to elaborate on the fix. I'm not sure I understand the proposed patch.
Comment #49
desbeers CreditAttribution: desbeers commentedWith 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).
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.
Comment #50
chx CreditAttribution: chx commentedComment #51
chx CreditAttribution: chx commentedSo 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.
Comment #52
catchHow come #251245: openid.module needs tests was able to go in without this?
Comment #53
c960657 CreditAttribution: c960657 commentedThe OpenID patch included the fix mentioned in #45.
Comment #54
catchAs 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.
Comment #55
sunHow 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.
Comment #56
merlinofchaos CreditAttribution: merlinofchaos commentedI agree with sun. That should just be a default setting.
Comment #57
webchickCommitted to HEAD.