Missing theme settings values because list_themes() has inconsistent theme object data

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!

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

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.

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

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.

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.

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().

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)

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

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.

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.

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

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

Trying #31 again to kick testbot.

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.

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

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.

Subscribing !

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

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

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

Also created a followup for the second issue: #1663606: Document that 'sub_themes', and other keys, in the array returned by list_themes() are sometimes not set

Possible problem here: #1708722: Call to undefined function drupal_find_base_themes() in drupal-7.15/includes/module.inc on line 184

Add bold text