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

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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Status: Needs review » Needs work

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.

fago’s picture

Title: (doc only) Add $form_state['wrapper_callback'] to description of $form_state in form_api » (doc only) Cleanup form_api $form_state docs

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.

rfay’s picture

Assigned: Unassigned » rfay

Whoops. Lost this one. Will try to work on it in this weekend's docs sprint.

rfay’s picture

Status: Needs work » Needs review
FileSize
9.02 KB

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.

rfay’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
rfay’s picture

Same patch to test D8

Status: Needs review » Needs work

The last submitted patch, drupal.wrapper_callback_doc_859970_04.patch, failed testing.

jn2’s picture

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.

jn2’s picture

Title: (doc only) Cleanup form_api $form_state docs » Cleanup form_api $form_state docs - keys in 2 places, missing some
Component: forms system » documentation

Moving to documentation component.

jn2’s picture

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

jhodgdon’s picture

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

jhodgdon’s picture

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

jn2’s picture

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.

jhodgdon’s picture

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.

rfay’s picture

Assigned: rfay » Unassigned
jn2’s picture

Assigned: Unassigned » jn2

@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.

jn2’s picture

Assigned: jn2 » Unassigned
Status: Needs work » Needs review
FileSize
16.03 KB

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.

rfay’s picture

I would say 'storage' should not be in the D8 version, but should be in the D7 version.

sun’s picture

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. :/

sun’s picture

whoopsie, fixed line length issues.

sun’s picture

Status: Needs review » Needs work
FileSize
14.4 KB
+++ 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.

sun’s picture

The last submitted patch, drupal8.form-state-keys.21.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
14.4 KB

Oh dear, sorry for the noise. d.o ate my attachment.

jn2’s picture

@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.

Also not happy about the quotes ('somekey') in the phpDoc. These should be omitted. See http://drupal.org/node/1354 for details and examples.

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.

sun’s picture

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.

jn2’s picture

Okay, here's the same patch with 'programmed' moved before 'process_input'.

sun’s picture

Status: Needs review » Reviewed & tested by the community

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.

jhodgdon’s picture

Wow. Excellent work! D8/d7 please...

jn2’s picture

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?)

sun’s picture

Let's discuss the backport patch after the current landed for D8.

jhodgdon’s picture

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.

jn2’s picture

@sun, @jhodgdon

Thanks for the explanation. It does make a lot more sense to wait.

xjm’s picture

Tagging issues not yet using summary template.

xjm’s picture

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.

jhodgdon’s picture

This has been sitting here for a long time, retesting...

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update, +Needs backport to D7

The last submitted patch, d8_form_state_keys-859970-27.patch, failed testing.

jhodgdon’s picture

Issue tags: +Novice

Looks like this needs a reroll. Probably a good novice project?

jn2’s picture

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.

jn2’s picture

Status: Needs work » Needs review
FileSize
16.9 KB

Here's the patch.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

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].

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 8.x, marking 'to be ported' for 7.x.

jn2’s picture

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.

jn2’s picture

Status: Patch (to be ported) » Needs review

#41: d8_form_state_keys-859970-41.patch queued for re-testing.

sun’s picture

Status: Needs review » Needs work

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.

jn2’s picture

Status: Needs work » Needs review
FileSize
16.96 KB

Here's the patch.

I don't see any reason to document the internal keys, validate_handlers and submit_handlers.

sun’s picture

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

Thanks!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

rfay’s picture

jn2++

Status: Fixed » Closed (fixed)
Issue tags: -Needs issue summary update, -Novice, -Needs backport to D7

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.