Theme settings forms are not inherited by sub-themes
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | theme system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | JohnAlbin |
| Status: | needs review |
I've been remiss in not reporting this sooner.
If foo theme defines some theme settings, those settings will be visible on the page admin/build/themes/settings/foo with the following text, “These settings only exist for the Foo theme and all the styles based on it.” (The words the styles should be changed to sub-themes, btw.)
But if a subfoo is a sub-theme of foo and it defines a couple extra theme settings of its own, the subfoo theme does not see any of its parent's theme settings when visiting admin/build/themes/settings/subfoo
The form altering logic in system_theme_settings(), which I wrote just after code freeze in July 2007, does not deal with the full level of inheritance that the rest of the theme system gives themers. It only shows the current theme's settings or, if the current theme doesn't have any settings, it shows the parent theme's settings. And it doesn't deal with multi-level inheritance.
A sub-theme's theme settings page (admin/build/themes/settings/SUBTHEME) should have all of the theme settings forms from its parent themes.
#1
John,
As the use of custom theme settings and sub-themes has increased, this bug has become critical. I'm strongly in favor of getting this fixed as soon as possible.
In order to try to help things along, I've taken a shot at a patch to fix the problem.
I've tested it against 6.12, but rolled it against HEAD. The only difference is due to Damien Tournoud's patch (http://drupal.org/node/445966), which passes a $form variable into the THEMENAME_settings() function, in addition to $settings.
Since this issue is targeted at 7.x-dev, I guess that's where it should go first, once it's RTBC. But I'm very hopeful that it can also be backported to the D6 branch as well, by simply removing "$form" from the "$group = $function($settings, $form)" statements.
The patch is intended to support full inheritance, so that a sub-theme will display theme settings from both itself and its base theme, if available. I wasn't sure about the effect of theme settings from the template engine (that one was new to me), so I just left it at the top of the flow. Inheritance should go: theme engine -> base theme -> sub-theme.
Thanks for highlighting this issue, and I hope this helps.
#2
The last submitted patch failed testing.
#3
Setting to 'needs review' - testbot was broken.
#4
@sociotech: Thanks for attempting a patch, but your patch doesn’t traverse the entire base theme ancestry. Base themes can have base themes, so we have to pull in all of those themes settings.
Traversing this ancestry will be simplied after #489762: Add theme lineage to .info cache (and prevent WSOD on admin/build/themes) lands.
#5
John,
You're correct, my patch only goes one level up as far as base themes.
But given that this is a D6 bug what do you think the chances are that this or something like it could this be considered as a partial (interim) solution in D6 until #489762 gets into D7?
Also, when I use the recommended practice of prefixing theme functions with the theme name, rather than phptemplate, I encounter a problem with THEMENAME_settings() in theme-settings.php. The behavior I'm seeing is that a sub-theme is not inheriting the parent's theme settings _at all_, even if the sub-theme does not include a theme-settings.php file.
Looking at the current theme setting code in system.admin.inc in the same area of the patch, I see where the base theme-settings.php file gets included, but i don't see that its THEMENAME_settings() function is called in order to include its theme settings in the sub-theme's theme_specific array. Instead, it looks like the template engine's function (e.g., phptemplate_settings()) is used when the sub-theme doesn't have its own THEMENAME_settings() function.
So another question would be whether or not the recommended practice is to name the function phptemplate_settings() in theme-settings.php. Sub-themes cannot inherit from the base theme at all unless it is named phptemplate_settings(). My previously submitted patch does seem to take care of this issue.
But barring this getting fixed in D6, I'm also open to suggestions. I know that Zen uses its own theme setting system, but I'd like to try to get things working with Drupal's default system if at all possible.
Thanks!
#6
#489762: Add theme lineage to .info cache (and prevent WSOD on admin/build/themes) has been committed.
#7
This issue will be fixed by #563708: Improve theme_get_setting() and make custom theme settings a true form_alter, but only if I get help with that one between now and code freeze! Help, please! :-)
#8
John, correct me if I'm wrong, but it looks like #563708 will only fix the issue of inheriting theme settings for D7. It seems like it's a new feature and thus not likely to be backported to D6.
Given that, wouldn't it still make sense to pursue a fix for theme setting inheritance in D6 based on #489762?
#9
@sociotech
Agreed. As soon as #563708: Improve theme_get_setting() and make custom theme settings a true form_alter is committed, let's see if we can use this issue to get a smaller-scope bugfix into 6.x.
#10
#563708: Improve theme_get_setting() and make custom theme settings a true form_alter has been committed. woo-hoo!
#11
.
#12
John,
Based on the successful commit of #489762: Add theme lineage to .info cache (and prevent WSOD on admin/build/themes) (wtg!), I've re-done my original patch to take advantage of the available base theme list in the .info cache. So it should now inherit the theme settings from all the base themes that a theme is based on.
I tested it with a base theme, a sub-theme, and a sub-sub-theme, and it appears to correctly inherit and display the theme settings from all the sub-theme's parents on its theme setting page. I haven't tried it with more than two parent themes yet, however.
EDIT: Do not use this patch! Incorrectly created. Use the v3 version below.
#13
Sorry, ignore the previous patch as it was incorrectly rolled.
Attached is a correct patch that should apply properly against Drupal 6.14.
#14
duplicate post
#15
It would be great if interested folks could review the patch in #13 in order to be able to make any changes or get this on to RTBC. Thanks!