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.
Comment | File | Size | Author |
---|---|---|---|
#48 | interdiff.txt | 1.25 KB | sun |
#47 | d7_form_state_keys-859970-47.patch | 16.96 KB | jn2 |
#41 | d8_form_state_keys-859970-41.patch | 16.9 KB | jn2 |
#27 | d8_form_state_keys-859970-27.patch | 16.76 KB | jn2 |
#25 | d8_form_state_keys-859970-25.patch | 16.76 KB | jn2 |
Comments
Comment #1
sunUm. 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.
Comment #2
fagoYep, 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.
Comment #3
rfayWhoops. Lost this one. Will try to work on it in this weekend's docs sprint.
Comment #4
rfayOK, 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.
Comment #5
rfayComment #6
rfaySame patch to test D8
Comment #8
jn2 CreditAttribution: jn2 commentedIt'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.
Comment #9
jn2 CreditAttribution: jn2 commentedMoving to documentation component.
Comment #10
jn2 CreditAttribution: jn2 commentedI'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
Comment #11
jhodgdonJust 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
Comment #12
jhodgdonAlso 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
Comment #13
jn2 CreditAttribution: jn2 commentedThere 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.
Comment #14
jhodgdonI'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.
Comment #15
rfayComment #16
jn2 CreditAttribution: jn2 commented@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.
Comment #17
jn2 CreditAttribution: jn2 commentedHere'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.
Comment #18
rfayI would say 'storage' should not be in the D8 version, but should be in the D7 version.
Comment #19
sunmmm, 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. :/
Comment #20
sunwhoopsie, fixed line length issues.
Comment #21
sunAlso reverting this unnecessary change.
Powered by Dreditor.
Comment #22
sunComment #24
sunOh dear, sorry for the noise. d.o ate my attachment.
Comment #25
jn2 CreditAttribution: jn2 commented@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).
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.
Comment #26
sunLet'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.
Comment #27
jn2 CreditAttribution: jn2 commentedOkay, here's the same patch with 'programmed' moved before 'process_input'.
Comment #28
sunReviewed 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.
Comment #29
jhodgdonWow. Excellent work! D8/d7 please...
Comment #30
jn2 CreditAttribution: jn2 commentedDoes 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:
Open to expert opinion on how to handle these and any other changes the D7 patch might need. (sun?)
Comment #31
sunLet's discuss the backport patch after the current landed for D8.
Comment #32
jhodgdonAh, 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.
Comment #33
jn2 CreditAttribution: jn2 commented@sun, @jhodgdon
Thanks for the explanation. It does make a lot more sense to wait.
Comment #34
xjmTagging issues not yet using summary template.
Comment #35
xjmCrossposting #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.
Comment #36
jhodgdonThis has been sitting here for a long time, retesting...
Comment #37
jhodgdon#27: d8_form_state_keys-859970-27.patch queued for re-testing.
Comment #39
jhodgdonLooks like this needs a reroll. Probably a good novice project?
Comment #40
jn2 CreditAttribution: jn2 commentedWhoever 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.
Comment #41
jn2 CreditAttribution: jn2 commentedHere's the patch.
Comment #42
jhodgdonThe 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):
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].
Comment #43
catchCommitted to 8.x, marking 'to be ported' for 7.x.
Comment #44
jn2 CreditAttribution: jn2 commentedI 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.
Comment #45
jn2 CreditAttribution: jn2 commented#41: d8_form_state_keys-859970-41.patch queued for re-testing.
Comment #46
sunThe 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.
Comment #47
jn2 CreditAttribution: jn2 commentedHere's the patch.
I don't see any reason to document the internal keys, validate_handlers and submit_handlers.
Comment #48
sunThanks!
Comment #49
webchickCommitted and pushed to 7.x. Thanks!
Comment #50
rfayjn2++
Comment #51.0
(not verified) CreditAttribution: commentedUpdated issue summary.