So one common task I'm starting to see in a couple D7 patches is determining the ancestors and/or the descendants of a theme. And trying to calculate that in various places in core seems silly when we have this perfectly capable .info cache which holds all kinds of meta data for themes and modules.

dww suggested I open a new issue to prevent kitten carnage in the other issues that depend on this data.

So, I'm proposing adding 2 new arrays to the .info data:

  • base_themes: which holds an array of all the ancestor base themes in order of their ancestry. The root of the ancestry is at the zero spot on the array and the parent of the sub-theme is at the last spot on the array. For example, a subsubsubzen theme (with obvious parentage) would have:
     'base_themes' => array(
      'zen' => 'Zen',
      'subzen' => 'Sub-Zen theme',
      'subsubzen' => 'Sub-Sub-Zen theme'), 
  • sub_themes: which holds an array of all the sub-themes based off this theme. The order of the array is alphabetical by theme name. For example, a zen theme would have:
     'sub_themes' => array(
      'subsubsubzen' => 'Sub-Sub-Sub-Zen theme',
      'subsubzen' => 'Sub-Sub-Zen theme',
      'subzen' => 'Sub-Zen theme'), 
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

Priority: Normal » Critical

Bumping the priority on this (and pointing out this needs to be backported to D6, so let's not get crazy with scope creep here) because this is blocking a critical bug fix for both D6 and D7: #456088: Sub-themes not notified of security updates for base themes.

Note to others: don't be confused by the fact that the example sub_themes array in the original post is exactly the opposite order of the base_themes example. That's just an (unfortunate) artifact of the letters used. A base theme can have a *lot* of sub themes potentially, and there's no good simple way to represent the ancestry in there. I think it makes sense to just give *all* the sub themes of a given base in a flat array, and give each subtheme an ordered array of its own ancestry. That should give you enough info to do whatever you need to do.

A more complete example would be something like:

'sub_themes' => array(
  'othersubzen' => 'Other Sub-Zen theme',
  'subsubsubzen' => 'Sub-Sub-Sub-Zen theme',
  'subsubzen' => 'Sub-Sub-Zen theme',
  'subsuzziesubzen' => 'Sub-Suzzie Sub-Zen theme'
  'subzen' => 'Sub-Zen theme', 
  'suzziesubzen' => 'Suzzie Sub-Zen theme'
)

;)

That said, why do we even bother sorting the subthemes of a given base? If someone wants it sorted, they can do that, but is it really important for us to always sort this array after parsing? Just curious.

JohnAlbin’s picture

Status: Active » Needs review
FileSize
3.09 KB

Here's the patch. I haven't done thorough testing on it.

JohnAlbin’s picture

Status: Needs review » Needs work
FileSize
12.29 KB

Ok. I did some thorough testing. And there's a typo in the patch. Plus I exposed a critical bug on the admin/build/themes page with my test suite.

Here's a tarball of my test themes that should thoroughly test the new patch (which I'll post in a follow-up post.)

  1. Expand these test themes into sites/all/themes
  2. Enable the D7 version of devel.module
  3. Visit admin/build/themes and make “Ruby Red” the admin theme

The Ruby Red theme will output the .info cache as krumo data in the "status messages" part of the page. You can expand that array to see the new base_themes and sub_themes data structures.

JohnAlbin’s picture

Title: Add theme lineage to .info cache » Add theme lineage to .info cache (and prevent WSOD on admin/build/themes)
Status: Needs work » Needs review
FileSize
4.44 KB

Ok. This new patch includes the base_themes and sub_themes data structures.

It does this by redoing system_find_base_theme, which returns a string, and making it into system_find_base_themes which returns an array.

Both the old and the new function had tests for making sure that an infinite theme loop doesn't occur. Meaning it won't WSOD when a parent theme says its child theme is its base theme (infinite loop of inheritance). In order to test that, I created some Zaphod Beeblebrox themes (in the test suite above) that have recursive inheritance.

Unfortunately, there's a critical bug in the admin/build/themes function, system_themes_form(), which doesn't check for the possibility of an infinite loop.

So in addition, this patch uses the new data structures in a modification to system_themes_form() so that you can actually test this patch by visiting admin/build/themes. :-p

