Posted by rfay on July 21, 2010 at 1:24am
8 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | documentation |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | needs backport to D7, Needs issue summary update, Novice |
Issue Summary
Problem/Motivation
Currently, Forms API properties (keys for $form_state) are documented above both drupal_get_form() and drupal_build_form(), which adds significant duplication. Furthermore, many properties are not documented.
Proposed resolution
Move documentation of all properties to drupal_build_form(), and add documentation for the undocumented properties. #859970-27: Cleanup form_api $form_state docs - keys in 2 places, missing some includes these changes along with some additional cleanup for this doxygen block.
Remaining tasks
- The current patch is for D8 only. Consider this for D7 backport once the D8 issue is resolved. (Some changes that will be needed for D7 are in #859970-30: Cleanup form_api $form_state docs - keys in 2 places, missing some.)
- Commit this patch first, then consider future improvements or corrections. E.g., once #597108: Consistent array key syntax in $form_state (underscores vs. spaces) committed, a new patch against the documentation will be needed.
User interface changes
None.
API changes
None.
Original report by @rfay
This patch adds a doxy snippet copied from drupal_build_form() into the consolidated $form_state documentation.
We missed this somehow when doing that section.
Change records for this issue
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal.wrapper_callback_doc.patch | 1.12 KB | Idle | PASSED: [[SimpleTest]]: [MySQL] 22,135 pass(es). | View details |
Comments
#1
Um. Not sure where this was discussed, but this patch only adds and does not (re)move.
That translates into double housekeeping. Double housekeeping is like pushing the button on a grave of vampires at 1 a.m. after walking 6 meters on a line of broken glass.
#2
Yep, that double house-keeping is weird. I'd vote for removing the docs from drupal_build_form() and refer to the general form_state docs instead. Let's have a single (but complete) place for this docs.
#3
Whoops. Lost this one. Will try to work on it in this weekend's docs sprint.
#4
OK, given the valid objection to repeated text, I've tried to consolidate the two sections that had redundant comments (the main form_api section and drupal_build_form()). There was far more than 'wrapper_callback'.
The idea here is to put the $form_state keys in a place where people can find them, since this is such an important array.
#5
#6
Same patch to test D8
#7
The last submitted patch, drupal.wrapper_callback_doc_859970_04.patch, failed testing.
#8
It's great to see this is being worked on, and that the list of keys is being consolidated. There are several keys which are still not included in either list:
Found in form_state_keys_no_cache().
buttons
clicked_button (deprecated)
complete form
groups
rebuild_info
submit_handlers
submitted
validate_handlers
All of these were also included in code in form.inc, except: clicked_button, validate_handlers and submit_handlers. So I'm guessing all three of those are deprecated.
Others found grepping through form.inc.
executed
has_file_element
process_input
programmed
There may be others, but adding these (except any that are deprecated) would make a fairly complete list.
#9
Moving to documentation component.
#10
I've made a start on the descriptions for the missing keys. Someone more experienced with D7/8 Form API needs to check this for accuracy:
- buttons: An array of names of the buttons in the form.
- complete form: Copy of the complete form; same as $form.
- groups: Array for handling fieldsets, including vertical tabs.
- executed: Boolean flag. If TRUE, the form has been processed and executed. Defaults to FALSE.
- has_file_element: Boolean flag. If TRUE, there is a file element and Drupal will set the form encoding.
- process_input: Boolean flag. If TRUE, we have correct form submission. This is always TRUE for programmed forms coming from drupal_form_submit() (see 'programmed' key), or if the form_id coming from the POST data is set and matches the current form_id.
- programmed: Boolean flag. If TRUE, form was submitted programmatically, usually invoked via drupal_form_submit(). Defaults to FALSE.
- rebuild_info: Array of information used to rebuild form when 'rebuild' is set to TRUE.
- submitted: Boolean flag. If TRUE, form has been submitted. Defaults to FALSE.
Although 'clicked_button' is apparently deprecated, it is used in code in field.form.inc., lines 376 and 401. So assuming 'validate_handlers' and 'submit_handlers' are also deprecated, I would suggest also including the following:
The following keys are deprecated in Drupal 7:
- clicked_button
- validate_handlers
- submit_handlers
#11
Just as a note: a couple of issues have been marked as a duplicate of this and may contain information:
#1168888: 'complete form' key missing from $form_state documentation
#1170030: $form_state doc in two places and incomplete
#12
Also this one, which definitely has some information that needs incorporating:
#284468: Document that $form_state['redirect'] can contain url options like query, not just a path
#13
There is another place $form_state keys are documented, but only for a specific function: drupal_redirect_form (http://api.drupal.org/api/drupal/includes--form.inc/function/drupal_redi...). The redirect options are complicated enough that keeping them in their own space would be less confusing, linking from here (as it already does).
That would seem the logical place to fix the documentation problem in #284468: Document that $form_state['redirect'] can contain url options like query, not just a path, in which case, it's not necessarily a duplicate.
EDITED: Also, as far as I can tell, there is no duplication of information between the redirect list and the main list that this patch is fixing.
#14
I'm fine with reopening the other issue and keeping the redirect info there, as long as both places link to each other and explain what information is available in the other spot.
#15
#16
@jhodgdon
Sounds reasonable for the redirect issue. I reopened it with a summary of this discussion.
rfay and I discussed next steps for this issue on IRC this morning. I'll reroll the patch for 8.x.
#17
Here's the patch rerolled for D8. It's in alpha order now, and I've also added most of the keys in #10. As rfay pointed out on IRC, 'clicked_button' doesn't belong in D8. I left in the reference to it under 'triggering_element', but maybe that should also go for D8. 'validate_handlers' and 'submit_handlers' are not included pending information on whether they are deprecated or not. Descriptions from #10 are also included, except 'rebuild_info', which I'm sure at best was only partially correct.
I wonder if the 'storage' key should be rewritten. The information about using keys with module names is useful, but the 'storage' key itself sounds deprecated.
Now the list needs some expert review.
#18
I would say 'storage' should not be in the D8 version, but should be in the D7 version.
#19
mmm, not happy about moving all of these details into the high-level Form API documentation. drupal_build_form() is the central hub for kick-starting a form in whatever way. Let's turn it around.
Also not happy about the quotes (
'somekey') in the phpDoc. These should be omitted. See http://drupal.org/node/1354 for details and examples.I'd recommend to leave 'storage' as is for now. Its description is still valid for D8, and we didn't have any kind of discussion pertaining to module namespaces for $form_state keys yet.
Attached patch additionally tweaks and corrects some of the descriptions.
Unfortunately, this puts myself out of the group of people who can RTBC this patch. :/
#20
whoopsie, fixed line length issues.
#21
+++ b/includes/form.inc@@ -266,8 +277,8 @@ function drupal_get_form($form_id) {
- * Further $form_state properties controlling the redirection behavior after
- * form submission may be found in drupal_redirect_form().
+ * See drupal_redirect_form() for $form_state keys that affect the redirection
+ * behavior after form submission.
Also reverting this unnecessary change.
Powered by Dreditor.
#22
#23
The last submitted patch, drupal8.form-state-keys.21.patch, failed testing.
#24
Oh dear, sorry for the noise. d.o ate my attachment.
#25
@sun
Big improvement. You didn't mention 'validate_handlers' and 'submit_handlers', so I assume that means they are deprecated and don't belong in the D8 version.
The existing docs are inconsistent about the quotes on key names. I chose to put them all in quotes because quotes were used in the descriptions. I've read but not memorized node/1354 and would have gone the other way if I'd realized it was in the guidelines. ;)
#21: I changed that sentence because the word "further" implies you'll find additional keys at drupal_redirect_form(), rather than more information about 4 of the same keys listed here. I edited the language in the patch a bit (better than the awkward sentence I had in there before).
+++ b/includes/form.inc@@ -232,32 +171,104 @@ function drupal_get_form($form_id) {
+ * to TRUE to do this. A prominent example is an AJAX-enabled form, in which
+ * ajax_process_form() enables form caching for all forms that include an
+ * element with the #ajax property. (The AJAX handler has no way to build
Changed AJAX to Ajax to follow the style guide (http://drupal.org/style-guide/content#relatedwords).
I can see good reason not to go with alpha order to keep things like 'build' and 'rebuild', or 'values' and 'input' together in the list. But if we're going that direction, it would be useful to group other keys as well. I moved the keys that affect redirection together, and the keys that pertain to submission together.
Powered by Dreditor.
#26
Let's move 'programmed' before 'process_input' then.
Afterwards, I'll review this once more, and would recommend to commit the patch as a first measure then. I did not change the order of documented keys any further, since we're changing a lot of quality documentation here, requiring in-depth reviews to ensure the changes are correct - a patch that attempts to do this should only change the order without any other changes.
#27
Okay, here's the same patch with 'programmed' moved before 'process_input'.
#28
Reviewed the changes since my last patch, this is ready to fly.
If any issues are remaining or will come up, I'd still highly recommend to commit this patch as is first. I don't want to have to re-review this patch once again for technical correctness.
#29
Wow. Excellent work! D8/d7 please...
#30
Does the D7 version of the patch need to wait until the D8 is committed?
AFAIK, all that needs to be changed from one to the other is adding the deprecated keys. If I make the patch, I will simply list them under the main list like so:
The following keys are deprecated:- clicked_button
- validate_handlers
- submit_handlers
Open to expert opinion on how to handle these and any other changes the D7 patch might need. (sun?)
#31
Let's discuss the backport patch after the current landed for D8.
#32
Ah, yes. The patch in #27 is for D8 only, then please mark this as d7/to be ported.
And yes we should always wait for d8 before making the d7 patch (unless they are the same), because (a) it's possible the committers won't accept the patch and the d7 one would then also need to be changed and (b) it's kind of confusing to have lots of patches floating around for review on the same issue, and it's less confusing if there's only one being reviewed at a time.
#33
@sun, @jhodgdon
Thanks for the explanation. It does make a lot more sense to wait.
#34
Tagging issues not yet using summary template.
#35
Crossposting #597108: Consistent array key syntax in $form_state (underscores vs. spaces). If that patch is committed, the text added here should be updated.
Edit: Changing my earlier comment, as sun's suggestion above is to commit the patch above as-is and then commit any subsequent changes.
#36
This has been sitting here for a long time, retesting...
#37
#27: d8_form_state_keys-859970-27.patch queued for re-testing.
#38
The last submitted patch, d8_form_state_keys-859970-27.patch, failed testing.
#39
Looks like this needs a reroll. Probably a good novice project?
#40
Whoever re-rolls this should add the underscore to the complete_form key. See #597108: Consistent array key syntax in $form_state (underscores vs. spaces).
EDITED: I'll do the re-roll.
#41
Here's the patch.
#42
The previous patch was RTBC for Drupal 8 only.
I have verified that this patch produces the same end result, except that it is missing this one change (which I think is fine to have omitted):
- * set $form_state['rebuild'] to cause the form processing to bypass submit+ * set $form_state['rebuild'] to cause form processing to bypass submit
And it also has a small amount of additional information in one place that looks fine to me.
So let's get this into D8, and then consider the d7 patch (see #30) [please mark "patch to be ported" after committing].
#43
Committed to 8.x, marking 'to be ported' for 7.x.
#44
I think the D8 patch works for D7, with removing the underscore from the 'complete path' key. Then it needs those 3 deprecated keys included somehow. I favor a separate list under the main list, but they could be included in the main list. In either case, they would be marked deprecated.
The three deprecated keys:
- clicked_button
- validate_handlers
- submit_handlers
I'll retest the patch for D7.
#45
#41: d8_form_state_keys-859970-41.patch queued for re-testing.
#46
The keys validate_handlers and submit_handlers are not deprecated in any way. They are actively used by form_execute_handlers().
If we want to document those two (internal) keys, let's create a separate issue for doing so.
Hence, the only change for D7 is the renamed 'complete form' (without underscore) and that clicked_button is deprecated. To be documented like this:
- clicked_button: Deprecated. Use triggering_element instead.
#47
Here's the patch.
I don't see any reason to document the internal keys, validate_handlers and submit_handlers.
#48
Thanks!
#49
Committed and pushed to 7.x. Thanks!
#50
jn2++
#51
Automatically closed -- issue fixed for 2 weeks with no activity.