| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | forms system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Views in Core |
Issue Summary
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:
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal_build_form.patch | 14.5 KB | Idle | Failed: Failed to apply patch. | View details |
Comments
#1
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.
#2
subscribe
#3
Subscribing.
This is enabler patch #0 for views in core. It'd be lovely to get folks with some FAPI chops on this.
#4
Marking for later review.
#5
#6
Subscribing.
Very interested in helping with getting views in core.
#7
subscribing
#8
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.
#9
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.
#10
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).
#11
Oh my, attached a wrong patch file in #9. This one is better.
#12
The last submitted patch failed testing.
#13
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?
#14
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.
#15
Nitpicks:
+ array_shift($args); // remove the form idInline 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 rerenderre-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()
#16
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.
#17
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.
#18
The last submitted patch failed testing.
#19
Re-rolled, converted the one delete query that's touched to dbtng while I was in there.
#20
The last submitted patch failed testing.
#21
Needs a re-roll for the tableselect changes it seems.
#22
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.
#23
The last submitted patch failed testing.
#24
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.
#25
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.
#26
I 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 againbut 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.
#27
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.
#28
I'm OK with omitting why 'get' would be useful, if you feel strongly about it.
#29
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).
#30
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?
#31
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.
#32
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.
#33
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.
#34
This should be a documentation-only improvement.
#35
@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?
#36
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?
#37
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.
#38
Improved some of the PHPDoc. RTBC again if the bot passes.
#39
RTBC based on sun's comment #38
#40
The last submitted patch failed testing.
#41
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.
#42
Bot #7 had issues with GD. Disabled now.
#43
I gave this another review, and it looked good. Thanks for the reviews, and incorporating the feedback (including mine). Committed!
#44
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:
This was discovered using the tests awaiting review in #251245: openid.module needs tests.
#45
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.#46
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.
#47
This is actually a bug-report as result of the task. Changed the status.
#48
desbeers: care to elaborate on the fix. I'm not sure I understand the proposed patch.
#49
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).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.
#50
#51
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.
#52
How come #251245: openid.module needs tests was able to go in without this?
#53
The OpenID patch included the fix mentioned in #45.
#54
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.
#55
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.
#56
I agree with sun. That should just be a default setting.
#57
Committed to HEAD.
#58
Automatically closed -- issue fixed for 2 weeks with no activity.