3 locations in core + countless repetitions in contrib justify a helper function to clean out submitted form values for further data storage processing.

Also makes upgrading code easier as developers no longer have to scan form.inc for all internal Form API variables. (At least some of my modules stored a lot of cruft after porting to D6 when new internal values were introduced into Form API.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

What about form_state_values_unset?

sun’s picture

What about form_state_clean_values() ?

sun’s picture

Putting the verb last. So, form_state_values_clean() it is. :)

sun’s picture

Added the removed values to doxygen.

However, I'd like to keep the separate unset()s, because that allows core newbies to grok their origin more easily, especially due to the inline comment.

c960657’s picture

How about simply removing all values generated by #type => 'submit' ?

sun’s picture

@c960657: There you go. If you get this test to pass, I'm happy with your proposal and fine with this approach. Otherwise, let's stick back to the totally trivial previous patch.

@chx: FYI, he proposed to remove the actual form values of buttons, which is the reason for the new PHP5-only and totally insane recursion. ;)

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

If you want to do that then why dont you keep a $form_state['button_array_parents'] or somesuch with stuff like

  // In _form_builder_handle_input_element:
  $form_state['buttons'][$button_type][] = $form; 
  $form_state['button_array_parents'][] = $form['#array_parents']; // New code

and then

  foreach ($form_state['button_array_parents'] as $button_array_parents) {
    $last_parent = array_pop($button_array_parents);
    $element = &$form;
    foreach ($button_array_parents as $parent) {
      $element = &$element[$parent];
    }
    unset($element[$last_parent]);
  }
chx’s picture

Sorry i got confused of what we want to unset. Disregard that post. Here is a better version then:

  foreach ($form_state['buttons'] as $button_type => $buttons) {
    foreach ($buttons as $button) {
      $parents = $button['#parents'];
      $values = &$form_state['values'];
      $last_parent = array_pop($parents);
      foreach ($parents as $parent) {
        $values = &$values[$parent];
      }
      unset($values[$last_parent]);
    }
  }
c960657’s picture

Status: Needs work » Needs review
FileSize
11.32 KB

Updated patch based on chx's suggestion.

sun’s picture

+++ includes/form.inc	7 Oct 2009 17:51:39 -0000
@@ -1293,18 +1285,57 @@ function _form_builder_ie_cleanup($form,
+ * @param &$values
+ *   Internal use only. Used to recursively iterate over $form_state['values'].
+ */
+function form_state_values_clean(&$form_state, &$values = NULL) {
+  if (!isset($values)) {
+    $values = &$form_state['values'];
+

Since there is no recursion anymore, this condition and setup of $values can go. :)

And also the second function argument along with its @param. :)

I'm on crack. Are you, too?

c960657’s picture

Fixed :)

sun’s picture

Status: Needs review » Reviewed & tested by the community

Rock solid.

sun’s picture

I think I would understand this.

And, no, this does not belong into the PHPDoc, because it does not matter for developers who want to use the function.

sun’s picture

Revised those docs with help from chx.

sun’s picture

