Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
- system_theme_settings_submit() unsets submitted form values instead of cleaning them up properly.
Proposed resolution
- Use
form_state_values_clean()
to clean the submitted values.
Original report by sun
Extracted from #1376166: Custom logo and favicon functionality inanely tries to support absolute local file paths
Comment | File | Size | Author |
---|---|---|---|
#27 | variable-unserialize-error-1284364-26.patch | 1.44 KB | kbasarab |
#26 | 1397882-system_theme_settings_submit-26.patch | 1.44 KB | pingers |
#24 | 1397882-system_theme_settings_submit-24.patch | 1.44 KB | kbasarab |
#21 | 1397882-system_theme_settings_submit-21.patch | 1.22 KB | kbasarab |
#16 | system_theme_settings_submit.patch | 947 bytes | catch |
Comments
Comment #1
xjmThis cleanup appears correct and straightforward.
Comment #2
catchCommitted/pushed to 8.x. Thanks.
Comment #3
sunComment #4
xjmYep, can't think of a reason not to backport it, either.
Comment #4.0
xjmUpdated issue summary.
Comment #5
webchickYeah, this seems like it would be pretty harmless.
It's weird to commit a bug fix without a test, but I guess the existing tests would cover that the theme settings form is working, and all we're really doing here is cleaning up the existing behaviour.
Committed and pushed to 7.x. Thanks!
Comment #6
sunApparently, we want to do a quick follow-up fix/adjustment here: The 'var' value should not be removed from $form_state['values'].
Comment #7
webchickThat last comment/patch was fairly obtuse, and none of us in #drupal-contribute could figure out what it meant. I think it means "+Needs tests" ;)
To be on the safe side, I've rolled this back from D7 since we're rolling a release in about an hour. Happy to re-commit it later.
Comment #8
sunThe follow-up patch does not remove the 'var' from the submitted form values, but only from the local copy of the form values being processed in this submission handler. Thus, 'var' remains as submitted form value for other modules.
Pointless to test.
Comment #9
catchThe follow-up needs a comment, and someone else to mark it RTBC ;)
Comment #10
jrreid CreditAttribution: jrreid commentedMakes sense to me and I see no issues either.
Comment #11
webchickSince it doesn't make sense to either of us, I think we still need that comment. :)
Comment #12
sunHrm.
If you look at the new code, it's actually very clear what is happening, and why. Translating that into proper English requires a comment of 4 lines to be technically correct.
So here is that addition. I'd still prefer to commit #6.
Comment #13
cweagansFixing tags per http://drupal.org/node/1517250
Comment #14
sun#12: drupal8.system-theme-settings-submit.12.patch queued for re-testing.
Comment #15
sunPlease choose either #6 or #12. :)
I still don't think that the extra comment in #12 is needed, so I'd prefer to go with #6.
Comment #16
catchNo that still looks awkward, what's wrong with this?
Comment #17
sunFine with me :)
Comment #18
catch#16: system_theme_settings_submit.patch queued for re-testing.
Comment #19
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #20
sunThanks!
Comment #21
kbasarab CreditAttribution: kbasarab commentedD7 backport attached.
Comment #22
kbasarab CreditAttribution: kbasarab commentedComment #23
sun#21 seems to be direct backport of #0, but we additionally need to merge in the follow-up patch from #16. Can you do that? :)
Comment #24
kbasarab CreditAttribution: kbasarab commentedOops. Thought I had done that. Here it is.
Comment #25
sunStray $ - unset() is function :)
Comment #26
pingers CreditAttribution: pingers commentedRe-roll without "$".
Comment #27
kbasarab CreditAttribution: kbasarab commentedThis is what I get for just making a quick switch. Updated now.
EDIT: Posted at same time as pingers. Thanks for the fix pingers.
Comment #28
sunExcellent! Thanks all :)
Comment #29
pingers CreditAttribution: pingers commented@kbasarab - hehe, no problem ;)
Comment #30
David_Rothstein CreditAttribution: David_Rothstein commentedI think this is OK for Drupal 7. In theory it could affect another submit handler which runs after this one, but the use case for checking those particular items in $form_state['values'] in a submit handler is probably pretty slim. (It would not be hard to change the patch to protect against that if we wanted, though.)
I committed the patch to Drupal 7, but because of the above issue I added it to CHANGELOG.txt also: http://drupalcode.org/project/drupal.git/commit/a6cc373
I also corrected the misspelling of "unnecessary" on commit (the Drupal 8 patch already had it right) :)
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commentedDrupal 7.16 was a security release only, so this issue is now scheduled for Drupal 7.17 instead.
Fixing tags accordingly.
Comment #32.0
David_Rothstein CreditAttribution: David_Rothstein commentedUpdated issue summary.