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.
Comment | File | Size | Author |
---|---|---|---|
#26 | system_admin_inherit_theme_settings-481142-26.patch | 3.16 KB | JohnAlbin |
#12 | system_admin_inherit_theme_settings_v2.patch | 4.17 KB | sociotech |
#13 | system_admin_inherit_theme_settings_v3.patch | 4.18 KB | sociotech |
#1 | system_admin_inherit_theme_settings.patch | 2.32 KB | sociotech |
Comments
Comment #1
sociotech CreditAttribution: sociotech commentedJohn,
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.
Comment #3
lilou CreditAttribution: lilou commentedSetting to 'needs review' - testbot was broken.
Comment #4
JohnAlbin@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.
Comment #5
sociotech CreditAttribution: sociotech commentedJohn,
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!
Comment #6
JohnAlbin#489762: Add theme lineage to .info cache (and prevent WSOD on admin/build/themes) has been committed.
Comment #7
JohnAlbinThis 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! :-)
Comment #8
sociotech CreditAttribution: sociotech commentedJohn, 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?
Comment #9
JohnAlbin@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.
Comment #10
JohnAlbin#563708: Improve theme_get_setting() and make custom theme settings a true form_alter has been committed. woo-hoo!
Comment #11
mattyoung CreditAttribution: mattyoung commented.
Comment #12
sociotech CreditAttribution: sociotech commentedJohn,
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.
Comment #13
sociotech CreditAttribution: sociotech commentedSorry, ignore the previous patch as it was incorrectly rolled.
Attached is a correct patch that should apply properly against Drupal 6.14.
Comment #14
sociotech CreditAttribution: sociotech commentedduplicate post
Comment #15
sociotech CreditAttribution: sociotech commentedIt 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!
Comment #16
flndr CreditAttribution: flndr commentedI just applied this patch to my install. It appears to work as expected (mostly, below probably is another issue).
I got this warning, and the blocks I created before (admin/build/block/add, ones provided by views were fine) were missing titles:
Other than that, looks great.
Comment #17
sociotech CreditAttribution: sociotech commentedjustinzero,
Thanks for checking it out the patch.
Regarding your other issue, if you're using a Fusion-based theme, or another theme that uses the same technique that we use to disable inherited Skinr styles, then this patch from the Skinr issue queue will fix it for you:
http://drupal.org/node/648816
Comment #18
flndr CreditAttribution: flndr commentedThanks for the tip.
I applied this patch to another site using 6.15 (subtheme of Austin, which is a subtheme of Zen). Works as expected.
Comment #19
psynaptic CreditAttribution: psynaptic commentedIs the current workaround to duplicate theme settings in the sub-theme?
Comment #20
sociotech CreditAttribution: sociotech commentedpsynaptic,
Yes, duplicating the theme settings in the sub-theme is the only workaround at this point.
I'm getting the feeling that since this bug is fixed in D7 there may not be much motivation to fix it in D6.
Nevertheless, if anyone is still interested in helping to move this patch forward with testing, I can re-roll it against 6.15.
Comment #21
psynaptic CreditAttribution: psynaptic commentedThanks sociotech :)
Comment #22
aitala CreditAttribution: aitala commentedI would like to see it rolled into D6 - I am not sure I am capable to testing out the patch, but would give it a shot.
Eric
Comment #23
sociotech CreditAttribution: sociotech commentedTurns out there were no changes in the file being patched (system.admin.inc) from 6.14 to 6.15, so the version 3 patch in #13 still applies cleanly to 6.15 for testing purposes.
To test, start with a sub-theme based on a base theme (e.g., Acquia Prosper, which is based on Fusion), and then create a sub-sub-theme based on the sub-theme and verify that the advanced theme settings are inherited and function correctly.
Thanks in advance for feedback.
Comment #24
vrajak@gmail.com CreditAttribution: vrajak@gmail.com commentedWhen trying to use the patch through terminal, I get the following:
I'm using a subtheme of Acquia Prosper, with Fusion of course. Didn't seem to work for me, as I checked the theme settings and not all of them are there. This is also the first time I've tried applying a patch, so maybe I'm doing something wrong.
Comment #25
vrajak@gmail.com CreditAttribution: vrajak@gmail.com commentedComment #26
JohnAlbin@sociotech: Man, I’m sorry I never got around to reviewing your patch! My apologies!!! :-)
However, reviewing the patch in #13, I see some issues:
I rolled a new patch that more closely matches the code in D7, while still retaining the same API flaws in D6. :-p
Comment #27
sociotech CreditAttribution: sociotech commentedJohn,
No problem, I know you've been working hard on D7 stuff.
Thanks for re-rolling the patch with fixes!
I'll test it locally, and hopefully others will as well so we can get this in soon.
Comment #28
aitala CreditAttribution: aitala commentedI gather this patch did not make it into D6.16? I just have not had time to do any testing...
Eric
Comment #29
klonosThis should be in the next D6.17! Subscribing...
Comment #30
ultimikeJohn,
What is the status of this patch? It doesn't look like it's been tested by the automated testing server?
Thanks,
-mike
Comment #31
ChrisRut CreditAttribution: ChrisRut commentedsubscribing
Comment #32
lolmaus CreditAttribution: lolmaus commentedSubscribing
Comment #33
ayalon CreditAttribution: ayalon commentedShould be in the next Drupal release! Subtheming of popular Drupal Themes is not possible due to this bug.
Comment #34
ayalon CreditAttribution: ayalon commentedI applied the patch in #26 and tested the following usecase:
I'm using a custom theme that is a sub theme based on Aquia Prosper (based on Fusion).
On the Theme Setting Page I get the following errors:
* warning: array_merge() [function.array-merge]: Argument #2 is not an array in \sites\all\themes\fusion\fusion_core\theme-settings.php on line 95.
* warning: array_merge() [function.array-merge]: Argument #1 is not an array in \sites\all\themes\fusion\fusion_core\theme-settings.php on line 98.
* warning: Invalid argument supplied for foreach() in \sites\all\themes\fusion\fusion_core\theme-settings.php on line 129.
* warning: array_merge() [function.array-merge]: Argument #2 is not an array in \sites\all\themes\fusion\fusion_core\theme-settings.php on line 95.
* warning: array_merge() [function.array-merge]: Argument #1 is not an array in \sites\all\themes\fusion\fusion_core\theme-settings.php on line 98.
* warning: Invalid argument supplied for foreach() in \sites\all\themes\fusion\fusion_core\theme-settings.php on line 129.
* warning: array_merge() [function.array-merge]: Argument #2 is not an array in \sites\all\themes\fusion\fusion_core\theme-settings.php on line 95.
* warning: array_merge() [function.array-merge]: Argument #1 is not an array in \sites\all\themes\fusion\fusion_core\theme-settings.php on line 98.
* warning: Invalid argument supplied for foreach() in \sites\all\themes\fusion\fusion_core\theme-settings.php on line 129.
It seems, that the patch does not resolve my issues. :-/
Comment #35
ayalon CreditAttribution: ayalon commentedIf I apply the patch under #13 i get the following errors with the same thest scenario.
* warning: array_merge() [function.array-merge]: Argument #2 is not an array in \sites\all\themes\fusion\fusion_core\theme-settings.php on line 95.
* warning: array_merge() [function.array-merge]: Argument #1 is not an array in \sites\all\themes\fusion\fusion_core\theme-settings.php on line 98.
* warning: Invalid argument supplied for foreach() in \sites\all\themes\fusion\fusion_core\theme-settings.php on line 129.
Comment #36
frank0987 CreditAttribution: frank0987 commented#1: system_admin_inherit_theme_settings.patch queued for re-testing.
Comment #37
pimok3000 CreditAttribution: pimok3000 commentedsubscribing
Comment #38
sociotech CreditAttribution: sociotech commentedayalon,
That sounds more like an issue with Fusion than with this patch.
I added some error checking for situations in which a theme has no theme settings in its .info file to theme-settings.php in the most recent dev release (#721386: array_merge() error in fusion_core/theme-settings.php). That should prevent these errors.
If you still get errors after trying the dev release, let me know here or in the Fusion issue queue.
Comment #39
jensimmons CreditAttribution: jensimmons commentedsubscribe
Comment #40
adrinux CreditAttribution: adrinux commentedHere's a workaround of sorts stolen from Tendu's subtheme. Add a theme-settings.php to your subtheme with code calling the parent's theme settings directly, instead of just duplicating the whole theme settings. Replace MYBASETHEME with the base theme you are using and MYSUBTHEME with the name of your sub theme:
Comment #41
mshepherd CreditAttribution: mshepherd commented#40 also works in this use case:
To inherit theme settings from a subtheme.
I have theme A which is a subtheme of fusion_core. I also have theme B which is a subtheme of A.
In the sub-sub-theme (B in this example), specify fusion_core for MYBASETHEME and B for MYSUBTHEME
Comment #42
JohnAlbinAutomated testing only works for D7 patches, not for D6 patches. For D6, "needs review" means someone besides the patch writer needs to apply it and review it! :-D
After applying this patch, you'll likely need to clear the cache; I've seen "argument is not an array" type errors when the cache needed to be cleared.
Comment #43
myregistration CreditAttribution: myregistration commentedSorry for my ignorance, I'm new. How do we include the theme-settings.php script per #40? Thanks!
Comment #44
mshepherd CreditAttribution: mshepherd commentedcreate a theme-settings.php file in your sub theme's directory. add the code from #40 changing the text as the poster suggests (to make it specific to your sub theme and parent theme)
Comment #45
dafederI tried #40 and got Fatal error: Call to undefined function phptemplate_settings() in /home/XXX/public_html/sites/all/themes/XXX/theme-settings.php on line 6
Comment #46
abaddon CreditAttribution: abaddon commented@#45 - try to rename phptemplate_settings to BASE_THEME_NAME_settings , it should work.. the base theme implemented the hook with a different name
Comment #47
rajivnarayana CreditAttribution: rajivnarayana commentedIt looks fixed for me. I used a subtheme of AcquiaMarina which is a subtheme of fusioncore. I am using 6.17 drupal and my subtheme has the following files only, and no more
.info
style.css which is empty
template.php which is empty
Comment #48
Jeff Burnz CreditAttribution: Jeff Burnz commentedsub
Comment #49
YesCT CreditAttribution: YesCT commentedI took a look at it, coding style looks ok to me.
Powered by Dreditor.
and #47 seems like a functional test was done.
Comment #50
Jeff Burnz CreditAttribution: Jeff Burnz commented@49, nope, please see the original issue:
thats the critical bit in bold, Fusion sub-themes do not have any settings by default - I mean the starterkit package that ships with Fusion doesnt attempt to do this. Fusion also includes a lot of trickery itself with regards to settings, it would be better tested on a very simple theme, with a few settings added.
Comment #51
Edgar Saumell CreditAttribution: Edgar Saumell commented#40 worked for me on a custom sub-theme of acquia_prosper which is a sub-theme of fusion_core
Comment #52
abaddon CreditAttribution: abaddon commentedi think someone should make this clear
#40 - is a workaround, it works, it wont ever get submitted to fix this because its theme specific, i dont think anyone cares if it works because it does, only post if it doesnt work and you need help with it
#26 - is the real patch that needs to be tested, its a patch to drupal core to fix the theming issue on hand here.. if you want this issue to get fixed, test #26
Comment #53
discipolo CreditAttribution: discipolo commentedTested against D6.19 without errors. seems to do the trick!
Comment #54
webkenny CreditAttribution: webkenny commentedAmazing as this might seem for a patch open over a year, I can confirm that this works against 6.19!! Nice job.
Comment #55
paskainos CreditAttribution: paskainos commentedsubscribing
Comment #56
Drupaliens CreditAttribution: Drupaliens commentedsubscribing
It as been commented on Durpal Commons website
http://commons.acquia.com/discussion/more-themes-drupal-commons#comment-...
Comment #57
servantleader CreditAttribution: servantleader commented+1
Comment #58
mariomaric CreditAttribution: mariomaric commentedSubscribing...
Comment #59
Gábor HojtsyOk, my understanding from reading this issue is that this was "fixed" in D7 by using a totally different approach for theme settings form setup. So given its fixed in D7, I've committed to D6. Thanks!
Comment #60
lolmaus CreditAttribution: lolmaus commentedI can't believe it's committed. O_O
Comment #61
juan_g CreditAttribution: juan_g commentedThank you very much!
For those who might want to test it now rather than waiting for Drupal 6.20, new bug fixes are added to Drupal 6.x-dev twice a day.
Comment #62
jonthomas83 CreditAttribution: jonthomas83 commentedEDIT
Sorry, somehow, overnight this has fixed itself! I now have theme specific issues :(
This patch is GREAT though! Nice one and thank you!
Comment #64
jadenoel CreditAttribution: jadenoel commentedDoes the fact that the patch in #26 failed SimpleTest mean I shouldn't/can't apply it?
Comment #65
mshepherd CreditAttribution: mshepherd commentedTry reading the rest of the thread. Specifically #42
Comment #66
pfortuna CreditAttribution: pfortuna commentedI believe you have to make sure that your $base_url in your settings.php file is not commented out.
$base_url = 'http://localhost/YOURSITE'; //no trailing slash