Issue tags: +D7 API clean-up

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
+++ includes/form.inc	8 Oct 2009 01:32:01 -0000
@@ -1115,14 +1115,6 @@ function form_builder($form_id, $element
-  // We should keep the buttons array until the IE clean up function
-  // has recognized the submit button so the form has been marked
-  // as submitted. If we already know which button was submitted,
-  // we don't need the array.
-  if (!empty($form_state['submitted'])) {
-    unset($form_state['buttons']);
-  }

Someone needs to test that image buttons still work in IE after this patch.

+++ includes/form.inc	8 Oct 2009 01:32:01 -0000
@@ -1299,6 +1291,59 @@ function _form_builder_ie_cleanup($form,
+ * Helper function to remove internal Form API elements and buttons from submitted form values.

First line should be <= 80 chars.

+++ includes/form.inc	8 Oct 2009 01:32:01 -0000
@@ -1299,6 +1291,59 @@ function _form_builder_ie_cleanup($form,
+ * This function may be used when all submitted values of a form need to be
+ * processed for storage, so internal Form API values and button elements should
+ * not be contained. The following internal values are removed:

I trust that this is technically accurate, but it doesn't help me understand why we have such a function, and when/why I need to call it.

It looks like a fair number of core forms are calling this in their submit handlers, but it's not clear what the pattern is, and why some call it but not others.

+++ includes/form.inc	8 Oct 2009 01:32:01 -0000
@@ -1299,6 +1291,59 @@ function _form_builder_ie_cleanup($form,
+  // Remove internal Form API values.
+  unset($form_state['values']['form_id'], $form_state['values']['form_token'], $form_state['values']['form_build_id'], $form_state['values']['op']);

Curious. Is there ever a chance of a contributed module adding additional "internal" properties? Are we ok hard-coding this list?

+++ includes/form.inc	8 Oct 2009 01:32:01 -0000
@@ -1299,6 +1291,59 @@ function _form_builder_ie_cleanup($form,
+      // Find the value corresponding to this button. We iterate over the
+      // #parents of this button and move a reference to each parent in
+      // $form_state['values']. For example, if #parents is:
+      //   array('foo', 'bar', 'baz')
+      // then the corresponding $form_state['values'] part will look like this:
+      // array(
+      //   'foo' => array(
+      //     'bar' => array(
+      //       'baz' => 'button_value',
+      //     ),
+      //   ),
+      // )
+      // We start by (re)moving 'baz' to $last_parent, so we are able unset it
+      // at the end of the iteration. Initially, $values will contain a
+      // reference to $form_state['values'], but in the iteration we move the
+      // reference to $form_state['values']['foo'], and finally to
+      // $form_state['values']['foo']['bar'], which is the level where we can
+      // unset 'baz' (that is stored in $last_parent).

Um. WOW. :) Definitely vying for the most extensive documentation in all of FAPI. :D

However, before that, could we get just a summary sentence or two that just gives me the Cliff's notes? I read this entire thing and I still don't really understand the "gist."

+++ modules/simpletest/tests/form.test	8 Oct 2009 01:31:44 -0000
@@ -463,3 +463,69 @@ class FormsFormWrapperTestCase extends D
+    // Populate internal Form API values.
+    drupal_prepare_form('', $form, $form_state);
+    // Invoke form_builder() once to generate element default values in
+    // $form_state['values'].
+    form_builder('', $form, $form_state);
+    // By copying the auto-generated 'values' into 'input', we let form
+    // processing believe that this form was properly submitted, which in turn
+    // will lead to populated $form_state['buttons'].
+    $form_state['input'] = $form_state['values'];
+    // Lastly, process the form. Since there are no submit handlers in the game,
+    // this will not redirect.
+    drupal_process_form('', $form, $form_state);

Um. Why are we going through all these gymastics? Why not add an actual form function to form_test.module, and use it? That would be a much more realistic test.

This review is powered by Dreditor.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
10.58 KB

Incorporated all requested changes.

Someone needs to test that image buttons still work in IE after this patch.

Not needed, because the "funky IE button scenario" is handled right before that removed hunk:

  // Now that we've processed everything, we can go back to handle the funky
  // Internet Explorer button-click scenario.
  _form_builder_ie_cleanup($element, $form_state);

_form_builder_ie_cleanup() checks on its own whether the form has been submitted before performing any funky actions. We only removed the 'buttons' array, because the data was considered useless (which is what the quoted comment explains with "we don't need the array."). Now we have a nice use-case for that data, so we want to keep it. :)

joachim’s picture

+ * proceeds to storage. Next to button elements, the following internal values

Do you mean 'As well as' rather than 'Next'?

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed this patch to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -API clean-up, -D7 API clean-up

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