Problem/Motivation

A sub-theme is supposed to inherit the default theme settings of its base themes. So if a base theme has a setting in its .info file like so:

settings[zen_skip_link_anchor]     = main-menu

then a sub-theme of that base theme doesn't need to repeat that line in its .info file.

This becomes critical when a base theme adds a new theme setting. The functionality can depend on a value being set, but, currently, the sub-theme does not properly inherit that theme setting value.

Drupal tries to find the base themes' setting values in theme_get_setting(), but fails because it is expecting an array of the sub-theme's base themes in $theme_object->base_themes. That array is only sometimes available from list_themes, as it conditionally gets its list of themes from either system_list() or _system_rebuild_theme_data().

Proposed resolution

_system_rebuild_theme_data() determines the base themes array using a helper function, system_find_base_themes(). We need system_list() to start using that helper function too. Unfortunately, system_list is in core/includes/modules.inc, so we have to move system_find_base_themes() from system.module to theme.inc. We also have to rename it to drupal_find_base_themes() so it makes sense in its new home.

Remaining tasks

Someone to RTBC the patch!

This is really easy to test if add a text field to a base theme and set the default value for that new text field in the base theme's .info file. Before the patch, you won't see the text field have any value in any sub-theme's settings form. And after the patch is applied, the sub-theme's settings form will show the base theme's default value.