tic2000’s picture

Why do we need the sub_themes array? Something specific or just to have the info in case we may need it?

JohnAlbin’s picture

@tic2000: Its specifically for Update module's usage. #456088: Sub-themes not notified of security updates for base themes

tic2000’s picture

@JohnAlbin
Then why is the base_themes array for? You don't need both for update module. And I would use base_themes for that.

dww’s picture

@tic2000: Please read the issue. On the available updates report, for usability, we want the base themes to list all their subthemes as "Required by:" and the subthemes to list all their base themes (there can be more than one) as "Depends on:". Therefore, we need to know both pieces of information for every theme.

dww’s picture

@JohnAlbin: I'm concerned about the API change for D6 here in system_find_base_theme(s)?. What if we just *added* system_find_base_themes() and left a comment that system_find_base_theme() is deprecated? I haven't looked closely at the patch and the surrounding code (and I won't really have time to do so for probably a week), but I wanted to ask since you've got this all swapped in right now and are thinking about it. Thanks!

JohnAlbin’s picture

What if we just *added* system_find_base_themes() and left a comment that system_find_base_theme() is deprecated?

That's exactly what I was thinking of doing for the D6 version of the patch. :-)

JohnAlbin’s picture

Added some additional comments to system.admin.inc.

And here's the patch for D6 as well.

JohnAlbin’s picture

FileSize
12.25 KB

And here's the test suite for D6. [edit: don't use this version]

JohnAlbin’s picture

FileSize
12.26 KB

Here's an updated test suite for D6. The previous "test for missing base theme" theme was using Chameleon as its base theme, but that's actually still in D6. :-p

JohnAlbin’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

tstoeckler’s picture

Status: Needs work » Needs review

Testbot broke.

sociotech’s picture

+1 for this patch, which is really important for full theme inheritance to work.

In the meantime, I've been passing in $used_keys by reference (e.g., as &$used_keys) to system_find_base_theme(). Doing this, $used_keys comes back with the other parent themes (if any) for a subtheme, minus the top level parent theme, which the function returns explicitly anyway. Put it all together and you can construct the lineage.

Problem is, passing variables by reference when you call a function is now deprecated in PHP and throws a warning unless you disable it in php.ini. So, I'd much prefer to get the lineage in a way that avoids warnings.

An alternative, of course, would be to just define $used_keys as passable by reference in the function definition itself. But John's patch is more explicit/less obscure and would be preferred.

Jacine’s picture

+1

merlinofchaos’s picture

I am in favour of this patch, tho I don't have the time (or energy actually) to review the actual code just now.

JohnAlbin’s picture

Issue tags: +Needs backport to D6

Adding “needs backport to D6” issue tag since this patch is needed for a critical bug in D6.

tic2000’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed the patch. It still applies cleanly, WOW.

What I noticed is that now we have an array of base themes in the form of grand grandparent > grandparent > parent.
But in the case of a circular relationship this order brakes. After talking with John on irc he said Its a inconsistency, but circular dependency is a failure. Currently, I"m noting that failure by having the first array item have a NULL value. It has a key, but a NULL value. That way the system knows what it was _trying_ to find, but failed to find. which makes perfect sense IMO.

What I would do next (after this one gets in) is to not display all those themes that will result in a failure (those in an infinite loop and those without a missing base theme)

JohnAlbin’s picture

What I would do next (after this one gets in) is to not display all those themes that will result in a failure (those in an infinite loop and those without a missing base theme)

Yep, yep. That's what #495040: Themes can be enabled even if base themes are missing and #495026: Circular dependency in theme .info data causes WSOD are for.

Just a reminder, the easiest way to review this patch is to follow the instructions in comment#3 above.

sociotech’s picture

John,

Reviewed the D6 version of the patch per your instructions in comment #3 and everything works as advertised. Got the correct base_themes and/or sub_themes arrays as expected with each theme in the minelli -> liza -> caberet lineage.

Of course, due to the circular nature of their base theme references, I got strange results with the zaphod3 -> zaphod2 -> zaphod lineage. Every theme had every other theme listed as a base theme and none of them had any subthemes. I assume this is just the default behavior in this situation. No WSODs at admin/build/themes though!

