During fixing #632148: cruft getting saved to {users} I discovered that form_state_values_clean() itself pollutes the form values when there are unused submit buttons. The problem is that a simple

<?php
$var
= &$array['non existing key'];
?>

creates the previously non existing key. Attached patch fixes form_state_values_clean() to don't further pollute the form values. However I fear the code to clean up the button values might leave empty parents in the array. Anyway that's another problem.

Related bug reports

Files: 
CommentFileSizeAuthor
#45 drupal.form-state-values-clean.45.patch5.95 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,250 pass(es).
[ View ]
#45 drupal.form-state-values-clean.45.test-only.patch3.77 KBsun
FAILED: [[SimpleTest]]: [MySQL] 39,265 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#43 drupal8.form-state-values-clean-revert.43.patch1.64 KBsun
FAILED: [[SimpleTest]]: [MySQL] 37,054 pass(es), 6 fail(s), and 6 exception(s).
[ View ]
#37 form-state-values-clean-635046-37.patch5.35 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 36,915 pass(es).
[ View ]
#32 drupal8.form-state-values-clean.32.patch5.43 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.form-state-values-clean.32.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#29 form-state-values-clean-635046-29.patch6.29 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 33,774 pass(es).
[ View ]
#26 form-state-values-clean-635046.patch6.19 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 33,705 pass(es).
[ View ]
#22 test-managed-file-element-form-value-clearance-635046.patch3.94 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] 33,695 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#20 core.635046-2.patch2.44 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 32,930 pass(es).
[ View ]
#17 drupal8.form-state-values-clean.14.patch2.42 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.form-state-values-clean.14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 form-635046.patch2.09 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] 32,918 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#11 drupal8.form-state-values-clean.11.patch1.59 KBsun
FAILED: [[SimpleTest]]: [MySQL] 32,927 pass(es), 5 fail(s), and 0 exception(es).
[ View ]
#5 drupal.form-state-values-clean-clean.5.patch1.89 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.form-state-values-clean-clean.5.patch. See the log in the details link for more information.
[ View ]
form_pollution.patch537 bytesfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form_pollution.patch.
[ View ]

Comments

Status:Active» Needs review

looks right to me. could use another fapi knowledgeable opinion.

form_pollution.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, form_pollution.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.89 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.form-state-values-clean-clean.5.patch. See the log in the details link for more information.
[ View ]

Shouldn't we simply re-use existing helpers?

Status:Needs review» Needs work

The last submitted patch, drupal.form-state-values-clean-clean.5.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal.form-state-values-clean-clean.5.patch, failed testing.

Priority:Normal» Critical
Status:Needs work» Reviewed & tested by the community

#5 fixes to problem.

Note that this bug makes a system settings form to crash if using a FAPI element like "managed_file". For example the next form:

<?php
function mymodule_settings($form, $form_state) {
 
$form['mymodule_icon'] = array(
   
'#type' => 'managed_file',
   
'#title' => t('Icon'),
   
'#upload_location' => 'public://icons/',
   
'#default_value' => variable_get('mymodule_icon', 0),
  );
  return
system_settings_form($form);
}
?>

will crash with:

PHP Fatal error:  Cannot unset string offsets in /var/www/html/includes/form.inc on line 2126

without the patch from #5.

Priority:Critical» Normal
Status:Reviewed & tested by the community» Needs work

The patch in 5 is not in git format, moving back to normal as through numerous triages this has never been promoted in priority before.

Version:7.x-dev» 8.x-dev
Assigned:Unassigned» sun
Priority:Normal» Major
Status:Needs work» Needs review
Issue tags:+needs backport to D7
StatusFileSize
new1.59 KB
FAILED: [[SimpleTest]]: [MySQL] 32,927 pass(es), 5 fail(s), and 0 exception(es).
[ View ]

Attached patch should do it. (and works based on my own testing)

Status:Needs review» Needs work

The last submitted patch, drupal8.form-state-values-clean.11.patch, failed testing.

Priority:Major» Critical

The patch in 5 is not in git format, moving back to normal as through numerous triages this has never been promoted in priority before.

In #9 I gave an example that prove this bug is critical. A simple settings form (based on system_settings_form()) is crashing. This is really critical. The patch may not be in git format... that is NOT critical.

Status:Needs work» Needs review
StatusFileSize
new2.09 KB
FAILED: [[SimpleTest]]: [MySQL] 32,918 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Changed that patch to be $ git format-patch like.

Priority:Critical» Major
Status:Needs review» Needs work

The git comment was in relationship to the patch being rbtc given that it no longer applies the patch can not have been tested because it no longer works.

The none critical comment was because it isn't according to the statuses here http://drupal.org/node/45111 - I argee it should be major based on this defintion.

'An example would be a PHP error which is only triggered under rare circumstances or which affects only a small percentage of all users'

Status:Needs work» Needs review

Cross post on status

