If a theme defines the "features" that it supports (via setting features[] in its .info file), it might choose not to support one of the default core features.

Currently, the checkboxes for those features will not appear at admin/appearance/settings/THEMENAME, but if you call theme_get_setting(), you will still get back a result that says the feature is toggled on, which is incorrect.

The attached patch seems to fix it, although this part of the code is a bit tricky so I'm not 100% sure I got it right (especially with regard to subthemes, etc). If it seems like the right fix, we could also write a test for it.

CommentFileSizeAuthor
#8 disabled-theme-feature-setting-629902-8.patch3.19 KBjohnalbin
#5 629902-theme-setting-features-d7.patch2.61 KBandypost
fix-theme-setting-features.patch2.64 KBDavid_Rothstein

Comments

jacine’s picture

jacine
she/her
Location New York
commented

fix-theme-setting-features.patch queued for re-testing.

jacine’s picture

jacine
she/her
Location New York
commented

I'm running into this bug now. This patch needs updating, but I tested and it works. However, I'm not sure about subtheme implications either.

+++ modules/system/system.module	11 Nov 2009 16:30:52 -0000
@@ -2210,18 +2210,7 @@ function _system_rebuild_theme_data() {
-        'search',

The search feature has been removed since this patch was created.

Powered by Dreditor.

jacine’s picture

jacine
she/her
Location New York
commented
Status: Needs review » Needs work
jacine’s picture

jacine
she/her
Location New York
commented
Priority: Normal » Critical

We can't ship D7 with broken theme settings. Marking this critical.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 629902-theme-setting-features-d7.patch2.61 KB

Quick re-roll, list of default features was changed (now there's no search)

dries’s picture

dries commented

Mmm, the attached patch looks good but it also suggests that adding the features[] array to the .info file might not have been the best plan. This is a bit of a WTF.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

This patch needs approval of JohnAlbin. I'll try to ping him via IRC.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
StatusFileSize
new disabled-theme-feature-setting-629902-8.patch3.19 KB

Nice catch, David!

I cleaned it up a bit since there's some existing variables we could have used, but its basically the same patch.

I also removed two dummy settings that weren't used anywhere; 'toggle_main_menu' and 'toggle_secondary_menu' are used, but 'main_menu' and 'secondary_menu' are bogus settings and likely just copy and paste errors made when rolling the patch in #160168: Let Primary and Secondary links be arbitrary menus as in the Drupal 5 menu system.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

+1 to commit

jacine’s picture

jacine
she/her
Location New York
commented

Just tested and it works. Yay! Thanks guys. :D

+ 1 for commit

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)

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