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

Files: 
CommentFileSizeAuthor
#27 variable-unserialize-error-1284364-26.patch1.44 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 39,395 pass(es).
[ View ]
#26 1397882-system_theme_settings_submit-26.patch1.44 KBpingers
PASSED: [[SimpleTest]]: [MySQL] 39,415 pass(es).
[ View ]
#24 1397882-system_theme_settings_submit-24.patch1.44 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] 39,367 pass(es), 24 fail(s), and 34 exception(s).
[ View ]
#21 1397882-system_theme_settings_submit-21.patch1.22 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 39,411 pass(es).
[ View ]
#16 system_theme_settings_submit.patch947 bytescatch
PASSED: [[SimpleTest]]: [MySQL] 39,889 pass(es).
[ View ]
#12 drupal8.system-theme-settings-submit.12.patch1.02 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,262 pass(es).
[ View ]
#6 drupal8.system-theme-settings-submit.6.patch765 bytessun
PASSED: [[SimpleTest]]: [MySQL] 34,163 pass(es).
[ View ]
#3 drupal.system-settings-form-submit.3.patch1.62 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,284 pass(es).
[ View ]
drupal8.system-theme-settings-submit.0.patch1.24 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,587 pass(es).
[ View ]

Comments

xjm’s picture

Status:Needs review» Reviewed & tested by the community

This cleanup appears correct and straightforward.

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x. Thanks.

sun’s picture

Version:8.x-dev» 7.x-dev
Status:Fixed» Needs review
StatusFileSize
new1.62 KB
PASSED: [[SimpleTest]]: [MySQL] 37,284 pass(es).
[ View ]
xjm’s picture

Status:Needs review» Reviewed & tested by the community

Yep, can't think of a reason not to backport it, either.

xjm’s picture

Issue summary:View changes

Updated issue summary.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Yeah, 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!

sun’s picture

Version:7.x-dev» 8.x-dev
Status:Fixed» Needs review
StatusFileSize
new765 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,163 pass(es).
[ View ]

Apparently, we want to do a quick follow-up fix/adjustment here: The 'var' value should not be removed from $form_state['values'].

webchick’s picture

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

sun’s picture

Status:Needs review» Reviewed & tested by the community
+++ b/core/modules/system/system.admin.inc
@@ -668,10 +668,10 @@ function _system_theme_settings_validate_path($path) {
-  unset($form_state['values']['var']);
...
+  unset($values['var']);

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

catch’s picture

Status:Reviewed & tested by the community» Needs work

The follow-up needs a comment, and someone else to mark it RTBC ;)

jrreid’s picture

Status:Needs work» Reviewed & tested by the community

Makes sense to me and I see no issues either.

webchick’s picture

Status:Reviewed & tested by the community» Needs work

Since it doesn't make sense to either of us, I think we still need that comment. :)

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new1.02 KB
PASSED: [[SimpleTest]]: [MySQL] 37,262 pass(es).
[ View ]

Hrm.

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.

cweagans’s picture

sun’s picture

sun’s picture

Status:Needs review» Reviewed & tested by the community

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

catch’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new947 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,889 pass(es).
[ View ]

No that still looks awkward, what's wrong with this?

sun’s picture

Status:Needs review» Reviewed & tested by the community

Fine with me :)

catch’s picture

#16: system_theme_settings_submit.patch queued for re-testing.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks!

sun’s picture

Version:8.x-dev» 7.x-dev
Status:Fixed» Patch (to be ported)

Thanks!

kbasarab’s picture

StatusFileSize
new1.22 KB
PASSED: [[SimpleTest]]: [MySQL] 39,411 pass(es).
[ View ]

D7 backport attached.

kbasarab’s picture

Status:Patch (to be ported)» Needs review
sun’s picture

Assigned:sun» Unassigned
Status:Needs review» Needs work

#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? :)

kbasarab’s picture

Status:Needs work» Needs review
StatusFileSize
new1.44 KB
FAILED: [[SimpleTest]]: [MySQL] 39,367 pass(es), 24 fail(s), and 34 exception(s).
[ View ]

Oops. Thought I had done that. Here it is.

sun’s picture

Status:Needs review» Needs work
+++ b/modules/system/system.admin.inc
@@ -691,8 +691,16 @@ function _system_theme_settings_validate_path($path) {
+  $unset($values['var']);

Stray $ - unset() is function :)

pingers’s picture

Status:Needs work» Needs review
StatusFileSize
new1.44 KB
PASSED: [[SimpleTest]]: [MySQL] 39,415 pass(es).
[ View ]

Re-roll without "$".

kbasarab’s picture

StatusFileSize
new1.44 KB
PASSED: [[SimpleTest]]: [MySQL] 39,395 pass(es).
[ View ]

This is what I get for just making a quick switch. Updated now.

EDIT: Posted at same time as pingers. Thanks for the fix pingers.

sun’s picture

Status:Needs review» Reviewed & tested by the community

Excellent! Thanks all :)

pingers’s picture

@kbasarab - hehe, no problem ;)

David_Rothstein’s picture

Status:Reviewed & tested by the community» Fixed

I 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) :)

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

Issue tags:+7.17 release notes

Drupal 7.16 was a security release only, so this issue is now scheduled for Drupal 7.17 instead.

Fixing tags accordingly.

David_Rothstein’s picture

Issue summary:View changes

Updated issue summary.