Status:Needs review» Needs work
StatusFileSize
new2.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.form-state-values-clean.14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Hm. Didn't expect those failures. Admittedly, I didn't run that dedicated test.

Ah, I get it now.

Status:Needs work» Needs review

Status:Needs review» Needs work

Applied #17 and tested with the simple system settings form from #9. Unfortunately the submit crashes again with:

PHP Fatal error:  Cannot unset string offsets in /var/www/html/includes/form.inc on line 2108

This is happens right now too, without any patch, in Drupal 7.8. But #11 worked for me.

StatusFileSize
new2.44 KB
PASSED: [[SimpleTest]]: [MySQL] 32,930 pass(es).
[ View ]

In some conditions (eg. system settings form in #9) $values obtained here (from #17 ):

<?php
$values
= &drupal_array_get_nested_value($form_state['values'], $parents, $key_exists);
?>

is not an array. I don't have time to investigate why but seems that is happen on 'managed_file' buttons. I just added an additional check in

<?php
   
if ($key_exists && is_array($values)) {
      unset(
$values[$last_parent]);
    }
?>

to be sure that unset() will not fail with that horrible error. This is fixing also cases like #9.

Attached a patch.

Issue tags:+Needs tests

It looks like the example in #9 could go into an automated tests somewhere.

Status:Needs work» Needs review
StatusFileSize
new3.94 KB
FAILED: [[SimpleTest]]: [MySQL] 33,695 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

This patch is containing only the new test Form API > Form state values clearance (advanced) (class FormStateValuesCleanAdvancedTestCase extends DrupalWebTestCase).

Run the test to see how posting the form crashes in form_state_values_clean(). Then comment out form_state_values_clean($form_state); from form_test_form_state_values_clean_advanced_form_submit() and retest. You will notice a HTTP 200.

If is what you expect for this test switch back to "needs work" so I will provide the whole patch.

Status:Needs review» Needs work

The last submitted patch, test-managed-file-element-form-value-clearance-635046.patch, failed testing.

Status:Needs work» Needs review

The patch from #22 failed in his own test, which is expected. Moving back to "needs review".

Looks good to me, can you post the combined patch with the fix?

StatusFileSize
new6.19 KB
PASSED: [[SimpleTest]]: [MySQL] 33,705 pass(es).
[ View ]

Sure.

Assigned:sun» claudiu.cristea

Status:Needs review» Needs work

Thanks @claudiu.cristea for adding the tests! They look pretty good to me. Here are a few code style cleanup pointers:

+++ b/includes/common.incundefined
@@ -6377,7 +6377,8 @@ function drupal_array_get_nested_value(array &$array, array $parents, &$key_exis
-      return NULL;
+      $null = NULL;
+      return $null;

Uh. I don't get why we are doing this? Since sun added it I'm assuming it's somehow needed for the patch to function, but can we add a code comment explaining it?

+++ b/modules/simpletest/tests/form.testundefined
@@ -1112,6 +1112,51 @@ class FormStateValuesCleanTestCase extends DrupalWebTestCase {
+ * Test $form_state clearance with form elements having buttons.

Should begin "Tests" rather than "Test."

+++ b/modules/simpletest/tests/form_test.moduleundefined
@@ -803,6 +811,33 @@ function form_test_form_state_values_clean_form_submit($form, &$form_state) {
+ * Form builder for form_state_values_clean() test.

This text should probably be:

Form constructor for the form_state_values_clean() test.

See: http://drupal.org/node/1354#forms

+++ b/modules/simpletest/tests/form_test.moduleundefined
@@ -803,6 +811,33 @@ function form_test_form_state_values_clean_form_submit($form, &$form_state) {
+ * Form submit handler for form_state_values_clean() test form.

Similarly, I guess this should be:

Form submission handler for form_test_form_state_values_clean_advanced_form().

Good gracious, but is that a function name!

Also, note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Status:Needs work» Needs review
StatusFileSize
new6.29 KB
PASSED: [[SimpleTest]]: [MySQL] 33,774 pass(es).
[ View ]

+++ b/includes/common.incundefined
@@ -6377,7 +6377,8 @@ function drupal_array_get_nested_value(array &$array, array $parents, &$key_exis
-      return NULL;
+      $null = NULL;
+      return $null;

That must be something related to returning by reference http://www.php.net/manual/en/language.references.return.php. I would not touch that but it would be nice to understand the reason :)

Fixed other issues, rerolled the patch.

Status:Needs review» Reviewed & tested by the community

Thanks!

Making that a reference returning function is not a bad idea but backporting needs to be careful to see whether function &x() {}; $x= x(); ends up as a reference or not. I know it shouldn't but. Um. PHP.

Edit: I came up with a simpler test http://paste.pocoo.org/show/501370/ here. It works. So we can go ahead with a straight backport.

Issue tags:-Needs tests
StatusFileSize
new5.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.form-state-values-clean.32.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

+++ b/core/includes/form.inc
@@ -2124,30 +2124,13 @@ function form_state_values_clean(&$form_state) {
-    // 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).

Decided to revert the removal of this comment, as it still applies, just done differently using the helper function.

No other changes, so leaving as RTBC.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Needs review

Committed and pushed to 8.x, thanks! Moving to 7.x.

Status:Needs review» Needs work
Issue tags:+needs backport to D7

The last submitted patch, drupal8.form-state-values-clean.32.patch, failed testing.

Status:Needs work» Patch (to be ported)

Status:Patch (to be ported)» Needs review
StatusFileSize
new5.35 KB
PASSED: [[SimpleTest]]: [MySQL] 36,915 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Looks good to me for 7.x.

#37 works for me in D7.

Solves an issue I was having with a 'cannot unset string reference' fatal error when using a managed_file field in a custom system settings form.

Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Needs work

Hm. I cannot trigger the bug cited in #9. I apply the test-only hunks of the patch, enable the form_test module, and save the form and see "You WIN!" I switch return $form; to return system_settings_form($form) and that also works properly ("Your configuration has been saved" dsm()). No fatal errors.

The test does give failures when used by itself, but they're not of the fatal variety. More about not finding the form on postback.

Further, I don't see any assertions around the fact that the form_state values were actually cleaned up, which seems to be the point of this initial bug.

So, basically, I don't think that test is right. I'm also wondering if there's a way to consolidate this with the other Form State Values Clean tests, as it doesn't really make sense to have a whole separate test case for this.

I also ran into this problem when altering the user_profile_form and adding an element of type managed_file AND setting the default value (without the default value it works fine). The patch in #37 with D7 works fine and solves the problem, although the actual problem might be somewhat deeper.

Note that in form.inc form_state_values_clean(), $last_parent has value 'upload_button' and $values has value '56', so unset($values[$last_parent]); will not work.

In any case, this change makes the code re-use existing functionality, and allows some calling code to be simplified due to the return by reference (could count as API addition).

@Martijn Houtman clarified that this patch solves a problem.

If absolutely required, we can write or revise the test. But IMHO it would be fine to let this slip through, because the change of re-using the existing general helper function is a good refactoring on its own.

Assigned:claudiu.cristea» sun
Status:Needs work» Needs review
StatusFileSize
new1.64 KB
FAILED: [[SimpleTest]]: [MySQL] 37,054 pass(es), 6 fail(s), and 6 exception(s).
[ View ]

I've added an entire stack of duplicate issues in Drupal core and contrib to the issue summary.

If @webchick is right with #40, then attached patch should pass -- it reverts the functional changes, but retains the tests.

However, the test is properly failing for me locally. I'm not sure whether @webchick might have tested on D7 instead of D8 - in which case it might be possible, since there was no test-only patch for the D7 fix in here yet.

Status:Needs review» Needs work

The last submitted patch, drupal8.form-state-values-clean-revert.43.patch, failed testing.

Version:8.x-dev» 7.x-dev
Status:Needs work» Needs review
StatusFileSize
new3.77 KB
FAILED: [[SimpleTest]]: [MySQL] 39,265 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new5.95 KB
PASSED: [[SimpleTest]]: [MySQL] 39,250 pass(es).
[ View ]

#43 confirms that D8 has been fixed properly and the test catches the bug.

Thus, moving back to D7. Apparently, the D8 commit can be cherry-picked and applies cleanly. Attaching both the full patch and a test-only patch.

I've manually tested the reported bug against Webform (#1515158: Webform with file upload fatal error/WSOD when used with mollom: Cannot unset string offsets in ... form.inc on line 2146) with this patch applied to D7 core, and it ceased to exist.

Status:Needs review» Needs work

The last submitted patch, drupal.form-state-values-clean.45.test-only.patch, failed testing.

Status:Needs work» Needs review

Expected result. So I think this is RTBC.

#45 fixed the Fatal error for me! Thanks a lot +1 RTBC

Status:Needs review» Reviewed & tested by the community

Back to RTBC then.

#45 is the same patch as mine from #37 but rerolled against the fresh code. So why was this stuck for so long?

Title:form_state_values_clean() is polluting form valuesform_state_values_clean() is polluting form values (followup)
Version:7.x-dev» 8.x-dev
Category:bug» task
Priority:Major» Normal
Status:Reviewed & tested by the community» Active
Issue tags:+7.15 release notes

This patch is making my head hurt, but I think it makes sense :) Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/4d26c30

We should leave this open to rewrite the test per @webchick's comment above (or we could file it as a separate issue and close this one). The test in its current form is definitely a bit iffy.

Per @xjm's comment, it would also be great to have code comments explaining the $null = NULL..... Although I think we have several of those in the codebase, so it probably should be a separate documentation issue.

Status:Active» Needs review

Status:Needs review» Closed (fixed)

Seems fixed in both 7.x, 8.x. Closing.

Assigned:sun» Unassigned
Status:Closed (fixed)» Active

Did anyone file the followups yet?

Issue summary:View changes

Updated issue summary.