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

$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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Status: Active » Needs review
moshe weitzman’s picture

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

MichaelCole’s picture

form_pollution.patch queued for re-testing.

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
1.89 KB

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.

claudiu.cristea’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

claudiu.cristea’s picture

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:

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.

marcingy’s picture

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.

sun’s picture

Version: 7.x-dev » 8.x-dev
Assigned: Unassigned » sun
Priority: Normal » Major
Status: Needs work » Needs review
Issue tags: +Needs backport to D7
FileSize
1.59 KB

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.

claudiu.cristea’s picture

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.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
2.09 KB

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

marcingy’s picture

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'

marcingy’s picture

Status: Needs work » Needs review

Cross post on status

sun’s picture

Status: Needs review » Needs work
FileSize
2.42 KB

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

Ah, I get it now.

sun’s picture

Status: Needs work » Needs review
claudiu.cristea’s picture

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.

claudiu.cristea’s picture

FileSize
2.44 KB

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

$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

    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.

catch’s picture

Issue tags: +Needs tests

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

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
3.94 KB

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.

claudiu.cristea’s picture

Status: Needs work » Needs review

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

catch’s picture

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

claudiu.cristea’s picture

Sure.

claudiu.cristea’s picture

Assigned: sun » claudiu.cristea
xjm’s picture

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.

claudiu.cristea’s picture

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

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

chx’s picture

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.

sun’s picture

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

catch’s picture

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.

catch’s picture

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

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

catch’s picture

Status: Needs work » Patch (to be ported)
claudiu.cristea’s picture

Status: Patch (to be ported) » Needs review
FileSize
5.35 KB
catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me for 7.x.

willvincent’s picture

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

webchick’s picture

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.

Martijn Houtman’s picture

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.

sun’s picture

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.

sun’s picture

Assigned: claudiu.cristea » sun
Status: Needs work » Needs review
FileSize
1.64 KB

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.

sun’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
3.77 KB
5.95 KB

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

sun’s picture

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.

sun’s picture

Status: Needs work » Needs review

Expected result. So I think this is RTBC.

acbramley’s picture

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

sun’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC then.

claudiu.cristea’s picture

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

David_Rothstein’s picture

Title: form_state_values_clean() is polluting form values » form_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.

k4b0um’s picture

Status: Active » Needs review
claudiu.cristea’s picture

Status: Needs review » Closed (fixed)

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

David_Rothstein’s picture

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

Did anyone file the followups yet?

David_Rothstein’s picture

Issue summary: View changes

Updated issue summary.

Crell’s picture

Issue summary: View changes
Status: Active » Closed (fixed)