The great form.inc cleanup tour, part 1

Frando - June 6, 2009 - 15:34
Project:Drupal
Version:7.x-dev
Component:forms system
Category:task
Priority:normal
Assigned:Frando
Status:needs work
Issue tags:Needs Documentation
Description

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

AttachmentSizeStatusTest resultOperations
fapicleanup1.patch23.48 KBIdleFailed: 11865 passes, 0 fails, 96 exceptionsView details | Re-test

#1

Frando - June 6, 2009 - 16:12

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.

AttachmentSizeStatusTest resultOperations
fapicleanup1.patch33.51 KBIdleFailed: 11749 passes, 0 fails, 96 exceptionsView details | Re-test

#2

System Message - June 6, 2009 - 17:15
Status:needs review» needs work

The last submitted patch failed testing.

#3

Frando - June 6, 2009 - 18:02
Status:needs work» needs review

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

AttachmentSizeStatusTest resultOperations
fapicleanup1.patch33.52 KBIdleFailed: Failed to apply patch.View details | Re-test

#5

Frando - June 17, 2009 - 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.

#6

System Message - June 24, 2009 - 01:25
Status:needs review» needs work

The last submitted patch failed testing.

#7

moshe weitzman - June 24, 2009 - 02:55

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

#8

Frando - June 24, 2009 - 08:53
Status:needs work» needs review

Patch still applies for me with some offset. Retesting.

#9

eaton - June 24, 2009 - 13:44

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

#10

System Message - June 28, 2009 - 03:55
Status:needs review» needs work

The last submitted patch failed testing.

#11

webchick - June 28, 2009 - 04:04
Status:needs work» needs review

Sigh @ Slave #26...

#12

moshe weitzman - June 30, 2009 - 02:04

What does it take to get a re-test?

#13

System Message - June 30, 2009 - 10:15
Status:needs review» needs work

The last submitted patch failed testing.

#14

Wim Leers - June 30, 2009 - 21:07

Subscribing :)

#15

plach - July 5, 2009 - 22:52

subscribe

#16

Frando - July 5, 2009 - 23:05
Status:needs work» needs review

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.

AttachmentSizeStatusTest resultOperations
fapicleanup.patch33.25 KBIdlePassed: 11563 passes, 0 fails, 0 exceptionsView details | Re-test

#17

chx - July 6, 2009 - 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 :)

#18

Dries - July 6, 2009 - 11:55
Status:reviewed & tested by the community» fixed

Committed to CVS HEAD. Thanks.

#19

eaton - July 6, 2009 - 13:32

And the ewoks dance.

Nice work, Frando. Thank you!

#20

moshe weitzman - July 6, 2009 - 15:52
Status:fixed» active

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

#21

chx - July 6, 2009 - 23:39
Status:active» needs work

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

#22

grendzy - November 10, 2009 - 20:44

fixing tag

#23

Frando - November 10, 2009 - 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?

 
 

Drupal is a registered trademark of Dries Buytaert.