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
- #1090198: managed_file form item in system settings form causes error in form_state_values_clean()
- Webform: #1515158: Webform with file upload fatal error/WSOD when used with mollom: Cannot unset string offsets in ... form.inc on line 2146
- Mollom: #1617178: WSOD using mollom on a webform with a file upload
Comment | File | Size | Author |
---|---|---|---|
#45 | drupal.form-state-values-clean.45.patch | 5.95 KB | sun |
#45 | drupal.form-state-values-clean.45.test-only.patch | 3.77 KB | sun |
#43 | drupal8.form-state-values-clean-revert.43.patch | 1.64 KB | sun |
#37 | form-state-values-clean-635046-37.patch | 5.35 KB | claudiu.cristea |
#32 | drupal8.form-state-values-clean.32.patch | 5.43 KB | sun |
Comments
Comment #1
fagoComment #2
moshe weitzman CreditAttribution: moshe weitzman commentedlooks right to me. could use another fapi knowledgeable opinion.
Comment #3
MichaelCole CreditAttribution: MichaelCole commentedform_pollution.patch queued for re-testing.
Comment #5
sunShouldn't we simply re-use existing helpers?
Comment #7
claudiu.cristea#5: drupal.form-state-values-clean-clean.5.patch queued for re-testing.
Comment #9
claudiu.cristea#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:
will crash with:
without the patch from #5.
Comment #10
marcingy CreditAttribution: marcingy commentedThe patch in 5 is not in git format, moving back to normal as through numerous triages this has never been promoted in priority before.
Comment #11
sunAttached patch should do it. (and works based on my own testing)
Comment #13
claudiu.cristeaIn #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.
Comment #14
claudiu.cristeaChanged that patch to be
$ git format-patch
like.Comment #15
marcingy CreditAttribution: marcingy commentedThe 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'
Comment #16
marcingy CreditAttribution: marcingy commentedCross post on status
Comment #17
sunHm. Didn't expect those failures. Admittedly, I didn't run that dedicated test.
Ah, I get it now.
Comment #18
sunComment #19
claudiu.cristeaApplied #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.
Comment #20
claudiu.cristeaIn some conditions (eg. system settings form in #9)
$values
obtained here (from #17 ):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 into be sure that
unset()
will not fail with that horrible error. This is fixing also cases like #9.Attached a patch.
Comment #21
catchIt looks like the example in #9 could go into an automated tests somewhere.
Comment #22
claudiu.cristeaThis patch is containing only the new test
Form API > Form state values clearance (advanced)
(classFormStateValuesCleanAdvancedTestCase extends DrupalWebTestCase
).Run the test to see how posting the form crashes in
form_state_values_clean()
. Then comment outform_state_values_clean($form_state);
fromform_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.
Comment #24
claudiu.cristeaThe patch from #22 failed in his own test, which is expected. Moving back to "needs review".
Comment #25
catchLooks good to me, can you post the combined patch with the fix?
Comment #26
claudiu.cristeaSure.
Comment #27
claudiu.cristeaComment #28
xjmThanks @claudiu.cristea for adding the tests! They look pretty good to me. Here are a few code style cleanup pointers:
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?
Should begin "Tests" rather than "Test."
This text should probably be:
See: http://drupal.org/node/1354#forms
Similarly, I guess this should be:
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.
Comment #29
claudiu.cristeaThat 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.
Comment #30
sunThanks!
Comment #31
chx CreditAttribution: chx commentedMaking 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.
Comment #32
sunDecided 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.
Comment #33
catchCommitted and pushed to 8.x, thanks! Moving to 7.x.
Comment #34
catch#32: drupal8.form-state-values-clean.32.patch queued for re-testing.
Comment #36
catchComment #37
claudiu.cristeaComment #38
catchLooks good to me for 7.x.
Comment #39
willvincent CreditAttribution: willvincent commented#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.
Comment #40
webchickHm. 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.
Comment #41
Martijn Houtman CreditAttribution: Martijn Houtman commentedI 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.
Comment #42
sunIn 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.
Comment #43
sunI'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.
Comment #45
sun#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.
Comment #46
sunI'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.
Comment #48
sunExpected result. So I think this is RTBC.
Comment #49
acbramley CreditAttribution: acbramley commented#45 fixed the Fatal error for me! Thanks a lot +1 RTBC
Comment #50
sunBack to RTBC then.
Comment #51
claudiu.cristea#45 is the same patch as mine from #37 but rerolled against the fresh code. So why was this stuck for so long?
Comment #52
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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.Comment #53
k4b0um CreditAttribution: k4b0um commented#17: drupal8.form-state-values-clean.14.patch queued for re-testing.
Comment #54
claudiu.cristeaSeems fixed in both 7.x, 8.x. Closing.
Comment #55
David_Rothstein CreditAttribution: David_Rothstein commentedDid anyone file the followups yet?
Comment #55.0
David_Rothstein CreditAttribution: David_Rothstein commentedUpdated issue summary.
Comment #56
Crell CreditAttribution: Crell commented