To make it super easy to review, you can download a base theme/sub-theme combo that has these settings already configured. (Attached in comment #19 below.)

  1. Download and install those 2 themes. http://drupal.org/files/test-themes.tgz
  2. Enable both themes
  3. Look at the base theme's "Settings" form. Look at the missing value in the sub-theme's "Settings" form.
  4. Apply the patch and visit admin/appearance to clear the .info file cache.
  5. Look at the no-longer-missing value in the sub-theme's "Settings" form.
  6. Mark this issue RTBC.

User interface changes

A sub-theme's settings form will now have the correct values set by default. Yayz!

API changes

Rename system_find_base_themes() to drupal_find_base_themes().

Original report by JohnAlbin

Over in #563708: Improve theme_get_setting() and make custom theme settings a true form_alter we discovered a regression such that a base theme's settings are ignored when using theme_get_setting().

list_themes() used to return a fully-loaded theme object that included an array of base themes in the 'base_themes' key, but it no longer does. I'm not sure how the regression was introduced at this point, but looking at the code it seems we have different data being returned depending on if the database is active or not. In other words, _system_rebuild_theme_data() returns sufficient data, but system_list() returns inadequate data.

For the record: list_themes(), system_rebuild_theme_data(), _system_rebuild_theme_data(), system_list('theme'), and _system_theme_list() all return various lists of themes.

So we've got a bit of a mess. I'm still investigating.

Files: 
CommentFileSizeAuthor
#40 theme-object-data-761608-40.patch12.31 KBJohnAlbin
PASSED: [[SimpleTest]]: [MySQL] 39,116 pass(es).
[ View ]
#38 theme-object-data-761608-38.patch12.24 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] 38,945 pass(es), 44 fail(s), and 368 exception(s).
[ View ]
#36 theme-object-data-761608-36.patch12.12 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] 38,996 pass(es), 0 fail(s), and 57 exception(s).
[ View ]
#34 theme-object-data-761608-34.patch7.35 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme-object-data-761608-34.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#32 theme-object-data-761608-25.patch14.26 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 37,883 pass(es), 6 fail(s), and 114 exception(s).
[ View ]
#31 theme-object-data-761608-25.patch14.26 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] 38,110 pass(es), 7 fail(s), and 117 exception(s).
[ View ]
#25 theme-object-data-761608-25-D7.patch14.26 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme-object-data-761608-25-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 theme-object-data-761608-23.patch14.6 KBJohnAlbin
PASSED: [[SimpleTest]]: [MySQL] 33,358 pass(es).
[ View ]
#20 theme-object-data-761608-20.patch13.87 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] 33,350 pass(es), 8 fail(s), and 16 exception(es).
[ View ]
#19 test-themes.tgz749 bytesJohnAlbin
#14 theme-object-data-761608-12.patch10.15 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme-object-data-761608-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 theme-object-data-761608-12-D8.patch10.15 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme-object-data-761608-12-D8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 theme-object-data-761608-12-D7.patch9.44 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme-object-data-761608-12-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Dries’s picture

Priority:Critical» Normal

I'm not sure this is critical -- it doesn't sound like this would be a release blocker. If you really think it should hold up a release, please explain why, and set the priority back to 'critical'. Thanks!

treksler’s picture

well does this not completely break the theme settings form for subthemes? in that case it is critical

dvessel’s picture

I just noticed this and traced it back to system_rebuild_theme_data() > system_update_files_database().

Notice the last block before the query is executed. The keys for each query value do not match what's passed into that function. The keys for each theme are these but the query doesn't add all of them into the db.

uri
filename
name
info
owner
prefix
template
base_themes
type
status
schema_version
weight

I don't know how the db works so I don't know of a fix but I'm certain that's where it's all happening.

aquariumtap’s picture

The $themes data object, when taken from system_list(), is built from the {system} table. That table has a column called info, which contains a serialized array with details about a given theme.

Inside of that serialized array, there's a key called 'base_theme', and its value is a string. Missing are all the variables that dvessel listed, including the array base_themes.

That's why John is seeing a difference between the data object loaded by querying the database vs. one that's built using the filesystem.

The theme_get_setting() function expects the "info" array to contain the key 'base_themes', with an array as a value. That's missing, so subthemes are unable to inherit their parents' settings. I'm not sure where base_theme (singular) is used, or if it should ever be used, since in theory, Drupal's subtheme system supports multiple inheritance.

One solution would be to alter system_update_files_database() to include "base_themes" inside of $file->info. That would let us repair theme setting inheritance without altering the data structure for {system}.

Cottser’s picture

Sub-themes will become a lot more useful once this is fixed. The only workaround I know of is copying the settings from the base theme into the sub-theme's .info file, and that makes me cringe. Subscribing.

JohnAlbin’s picture

Title:theme object data returned from list_themes() is inconsistent» list_themes() usually has missing theme object data
Version:7.x-dev» 8.x-dev
Priority:Normal» Major

As I add new theme settings to a base theme, the base theme's default values are not being inherited in any sub-theme. Since a settings default values can be critical for the theme to display information correctly, this bug can cause all sorts of Usability and Accessibility issues (depending on what the setting was trying to perform.

For example, a setting can be used to define the text used in an accessibility-helping "skip link". If the sub-theme doesn't inherit that setting, the skip link's text will be completely blank, making the "accessible skip link" completely inaccessible. :-( This is not theoretical, I can't release Zen 7.x-3.2 until this core bug is fixed or I'll break that accessibility feature in tens of thousands of websites. #1357538: PHP warning message about in_array() after updating Zen theme

JohnAlbin’s picture

Ok. Digging into this properly now.

In addition to theme_get_setting() calling list_themes() and expecting a base theme list (and not getting one)…

These two functions call list_themes() and build their own list of base themes or sub-themes. Efficient!

The base_themes array that theme_get_setting() was expecting is built during _system_rebuild_theme_data()’s system_find_base_themes().

There is definitely some re-factoring that needs to happen as there is a lot of overlap and between list_themes() and system_rebuild_theme_data(). There's more useful meta-data being created during system_rebuild_theme_data(), but most functionality shouldn't be "rebuilding" the data just to get at the useful data.

However, I think the refactoring should be a follow-up to a much simpler bugfix which we need to write now for D7.

aquariumtap’s picture

Version:8.x-dev» 7.x-dev

The crux of the issue is list_themes() is not returning 'base_themes' along with its return array. That breaks inheritance in theme_get_setting(), which tries to loop through a base_themes array that is unavailable.

So why doesn't list_themes() include base_themes? Should it?

I don't think it should. That list_themes() function pulls data from the registry, and the registry contains info from the filesystem (.info files). The meaningful metadata that @dvessel pointed out is not stored in the registry, and it probably won't be -- not in 7.x, anyway. The 'base_theme' (singular) comes from the .info file; 'base_themes' (plural / array) is meta data generated during runtime.

In that case, the only option I see is to augment theme_get_setting() to gather the meta data it wants: the base_themes array. This can perhaps be done with system_find_base_themes().

I agree with @JohnAlbin that any refactoring should be a separate effort. Marking this as 7.x-dev to focus on a bug patch for this branch.

JohnAlbin’s picture

Version:7.x-dev» 8.x-dev

So why doesn't list_themes() include base_themes? Should it?

Yes, it should. If the database is unavailable, list_themes() does return the base_themes array. It used to return it all the time, but sometime before Drupal 7.0 was released something changed and this bug was introduced.

You can see in the original theme settings issue (#563708: Improve theme_get_setting() and make custom theme settings a true form_alter) that theme_get_setting() (which only ever used list_themes()) used to work with inherited base theme settings. From comment #5:

Verified that theme settings from the base theme are inherited by the sub-theme.

Also, you can clearly see that the existing code in theme_get_setting() expects to get a base_themes array from list_themes().

So, I have to disagree with you, Jason. :-) list_themes() should be returning the base_themes and sub_themes meta data at all times and not just sometimes like it does now.

Also, we have to get bug fixes into D8 before they can be applied to D7.

I'm just finishing the simple tests on a patch right now. Will post it when they are working correctly.

aquariumtap’s picture

Okay, understood. Thanks for the explanation! I'm new to core dev, sorry about the version faux pas

Will the goal be to get all meta data produced by _system_rebuild_theme_data() into the registry? If I remember correctly from reading through the code, that should get base_themes back into list_themes().

JohnAlbin’s picture

New to core dev? Excellent! Glad to have you. Thanks for taking the time to investigate this. It's not an easy problem.

Will the goal be to get all meta data produced by _system_rebuild_theme_data() into the registry?

I think in the refactoring for D8, that might be a good solution. But for the first-round bugfix, I think we only need to determine the base themes and sub themes.

JohnAlbin’s picture

Status:Active» Needs review
StatusFileSize
new9.44 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme-object-data-761608-12-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new10.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme-object-data-761608-12-D8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ok. list_themes() is in includes/theme.inc and it grabs its data from system_list('theme') which is in includes/module.inc.

The base_themes array is generated from system_find_base_themes() which is in system.module since it was only ever called from system.module's system_rebuild_theme_data().

I don't think its kosher to call system_find_base_themes() from theme.inc or module.inc since it adds a dependency on system.module. So we need to move system_find_base_themes() into theme.inc. And if we do that, we can't call it "system_find_base_themes" anymore. So we should move it into theme.inc and rename it drupal_find_base_themes().

Here's the patches for D8 and D7 that uses that method to fix the bug in list_themes(). After those are committed, we can work on the refactoring in D8.

Jason, maybe you'd care to review these patches? :-)

JohnAlbin’s picture

I think the "-d8" patch is being ignore because of the filename. Renaming it and re-uploading.

JohnAlbin’s picture

StatusFileSize
new10.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme-object-data-761608-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Guh. this time with the patch. :-p

aquariumtap’s picture

Looks good! You rock!

Things I checked:

  • A core function has been moved: system_find_base_themes(). Does the depreciated function still exist as a wrapper in D7? (YES)
  • The system_find_base_themes() wrapper won't be needed in D8. Has it been removed completely? (YES)
  • D7 before patch: unable to get inherited theme setting? (YES)
  • D7 after patch: able to get setting from base theme when subtheme is the default? (after clearing the cache, YES)
  • D8 before patch: unable to get inherited theme setting? (YES)
  • D8 after patch: able to get setting from base theme when subtheme is the default? (after clearing the cache, YES)
JohnAlbin’s picture

Further evidence that list_themes() should be returning a base_themes array. From ctools module's _ctools_list_themes() in includes/plugin.inc:

    $all_themes = list_themes();
    foreach ($all_themes as $name => $theme) {
      // Prior to drupal 6.14, $theme->base_themes does not exist. Build it.
      if (!isset($theme->base_themes) && !empty($theme->base_theme)) {
        $active[$name]->base_themes = ctools_find_base_themes($all_themes, $name);

where ctools_find_base_themes is a “a verbatim copy of system_find_base_themes(), which was not implemented until 6.14. It is included here only as a fallback for outdated versions of drupal core.” In other words, ctools expects list_themes() to return the base_themes array and the ctools_find_base_themes is a fallback if that array is not set (!isset($theme->base_themes)).

I still need someone to RTBC the patch. :-)

JohnAlbin’s picture

#14: theme-object-data-761608-12.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, theme-object-data-761608-12.patch, failed testing.

JohnAlbin’s picture

StatusFileSize
new749 bytes

Here's some test themes that make it super easy to test this patch.

JohnAlbin’s picture

Issue summary:View changes

Add proper issue summary

JohnAlbin’s picture

Status:Needs work» Needs review
StatusFileSize
new13.87 KB
FAILED: [[SimpleTest]]: [MySQL] 33,350 pass(es), 8 fail(s), and 16 exception(es).
[ View ]

Here's a re-rolled patch after the test themes were moved out of core/themes/tests/.

JohnAlbin’s picture

Issue summary:View changes

Added note about test themes.

JohnAlbin’s picture

Title:list_themes() usually has missing theme object data» Missing theme settings values because list_themes() has inconsistent theme object data

Status:Needs review» Needs work

The last submitted patch, theme-object-data-761608-20.patch, failed testing.

JohnAlbin’s picture

Status:Needs work» Needs review
StatusFileSize
new14.6 KB
PASSED: [[SimpleTest]]: [MySQL] 33,358 pass(es).
[ View ]

Didn't realize that modules need to call hook_system_theme_info() to get themes into the system. Fixed the tests.

JohnAlbin’s picture

Issue tags:+needs backport to D7

Tagging

JohnAlbin’s picture

StatusFileSize
new14.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme-object-data-761608-25-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled the D7 patch while I was here.

JohnAlbin’s picture

Issue summary:View changes

Note about clearing cache.

fubhy’s picture

Status:Needs review» Reviewed & tested by the community

Works for me and it is a definitive must-have feature for all the base themes out there indeed.

sheena_d’s picture

I have tested the D7 patch in #25 with a number of base themes. I git cloned Drupal and applied the patch then installed using the Standard profile.
After installation, I followed the following steps with each base theme:

  1. drush dl and en base theme.
  2. Created a simple sub-theme by re-naming the base theme's starterkit.
  3. Enabled the sub-theme.
  4. Visited the sub-theme's settings page.
  5. Removed all settings from the sub-theme's .info file.
  6. Cleared site cache.
  7. Visited sub-theme's settings page.

The results were as follows:

  • Zen sub-theme showed no issues.
  • Omega sub-theme showed no issues.
  • Adaptive Theme sub-theme showed no issues.
  • Fusion sub-theme showed a number of undefined index errors in fusion core's theme-settings.php file. This turned out to be caused by part of the theme settings form that dynamically generates a number of options based on the settings in the sub-theme's .info file. I have opened a Fusion core issue and provided a patch to solve this issue: #1421938: Pull default grid settings from Fusion core when sub theme's .info does not define them.
webchick’s picture

This looks fairly harmless, but I'd feel better about it if we had longer than 2 days of "in the wild" testing, so I am going to hold this until the next release. Sorry about that.

barraponto’s picture

@webchick @catch can we get it commited for d8, at least?

catch’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Needs review

Committed/pushed to 8.x, moving back to 7.x.

JohnAlbin’s picture

StatusFileSize
new14.26 KB
FAILED: [[SimpleTest]]: [MySQL] 38,110 pass(es), 7 fail(s), and 117 exception(s).
[ View ]

I'm re-uploading the same patch from #25 so the testbot can have a go.

effulgentsia’s picture

Issue tags:-needs backport to D7
StatusFileSize
new14.26 KB
FAILED: [[SimpleTest]]: [MySQL] 37,883 pass(es), 6 fail(s), and 114 exception(s).
[ View ]

Trying #31 again to kick testbot.

Status:Needs review» Needs work

The last submitted patch, theme-object-data-761608-25.patch, failed testing.

JohnAlbin’s picture

Status:Needs work» Needs review
StatusFileSize
new7.35 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme-object-data-761608-34.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I moved the test themes to the wrong directory in the last D7 patch. Updated.

Status:Needs review» Needs work

The last submitted patch, theme-object-data-761608-34.patch, failed testing.

JohnAlbin’s picture

Status:Needs work» Needs review
StatusFileSize
new12.12 KB
FAILED: [[SimpleTest]]: [MySQL] 38,996 pass(es), 0 fail(s), and 57 exception(s).
[ View ]

Huh. Only half the patch made it to comment #34. Re-uploading.

Status:Needs review» Needs work

The last submitted patch, theme-object-data-761608-36.patch, failed testing.

JohnAlbin’s picture

Status:Needs work» Needs review
StatusFileSize
new12.24 KB
FAILED: [[SimpleTest]]: [MySQL] 38,945 pass(es), 44 fail(s), and 368 exception(s).
[ View ]

Ok, so the D6 upgrade tests were failing because D6 plain themes don't need to specify engine = theme in their .info files, but D7 plain themes do. So the D6 plain themes were throwing PHP warnings when checking for that key. The only way to get around that is to check for a missing engine in system_list().

We don't need to make that same check in D8's system_list(), so there's no regression.

Status:Needs review» Needs work

The last submitted patch, theme-object-data-761608-38.patch, failed testing.

JohnAlbin’s picture

Status:Needs work» Needs review
StatusFileSize
new12.31 KB
PASSED: [[SimpleTest]]: [MySQL] 39,116 pass(es).
[ View ]

Fixing the last fix. :-p

JohnAlbin’s picture

I should note that, in comment #27, sheena_d tested the D7 patch in #25. The only difference between the patch in #40 and #25 is the fix to the D6->D7 upgrade test.

This should be fairly easy to test and RTBC.

catch’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+needs backport to D7

Re-roll looks good to me, I think the automated tests are enough here, and they're clearly catching bugs when we see them. Adding the backport tag for tracking.

David_Rothstein’s picture

#40: theme-object-data-761608-40.patch queued for re-testing.

David_Rothstein’s picture

Patch looks great to me, but two small questions before committing it to D7:

  1. @@ -400,7 +400,7 @@ function system_theme_settings($form, &$form_state, $key = '') {
       // Default settings are defined in theme_get_setting() in includes/theme.inc
       if ($key) {
         $var = 'theme_' . $key . '_settings';
    -    $themes = system_rebuild_theme_data();
    +    $themes = list_themes();

    Won't this change the behavior of the theme settings page? Previously you visited that page and automatically got the latest theme data from the filesystem; now you have to clear caches (or visit some other page which triggers a theme rebuild first) before getting the latest updates that have been made to the theme's info file.

    It's a small change, but for Drupal 7 I think we shouldn't do it unless it's necessary for the rest of this patch to work correctly (and my impression is that it's not necessary).

  2. - *   - 'base theme': The name of the base theme.
    + *   - 'base_theme': The name of the base theme.
    + *   - 'base_themes': An ordered array of all the base themes. If the first item
    + *     is NULL, a base theme is missing for this theme.
    + *   - 'sub_themes': An unordered array of sub-themes of this theme.
      */
    function list_themes($refresh = FALSE) {

    Will the 'sub_themes' key actually be set if the theme has no subthemes? It looks to me like it won't. If so, we should change the documentation to reflect that (or change the code to force it to be an empty array in this case).

    This comment affects D8 too, so would be OK to handle in a followup.

Gyver06’s picture

Subscribing !

fubhy’s picture

@Gyver Please use the "Follow" / "Unfollow" button in the top right corner of the issue summary to subscribe to issues instead of posting a comment.

Gyver06’s picture

Sorry, I didn't know that. Thank you very much. It is done now.

JohnAlbin’s picture

1. Won't this change the behavior of the theme settings page?

There's some slight confusion here. Yes, that change is in system_theme_settings() which is used on the theme settings tabs of the Appearance page, but we really don't need the side-effect of rebuilding the theme .info registry just to change to a theme's settings.

The documented behavior for rebuilding the theme .info registry is to visit the Appearance page, not to visit one of the theme settings tabs of the Appearance page. See http://drupal.org/node/337176

The main Appearance page callback uses system_themes_page() which still has the call to system_rebuild_theme_data().

So the behavior only changes on the theme settings tabs, a behavior which isn't even mentioned in any of our handbook pages.

2. Will the 'sub_themes' key actually be set if the theme has no subthemes? It looks to me like it won't. If so, we should change the documentation to reflect that (or change the code to force it to be an empty array in this case).

That is correct. It won't have that key if the theme has no sub-themes. But we already have the same situation in D7 with the base_themes array. hmm… Actually, we have the same issue with ALL of the properties in this array; they are only created if needed. Take a look at how stylesheets, scripts, engines, base_theme and status are created in list_themes(). I say leave it the way it is in D7. There's undoubtably code that is already doing isset on those properties in contrib.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Fixed
Issue tags:+7.15 release notes

Thanks for pointing out that documentation page - I didn't realize we had documentation that describes in so much detail which exact steps you should take to rebuild the theme cache.

If someone did update their theme and then visited the settings page by itself, they'll see some weird behavior after this patch, but no weirder than many other places, so given that it's explicitly documented to go to the main Appearance page when the cache needs to be cleared, I agree this is OK.

Committed to 7.x - thanks! Glad to get another major bug fixed.
http://drupalcode.org/project/drupal.git/commit/46cf232

David_Rothstein’s picture

Status:Fixed» Closed (fixed)

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

David_Rothstein’s picture

David_Rothstein’s picture

Issue summary:View changes

Add bold text