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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sociotech’s picture

Status: Active » Needs review
FileSize
2.32 KB

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

Status: Needs work » Needs review

Setting to 'needs review' - testbot was broken.

JohnAlbin’s picture

Status: Needs review » Postponed

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

sociotech’s picture

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!

JohnAlbin’s picture

JohnAlbin’s picture

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

sociotech’s picture

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?

JohnAlbin’s picture

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

JohnAlbin’s picture

Version: 7.x-dev » 6.x-dev
mattyoung’s picture

.

sociotech’s picture

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.

sociotech’s picture

Status: Needs work » Needs review
FileSize
4.18 KB

Sorry, ignore the previous patch as it was incorrectly rolled.

Attached is a correct patch that should apply properly against Drupal 6.14.

sociotech’s picture

duplicate post

sociotech’s picture

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!

flndr’s picture

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

warning: in_array() [function.in-array]: Wrong datatype for second argument in /base path/sites/all/modules/skinr/skinr.module on line 145.

Other than that, looks great.

sociotech’s picture

justinzero,

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

flndr’s picture

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

psynaptic’s picture

Is the current workaround to duplicate theme settings in the sub-theme?

sociotech’s picture

psynaptic,

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.

psynaptic’s picture

Thanks sociotech :)

aitala’s picture

I 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

sociotech’s picture

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

vrajak@gmail.com’s picture

When trying to use the patch through terminal, I get the following:

<system_admin_inherit_theme_settings_v3.patch can't find file to patch at input line 8
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: modules/system/system.admin.inc
|===================================================================
|RCS file: /cvs/drupal/drupal/modules/system/system.admin.inc,v
|retrieving revision 1.63.2.13
|diff -u -r1.63.2.13 system.admin.inc
|--- modules/system/system.admin.inc	16 Sep 2009 18:02:32 -0000	1.63.2.13
|+++ modules/system/system.admin.inc	6 Nov 2009 09:03:38 -0000
--------------------------
File to patch: system_admin_inherit_theme_settings_v3.patch
patching file system_admin_inherit_theme_settings_v3.patch
Hunk #1 FAILED at 503.
Hunk #2 FAILED at 562.

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.

vrajak@gmail.com’s picture

JohnAlbin’s picture

@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:

  1. It breaks the D6 string freeze. The changes aren’t essential and the old string can be re-used since it is still accurate. “These settings only exist for the %theme theme and all the styles based on it.” To me, that still works even when base theme settings are merged into mix. A site admin doesn't need to know that some of the settings are from the base theme.
  2. In the “Call and merge base theme settings” section, it looks like you've accidentally altered the API to pass the $form as the second param.
  3. As much as I hate the "phptemplate_" prefix… ;-) We have to leave it as an option for the theme settings function; phptemplate_settings() should still work after the patch.

I rolled a new patch that more closely matches the code in D7, while still retaining the same API flaws in D6. :-p

sociotech’s picture

John,

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.

aitala’s picture

I gather this patch did not make it into D6.16? I just have not had time to do any testing...

Eric

klonos’s picture

This should be in the next D6.17! Subscribing...

ultimike’s picture

John,

What is the status of this patch? It doesn't look like it's been tested by the automated testing server?

Thanks,
-mike

ChrisRut’s picture

subscribing

lolmaus’s picture

Subscribing

ayalon’s picture

Should be in the next Drupal release! Subtheming of popular Drupal Themes is not possible due to this bug.

ayalon’s picture

I 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. :-/

ayalon’s picture

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

frank0987’s picture

pimok3000’s picture

subscribing

sociotech’s picture

ayalon,

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.

jensimmons’s picture

subscribe

adrinux’s picture

Here'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:

// Include the definition of phptemplate_settings().
include_once $base_path . drupal_get_path('theme', 'MYBASETHEME') . '/theme-settings.php';

function MYSUBTHEME_settings($saved_settings){
    return phptemplate_settings($saved_settings);
}
mshepherd’s picture

#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

JohnAlbin’s picture

What is the status of this patch? It doesn't look like it's been tested by the automated testing server?

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

myregistration’s picture

Sorry for my ignorance, I'm new. How do we include the theme-settings.php script per #40? Thanks!

mshepherd’s picture

create 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)

dafeder’s picture

I 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

abaddon’s picture

@#45 - try to rename phptemplate_settings to BASE_THEME_NAME_settings , it should work.. the base theme implemented the hook with a different name

rajivnarayana’s picture

It 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

Jeff Burnz’s picture

sub

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

I took a look at it, coding style looks ok to me.

Powered by Dreditor.

and #47 seems like a functional test was done.

Jeff Burnz’s picture

Status: Reviewed & tested by the community » Needs review

@49, nope, please see the original issue:

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

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.

Edgar Saumell’s picture

#40 worked for me on a custom sub-theme of acquia_prosper which is a sub-theme of fusion_core

abaddon’s picture

i 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

discipolo’s picture

Tested against D6.19 without errors. seems to do the trick!

webkenny’s picture

Amazing as this might seem for a patch open over a year, I can confirm that this works against 6.19!! Nice job.

paskainos’s picture

subscribing

Drupaliens’s picture

subscribing
It as been commented on Durpal Commons website
http://commons.acquia.com/discussion/more-themes-drupal-commons#comment-...

servantleader’s picture

Status: Needs review » Reviewed & tested by the community

+1

mariomaric’s picture

Subscribing...

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

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

lolmaus’s picture

I can't believe it's committed. O_O

juan_g’s picture

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

jonthomas83’s picture

EDIT

Sorry, somehow, overnight this has fixed itself! I now have theme specific issues :(

This patch is GREAT though! Nice one and thank you!

Status: Fixed » Closed (fixed)

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

jadenoel’s picture

Version: 6.x-dev » 6.19

Does the fact that the patch in #26 failed SimpleTest mean I shouldn't/can't apply it?

mshepherd’s picture

Try reading the rest of the thread. Specifically #42

pfortuna’s picture

I 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