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.)
- Download and install those 2 themes. http://drupal.org/files/test-themes.tgz
- Enable both themes
- Look at the base theme's "Settings" form. Look at the missing value in the sub-theme's "Settings" form.
- Apply the patch and visit admin/appearance to clear the .info file cache.
- Look at the no-longer-missing value in the sub-theme's "Settings" form.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#40 | theme-object-data-761608-40.patch | 12.31 KB | JohnAlbin |
#38 | theme-object-data-761608-38.patch | 12.24 KB | JohnAlbin |
#36 | theme-object-data-761608-36.patch | 12.12 KB | JohnAlbin |
#34 | theme-object-data-761608-34.patch | 7.35 KB | JohnAlbin |
#32 | theme-object-data-761608-25.patch | 14.26 KB | effulgentsia |
Comments
Comment #1
Dries CreditAttribution: Dries commentedI'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!
Comment #2
treksler CreditAttribution: treksler commentedwell does this not completely break the theme settings form for subthemes? in that case it is critical
Comment #3
dvessel CreditAttribution: dvessel commentedI 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.
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.
Comment #4
aquariumtap CreditAttribution: aquariumtap commentedThe $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}.Comment #5
star-szrSub-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.
Comment #6
JohnAlbinAs 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
Comment #7
JohnAlbinOk. 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.
Comment #8
aquariumtap CreditAttribution: aquariumtap commentedThe 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 abase_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.
Comment #9
JohnAlbinYes, 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 usedlist_themes()
) used to work with inherited base theme settings. From comment #5: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.
Comment #10
aquariumtap CreditAttribution: aquariumtap commentedOkay, 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().
Comment #11
JohnAlbinNew to core dev? Excellent! Glad to have you. Thanks for taking the time to investigate this. It's not an easy problem.
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.
Comment #12
JohnAlbinOk. 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? :-)
Comment #13
JohnAlbinI think the "-d8" patch is being ignore because of the filename. Renaming it and re-uploading.
Comment #14
JohnAlbinGuh. this time with the patch. :-p
Comment #15
aquariumtap CreditAttribution: aquariumtap commentedLooks good! You rock!
Things I checked:
Comment #16
JohnAlbinFurther evidence that list_themes() should be returning a base_themes array. From ctools module's _ctools_list_themes() in includes/plugin.inc:
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. :-)
Comment #17
JohnAlbin#14: theme-object-data-761608-12.patch queued for re-testing.
Comment #19
JohnAlbinHere's some test themes that make it super easy to test this patch.
Comment #19.0
JohnAlbinAdd proper issue summary
Comment #20
JohnAlbinHere's a re-rolled patch after the test themes were moved out of core/themes/tests/.
Comment #20.0
JohnAlbinAdded note about test themes.
Comment #21
JohnAlbinComment #23
JohnAlbinDidn't realize that modules need to call hook_system_theme_info() to get themes into the system. Fixed the tests.
Comment #24
JohnAlbinTagging
Comment #25
JohnAlbinRe-rolled the D7 patch while I was here.
Comment #25.0
JohnAlbinNote about clearing cache.
Comment #26
fubhy CreditAttribution: fubhy commentedWorks for me and it is a definitive must-have feature for all the base themes out there indeed.
Comment #27
sheena_d CreditAttribution: sheena_d commentedI 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:
The results were as follows:
Comment #28
webchickThis 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.
Comment #29
barraponto CreditAttribution: barraponto commented@webchick @catch can we get it commited for d8, at least?
Comment #30
catchCommitted/pushed to 8.x, moving back to 7.x.
Comment #31
JohnAlbinI'm re-uploading the same patch from #25 so the testbot can have a go.
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedTrying #31 again to kick testbot.
Comment #34
JohnAlbinI moved the test themes to the wrong directory in the last D7 patch. Updated.
Comment #36
JohnAlbinHuh. Only half the patch made it to comment #34. Re-uploading.
Comment #38
JohnAlbinOk, 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.
Comment #40
JohnAlbinFixing the last fix. :-p
Comment #41
JohnAlbinI 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.
Comment #42
catchRe-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.
Comment #43
David_Rothstein CreditAttribution: David_Rothstein commented#40: theme-object-data-761608-40.patch queued for re-testing.
Comment #44
David_Rothstein CreditAttribution: David_Rothstein commentedPatch looks great to me, but two small questions before committing it to D7:
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).
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.
Comment #45
Gyver06 CreditAttribution: Gyver06 commentedSubscribing !
Comment #46
fubhy CreditAttribution: fubhy commented@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.
Comment #47
Gyver06 CreditAttribution: Gyver06 commentedSorry, I didn't know that. Thank you very much. It is done now.
Comment #48
JohnAlbinThere'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.
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.
Comment #49
David_Rothstein CreditAttribution: David_Rothstein commentedThanks 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
Comment #50
David_Rothstein CreditAttribution: David_Rothstein commentedAlso 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
Comment #52
David_Rothstein CreditAttribution: David_Rothstein commentedPossible problem here: #1708722: Call to undefined function drupal_find_base_themes() in drupal-7.15/includes/module.inc on line 184
Comment #52.0
David_Rothstein CreditAttribution: David_Rothstein commentedAdd bold text