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.)
Comment | File | Size | Author |
---|---|---|---|
#18 | drupal.form-state-values-clean.17.patch | 10.58 KB | sun |
#15 | drupal.form-state-values-clean.15.patch | 9.73 KB | sun |
#14 | drupal.form-state-values-clean.14.patch | 9.71 KB | sun |
#12 | drupal.form-state-values-clean.8.patch | 11.13 KB | c960657 |
#10 | drupal.form-state-values-clean.7.patch | 11.32 KB | c960657 |
Comments
Comment #1
chx CreditAttribution: chx commentedWhat about form_state_values_unset?
Comment #2
sunWhat about form_state_clean_values() ?
Comment #3
sunPutting the verb last. So, form_state_values_clean() it is. :)
Comment #4
sunAdded 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.
Comment #5
c960657 CreditAttribution: c960657 commentedHow about simply removing all values generated by #type => 'submit' ?
Comment #6
sun@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. ;)
Comment #8
chx CreditAttribution: chx commentedIf you want to do that then why dont you keep a
$form_state['button_array_parents']
or somesuch with stuff likeand then
Comment #9
chx CreditAttribution: chx commentedSorry i got confused of what we want to unset. Disregard that post. Here is a better version then:
Comment #10
c960657 CreditAttribution: c960657 commentedUpdated patch based on chx's suggestion.
Comment #11
sunSince 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?
Comment #12
c960657 CreditAttribution: c960657 commentedFixed :)
Comment #13
sunRock solid.
Comment #14
sunI 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.
Comment #15
sunRevised those docs with help from chx.
Comment #16
sunTagging 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.
Comment #17
webchickSomeone needs to test that image buttons still work in IE after this patch.
First line should be <= 80 chars.
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.
Curious. Is there ever a chance of a contributed module adding additional "internal" properties? Are we ok hard-coding this list?
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."
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.
Comment #18
sunIncorporated all requested changes.
Not needed, because the "funky IE button scenario" is handled right before that removed hunk:
_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. :)
Comment #19
joachim CreditAttribution: joachim commented+ * proceeds to storage. Next to button elements, the following internal values
Do you mean 'As well as' rather than 'Next'?
Comment #20
Dries CreditAttribution: Dries commentedCommitted this patch to CVS HEAD. Thanks!