In addition, I went ahead and re-implemented a theme function of mine that retrieves the base theme lineage (with paths) using the new system_find_base_themes() function. No problems and it generated identical output with less code :)

From my tests, this patch looks good and is RTBC.

Thanks for the patch and your other work to fix Drupal's sub-theme system!

webchick’s picture

Spent about an hour on this patch. Thanks so much for the excellent test data and thorough instructions. It made it much easier to see the problems (notably, notices about non-existant base themes on admin/build/themes and infinite loops causing exhaustion of memory).

Normally I'd hold a patch like this for test coverage since the bugs that are fixed here are very clearly edge of edge cases. Unfortunately, we have no means of testing themes currently. :( I've cross-referenced this test data at #343898: Let SimpleTest test the theme system so we can hopefully include it later.

Once I finally wrapped my head around what was going on, I really only came up with some minor stuff:

- It's odd that it's 'base theme' in the .info file but base_themes in the object. But this would require introducing a wtf to themers to make them match since base_themes is a variable name, so nothing to do here.

- There are places you've used AS instead of as. Please fix.

-

+      // Don't continue if there was a problem with the root base theme.
+      if (!current($themes[$key]->base_themes)) {
         continue;
       }

very minor, but the comment says "don't continue" and the code says "continue" ;) I know what you mean, but could we clarify?

- + // Look for the most accurate screenshot.

I think I know what you mean but spelling it out for me might be good here.

JohnAlbin’s picture

New patch with the updated comments and "AS" capitalization fixed.

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Ok, great. Testing bot is happy, so I've committed this to HEAD!

Moving down to 6.x for consideration. Marking to be ported because of the minor changes.

JohnAlbin’s picture

Status: Patch (to be ported) » Reviewed & tested by the community
FileSize
4.41 KB

I made the updates from #24 in the D6 patch. Based on sociotech's comment in #23, marking this RTBC.

JohnAlbin’s picture

Issue tags: -Needs backport to D6

Removing tag

sociotech’s picture

Status: Reviewed & tested by the community » Needs work

John,

Unfortunately, I found what looks to be a bug in the D6 (and D7) version of this patch that will prevent a sub-theme from displaying its own unique screenshot.png, rather than its parent's, on the theme listing page.

On line 17 of the patch (line 203 of system.admin.inc after the patch has been applied), the current theme is added to an array of base themes to check for screenshot images:

$theme_keys = array_keys($themes[$theme->name]->base_themes) + array($theme->name);

But both the array of base themes and the current theme array will have a zero-keyed element ([0]) in their arrays, so the plus sign operator ("+") won't add the current theme to the combined $theme_keys array. This is because the plus operator doesn't overwrite duplicate keys. Less obviously, it also doesn't _add_ them either, leaving us without the current theme in the new array.

What would do the trick here is an array_merge() function like so:

$theme_keys = array_merge(array_keys($themes[$theme->name]->base_themes), array($theme->name));

Sorry I didn't catch this earlier in my testing, but the test data didn't include screenshot images and I didn't notice the missing unique screenshots on my own sub-themes until now.

JohnAlbin’s picture

Version: 6.x-dev » 7.x-dev
Category: task » bug
Status: Needs work » Needs review
FileSize
833 bytes

@sociotech. No worries. The fix is easy. I'd forgotten that adding arrays together looks at the key values (which since they are numeric) aren't unique in this case. Hence, the array that is "added" isn't added at all.

Temporarily putting back on D7 to get that fixed.

Before this patch, you wouldn't see the screenshot for Minnelli (it would incorrectly show Garland’s). After the patch, the proper screenshot is restored.

Please review.

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Needs review » Patch (to be ported)

Confirmed the bug and the fix.

Committed to HEAD.

This change needs to be added to the 6.x patch so "to be ported".

JohnAlbin’s picture

Category: bug » task
Status: Patch (to be ported) » Needs review
FileSize
4.43 KB

Thanks!

And here's the updated D6 patch.

tic2000’s picture

Status: Needs review » Reviewed & tested by the community
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thank you! Committed to Drupal 6 too. Would love to see the WSOD and security related improvements too :)

Status: Fixed » Closed (fixed)

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