The great form.inc cleanup tour, part 1 [#483778]

This patch cleans up various parts of form.inc. It does not change any functionality.

This patch and a few more similar ones are long overdue, form.inc has too much code that can be simplified thanks to newer FAPI techniques.

Here's what is changed by the patch:

Set $form_state['submitted'] = TRUE for programmed forms in drupal_form_submit(), not in drupal_process_form()
Introduce a new $form_state['process_input'] flag that is set to TRUE if the form has input and the form ID from $_POST matches or if the form is programmed. This check was performed many times seperately before. Now it's much cleaner.
Better variable names: $form is now $element in form_builder et. al. form_builder processes elements, after all. Also renamed $form_item to $element in some functions. Consistency and logical names ++.
Renamed some other variables and added comments here and there. Updated some comments that stayed the same since 2006 or something while the code around 'em changed. Will do more of those in a follow up.
Made several if() checks simpler. We have default values for $form_state, so no need to do an isset() each time.
Removed all static vars from form_builder. They are ugly. They rely on global state. They have to go. That's what $form_state is for.
We used to pass a $complete_form variable containing a complete form to various process and validate functions. We needed static vars for this. So instead, now we set a $form_state['complete form'] property that contains a copy of the complete form.

As I said, no functionality changes, just cleanup. There's more stuff like this in form.inc that should be done, but this is a start and keeps the patch size reviewable. Patches like this or upcoming ones in the same spirit should also finally make it a bit easier for new developers to understand form.inc by simplifying the code flow. Would be great to get this in soon. I have more of those in the works..

Comment	File	Size	Author
#16	fapicleanup.patch	33.25 KB	Frando
#16
#3	fapicleanup1.patch	33.52 KB	Frando
#3
#1	fapicleanup1.patch	33.51 KB	Frando
#1
	fapicleanup1.patch	23.48 KB	Frando

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Frando CreditAttribution: Frando commented 6 June 2009 at 16:12

File	Size
fapicleanup1.patch	33.51 KB

Used - instead of * for listing things in comments. Also caught a couple more variable renames. Will stop now to keep the patch reviewable. If all tests pass this should basically be ready to go I think.

Comment #2

6 June 2009 at 17:15

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #3

Frando CreditAttribution: Frando commented 6 June 2009 at 18:02

Status:

Needs work

» Needs review

File	Size
fapicleanup1.patch	33.52 KB

This time without notices. Removed one isset check too much.

Comment #5

Frando CreditAttribution: Frando commented 17 June 2009 at 00:33

Bump. Any FAPI-savvy person around to give this a quick review? It's really just cleanup, but IMO very much needed. Would be great to get this in soon, as every other FAPI patch will break it.

Comment #6

24 June 2009 at 01:25

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #7

moshe weitzman CreditAttribution: moshe weitzman commented 24 June 2009 at 02:55

subscribe. all the changes sound proper to me. i'll ask eaton or chx to take a look at this.

Comment #8

Frando CreditAttribution: Frando commented 24 June 2009 at 08:53

Status:

Needs work

» Needs review

Patch still applies for me with some offset. Retesting.

Comment #9

eaton CreditAttribution: eaton commented 24 June 2009 at 13:44

Looking over it in detail now but this looks splendid. I owe you many, many beers.

Comment #10

28 June 2009 at 03:55

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #11

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 28 June 2009 at 04:04

Status:

Needs work

» Needs review

Sigh @ Slave #26...

Comment #12

moshe weitzman CreditAttribution: moshe weitzman commented 30 June 2009 at 02:04

What does it take to get a re-test?

Comment #13

30 June 2009 at 10:15

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #14

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 30 June 2009 at 21:07

Subscribing :)

Comment #15

plach

he/him

Italian

Venezia

CreditAttribution: plach commented 5 July 2009 at 22:52

Comment #16

Frando CreditAttribution: Frando commented 5 July 2009 at 23:05

Status:

Needs work

» Needs review

File	Size
fapicleanup.patch	33.25 KB

Reroll. Now please, this is really needed, doesn't do any functionality changes and is lying around passing the testbot for a months now, waiting to be broken again by another patch. So anyone fapi-savvy please give this a quick review and rtbc it.

Comment #17

chx CreditAttribution: chx commented 6 July 2009 at 07:37

Status:

Needs review

» Reviewed & tested by the community

I can do that... I like this, actually. Some of the code you are touching noone touched since 2005 September :)

Comment #18

Dries CreditAttribution: Dries commented 6 July 2009 at 11:55

Status:

Reviewed & tested by the community

» Fixed

Committed to CVS HEAD. Thanks.

Comment #19

eaton CreditAttribution: eaton commented 6 July 2009 at 13:32

And the ewoks dance.

Nice work, Frando. Thank you!

Comment #20

moshe weitzman CreditAttribution: moshe weitzman commented 6 July 2009 at 15:52

Status:

Fixed

» Active

Do we need to mention any API changes in update docs? If no, please move back to fixed.

Comment #21

chx CreditAttribution: chx commented 6 July 2009 at 23:39

Status:

Active

» Needs work

A documentation on the various form_state keys now is in order IMO.

Comment #22

grendzy CreditAttribution: grendzy commented 10 November 2009 at 20:44

Issue tags:

+Needs documentation

fixing tag

Comment #23

Frando CreditAttribution: Frando commented 10 November 2009 at 20:53

What would be the proper place to document the $form_state keys? Forms API reference in CVS contrib (http://api.drupal.org/api/drupal/developer--topics--forms_api_reference....)? I fear that needs a general update first as it really is based on FAPI 1.. Other suggestions?

Comment #24

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 7 December 2010 at 23:05

Issue tags:

-Needs documentation

+Needs documentation updates

The idea would be to document any changes in the 6/7 update guide. Tagging accordingly with new tagging scheme.

Hopefully the relevant function doc was changed with the patch? If not, please comment here and add back the generic "needs documentation" tag.

Comment #25

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 8 June 2011 at 20:43

Looking at the patch that was committed (#16) I can see the following API changes:
a)

@@ function drupal_build_form($form_id, &$f
...
-    if ($cacheable && !empty($form['#cache']) && empty($form['#no_cache'])) {
+    if ($cacheable && !empty($form_state['cache']) && empty($form['#no_cache'])) {

Looks like $form['#cache'] became $form_state['cache'] ?

b) New elements to $form_state: cache, rebuild, redirect, complete_form, process_input

I don't think (b) needs documenting in the 6/7 module update guide, since it would not require any changes to modules, right? And the new parts of $form_state for documentation are being worked on in these issues:
#859970: Cleanup form_api $form_state docs - keys in 2 places, missing some
#284468: Document that $form_state['redirect'] can contain url options like query, not just a path
so that part of the "needs documentation" tag has been at least recognized elsewhere.

So (a) remains.

Comment #26

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 8 June 2011 at 20:46

Status:

Needs work

» Postponed (maintainer needs more info)

So... I'm a bit confused about this. How does this change of $form['#cache'] to $form_state['cache'] affect module developers (or does it)?

Can someone please provide a short summary, and also, if I missed anything -- is there anything else in this issue/patch that affects module developers, that we should mention in the 6/7 module update guide?