Sub-themes not notified of security updates for base themes

Nasty. Sounds a bit related to #162788: Include modules that aren't enabled but this is obviously a different problem. This is going to require some serious thinking. I don't think subthemes existed when update_status was first written, and clearly no one considered the implications for update.module when they were added. I certainly didn't. ;) i confess to not being too deep in the theme world... alas.

I need to understand more about how subthemes work and are handled by core to start to have a clue about how to patch update.module to handle this. Maybe we should try to chat in IRC some time...

No time to digest it now, but via email, John sent the following code-snippet. Probably a very good place to start if anyone's interested in digging into this:

 $projects_to_check = array();
 $active_themes = array();
 // Grab the themes listed on admin/build/themes.
 $themes = list_themes();
 foreach ($themes AS $theme => &$value) {  // For D6, we should use: foreach (array_keys($themes) AS $theme) { // which is slower, but PHP4 compatible
   // We only need to process active themes.
   if ($themes[$theme]->status) {
     $active_themes[$theme] = $theme;
   }
 }
 // Now loop over the active themes to find the project information.
 foreach ($active_themes AS $theme) {
   if (!empty($themes[$theme]->info['project'])) {
     $projects_to_check[$theme] = array(
       'project' => $themes[$theme]->info['project'],
       'datestamp' => $themes[$theme]->info['datestamp'],
       'version' => $themes[$theme]->info['version'],
     ); // I don't actually know what data upload.module needs.
   }
   // Also, check any base themes by tacking them onto the end of $active_themes.
   // Not sure if adding items to a variable as you loop over it works; I hope! :-D
   // But this should work, undiscovered themes get added to the end of $active_themes
   // and already-looked-at themes have already been looped over in this foreach, so
   // hopefully no infinite loop.
   if (!empty($themes[$theme]->base_theme)) {
     $active_themes[$themes[$theme]->base_theme] = $themes[$theme]->base_theme;
   }
 }

 // Now $projects_to_check should have a list of all active themes and disabled base themes and their project data.

subscribe

@JohnAlbin: This is great, thanks! I need to run now, but I'll definitely give this a close review mon or tues.

One minor concern: t('Sub-themes: %subthemes') isn't going to make Gabor happy with the D6 backport breaking the string freeze, especially since it's directly on the main update status report. :( Any ideas on ways to fix this without breaking translations? Might be impossible, and such is life, but I wanted to ask in case there's another way.

Also: did subthemes exist in D5? Wondering if this needs to be backported to D5 contrib update_status, too.

Thanks!
-Derek

Sorry, I got swamped with other things and didn't have a chance to look at this patch. I just tried testing it and it's definitely a step in the right direction, but it's slightly broken:

A) Once you apply the patch, the "Modules" heading on admin/reports/updates goes away, and everything other than core is lumped together under "Themes". I'll take a quick look now, I think I might know what the problem is.

B) Do we want to call the base theme a "Theme" or a "Disabled theme"? Not sure which is more confusing. There's already support and strings for "Disabled theme", so that's not going to break the string freeze if we start using it. We just need to agree on how we want this to show up. I guess the argument behind this issue is that even though the base theme is itself disabled, it's effectively enabled if a subtheme of it is enabled... Is that an accurate way to look at it?

I also ran into some weird interactions with my test case between zen/STARTERKIT and cvs_deploy, but that's all resolved now. ;) If you do it that way, be sure that when you copy zen/STARTERKIT/STARTERKIT.info to your subtheme folder, you need to remove any d.o packaging lines added to the .info file or if you use cvs_deploy, you need to not copy the STARTERKIT/CVS directory to your new subtheme...

(A) was caused by this line:

  if ($project_type = 'theme') {

;) Corrected in the attached patches.

Still not sure about (B)...

Sorry, that D7 patch is wrong, my workspace was both out of date and had some extra stuff in it. ;)

A few UI notes about (B). Here's a screenshot of what you see on admin/reports/updates with 456088-14.basetheme-updates.D6.patch

I think it's pretty subtle that the only hint why Zen is showing up on this report as if it were enabled is the "Sub-themes: Lucha" thing.

John and I were discussing maybe using "Required by: " instead. That also has the benefit that D6 core already has this string (thanks, modules/system/system.admin.inc): t('Required by: !required', array('!required' => ...)). So, if we do slighty weird things in the D6 backport, we can actually re-use this without even breaking the string freeze for this, something like so:

// Instead of using %required to get theme('placeholder') automatically, we use !required to avoid breaking the string freeze
t('Required by: !required', array('!required' => theme('placeholder', implode(', ', $project['subthemes']))));

Yay, nicely done, John! I've got some urgent work to do in the next few days, then hopefully I'll have time to revisit this. But if you get a chance to pick up where we left off, that'd be great! Thanks.

Any progress on this one?

Subscribing. Will review once John re-rolls his patch.

#489762: Add theme lineage to .info cache (and prevent WSOD on admin/build/themes) is in D6 too!

Oh, that's a problem.

I'm working on a test for this, based on #343898-8: Let SimpleTest test the theme system. However, it seems that while the subtheme says it depends on the basetheme, it doesn't actually say it's out of date. Not sure if that's what we're really going for here. I'm testing with a subtheme that's using a core theme (Stark) as the base, and then setting core to be missing a security update. See attached screenshot. Is that what we want to see? Maybe I should write the test such that the base theme is also contrib, disabled, and missing a security update. Perhaps that would give us a cleaner view?

Ok, glad I'm trying to write tests for this. ;) The patch is still kinda fubar. Here's the test case:

Core == 7.0, which is latest available.
update_test_basetheme [disabled] = 7.x-1.0, but 7.x-1.1 is available and a security update.
update_test_subtheme [enabled] = 7.x-1.0, which is latest available.

So, we're expecting that update_test_basetheme is going to show up as missing a security update, and somehow the subtheme is going to reference that.

However, now that #162788: Include modules that aren't enabled landed, if the checkbox to show disabled modules and themes is not selected, you don't see any reference of the missing security update for update_test_basetheme at all. See 456088-27.subtheme-security-disabled-basetheme.png

But, if you enable that checkbox, and want to see disabled code, the report looks something like 456088-27.subtheme-security-show-disabled.png

Clearly this patch needs some more help. I'm trying to get feedback from yoroy and davereid in IRC right now as to what it *should* look like in this case.

Okay, there were some logic bugs in John's patch in #25, since things are a bit different now that #162788: Include modules that aren't enabled is in. I'm attaching the new code here as 456088-28.basetheme-updates.D7.patch which is better commented about what's going on, and is confirmed to be working properly. I'm also attaching a test that fails without this patch, and passes once it's applied, but the test depends on #343898-8: Let SimpleTest test the theme system, so I can't include the test in the main code patch or the testbot will be confused.

BTW, here's a screenshot of what the Available updates report looks like inside that test case, with the patch attached.

Yay, now that #343898-8: Let SimpleTest test the theme system is in HEAD, we can combine the patches in #28 into a single patch that the testbot will like.

When committing, you'll need the following CVS commands:

cvs add modules/update/tests/update_test_basetheme.1_1-sec.xml
cvs add modules/update/tests/update_test_subtheme.1_0.xml
cvs add themes/tests/update_test_basetheme
cvs add themes/tests/update_test_basetheme/update_test_basetheme.info
cvs add themes/tests/update_test_subtheme
cvs add themes/tests/update_test_subtheme/update_test_subtheme.info

Edited to add a few missing cvs add commands

Testing out the patch, and I changed the .info files of the two test themes to make them show up in update status. The base theme, even though disabled, shows up in the 'Themes' section, when it should be in the 'Disabled themes' section.

@Dave Reid: That's by design. If a subtheme is enabled, the base theme *is* enabled... That's exactly what this issue is trying to fix. We should *always* show the status of base themes when any of that base theme's subthemes are on. Notice the "Required by" part in the screenshot in #29... See also the code comments that explain this. Yoroy and I have gone around a few iterations on what the UI should exactly look like, but AFAIK, what I've implemented here is his favorite.

Also, this seems to be a collective effort, so I'm just setting it to anonymous. ;)

Since #162788: Include modules that aren't enabled doesn't exist in D6 core, these patches don't apply at all to D6. So, this is almost a complete re-roll from scratch. I also included the slightly weird t() stuff I mentioned in comment #16 (along with copious code comments) to avoid breaking the D6 string freeze to fix this bug. I hope Gabor is happy with that approach. ;)

@dww: I had both the base theme and sub theme disabled, but it was showing the base theme as an enabled module. That didn't seem right. I an understand having an enabled subtheme and a disabled base theme showing up, but that was not the case.

Oh, whoops. ;) I can't reproduce what Dave is seeing right now on my test site, but I'm seeing other problems. Looks like we need some more tests for this case. ;) When I manually force the two update_test_* themes to appear by defining their own project = update_test_basetheme (or _subtheme) line, giving them a version string, and commenting out hidden = TRUE, both show up at /admin/appearance but neither show up at /admin/reports/updates. Then, if I enable the checkbox to show disabled modules and themes, only the subtheme shows up, the basetheme doesn't show up at all...

Anyway, I'll take a look at see if I can track all this down and reroll. Stay tuned...

Same code as #30, but now with another test case specifically for if disabled themes are shown properly on the available updates report, both with and without the checkbox for displaying disabled modules and themes. This test fails in a few ways with the current code, but at least now we can properly debug wtf is going on here. ;)

Here you go. I had just confused myself a bit with this whole thing about always needing to invoke _update_process_info_list() for disabled themes. :( Instead, when we're processing it for *enabled* themes (regardless of the checkbox), we want to special case the situation where the base theme is disabled but it has enabled subthemes, and treat the base theme as enabled in that case. See attached patch. Now passes all the tests, even the new ones I added in #36 that will fail with that code.

Tested, and this works exactly as it should. Disabled themes without any kind of subtheme or basetheme are not shown unless they have an enabled base ou sub theme with an update! Great job dww and JohnAlbin!

For UX review, here are 3 screenshots (from how the available updates report looks during the 3 new test cases added in this patch):

456088-39.basetheme-security.png shows the case we care about for the bug here -- the subtheme is enabled, the base theme is disabled, but the base theme is missing a security update.

456088-39.show-disabled-themes.png shows how things look when both the base theme and subtheme are disabled, but your available updates report is configured to show disabled modules and themes.

456088-39.hide-disabled-themes.png is with both disabled, and the checkbox to show disabled stuff is not selected.

Disabled themes without any kind of subtheme or basetheme are not shown unless they have an enabled base ou sub theme with an update!

<-- these kind of summaries really help! :-)

The only question I have is if a subtheme should mention/link back to it's base theme if that base theme has security updates. I suspect the green of a subtheme's status could suss people into thinking they are still safe, whereas they are not because of the flaw in the base theme. Should we help people connect the dots and do something like this?

Keep in mind that where these are shown directly below eachother this wouldn't be the case if my hypothetical Zen subtheme was called, say, Applesque. I'm hesitant to add more 'needs this or that' messages, but since this page *is* about security, it would be good for UX too, to help people connect the dots.

@yoroy: That might be nice, but it's sort of an expensive pain in the butt to do that, and it's also very likely to break the string freeze in D6. :( We know that the subtheme depends on the update_test_basetheme theme, but we only know the "security update required" stuff on entire projects. Sometimes the project name matches the module or theme name, but not always. So, for any subtheme on the system, we basically have to iterate through a huge array of status info, searching for a project that says it includes the base theme, and then look up the status of that project, etc. Ugh. ;)

How important is this to you?

I'm not thrilled with this code, but it works, and it basically preserves the string freeze. It's definitely a bit more inefficient, though I haven't actually profiled or benchmarked it to see how much worse. Probably not a big deal, in the scheme of things. Here's the patch and a screenshot.

Bah, now without the debug() calls in the test cases. ;)

Hmm...using a local fresh install these tests have two failures and three exceptions for me:

SimpleXMLElement::__construct(): Entity: line 25: parser error : Extra content at the end of the document	Warning	update.fetch.inc	193	update_parse_xml()	Exception
SimpleXMLElement::__construct():
	Warning	update.fetch.inc	193	update_parse_xml()	Exception
SimpleXMLElement::__construct(): ^	Warning	update.fetch.inc	193	update_parse_xml()	Exception
Link to the Update test base theme project appears.	Other	update.test	357	UpdateTestContribCase->testUpdateShowDisabledThemes()	Fail
Link to the Update test subtheme project appears.	Other	update.test	358	UpdateTestContribCase->testUpdateShowDisabledThemes()	Fail

FWIW, all the update tests pass just fine when I run them locally on a clean HEAD install with either #43 or #37 applied...

Summary for committers...

7.x HEAD

Patch #43 (456088-43.basetheme-updates.D7.patch)

Has been tested by the bot (and me locally), and the code has been reviewed by me, JohnAlbin, and Dave Reid. It makes the Available updates report look like this:

That seems to be the preference of both yoroy (#40) and JohnAlbin (#47). I'm totally happy with that, too. Note, the parenthetical reason label only shows up for the "critical" warnings (security update, project was revoked, etc).

Patch #37 (456088-37.basetheme-updates.D7.patch)

Has been tested by the bot (and me locally), and the code has been reviewed by me and Dave Reid. It makes the Available updates report look like this:

In both cases, after applying the patch, you need to run these commands before committing:

6.x DRUPAL-6

Patch #33 (456088-33.basetheme-updates.D6.patch) is a clean backport of the functionality (and look) of patch #37 (without the extra warning labels). It does not break the D6 string freeze at all. It's been tested by me and Dave Reid, and is nearly identical code logic to patch #37, there are just changes in HEAD which aren't in D7 which made the patch not apply.

If we want to exactly backport the appearance of patch #43, that'll involve introducing a new theme function (mentioned by JohnAlbin in #47) and slightly break the string freeze, since there'd be a new string t('%base_theme (!base_label)', although there's no actual language in there -- it's just t() since I thought the order might make more sense if it's reversed in RTL languages. So, it's a *pretty* minor breaking of the string freeze. ;) @Gabor: if Dries/webchick decide to go with #43, you want that backported, and you're willing to accept this new t() string, let me know and I'm happy to reroll. Otherwise, #33 is RTBC for DRUPAL-6.

Thanks all!
-Derek

Yeah, I had something going wrong with testing yesterday. Works for me now and looks good.

@dww: I'm fine with the existing patch. Introducing new strings is less of a problem then modifying existing theme functions in the way you did for D7 (to reuse the theming of the message). So that is why I'd not like the more extended D6 change, not for the string addition.

Committed to CVS HEAD. Thanks.

FYI: Dries picked #43. Thanks!

@Gabor: #33 is RTBC for D6, since you didn't want the larger changes to the theme_update_report() function...

We shouldn't ship 6.15 without this...

Committed to Drupal 6, thank you.

I'm not sure if this is related or not, but I'm having a problem right now with an update to the Zen theme in my installation of D6.14:

Updates called for an update of Zen, which I did, but after updating it is STILL calling for an update.

I'm wondering if this is related to the sub-theme that I have for it. When Zen is updated, do I have to update anything in my sub-theme?

Probably your subtheme copied .info files from zen with data like:

project = zen
version = something stale

So, you're basically a victim of #166333: Visually display a warning if there are version mismatches within a single project

Newer versions of Zen solve this problem since the STARTERKIT.info file is now called STARTERKIT.info.txt, so the packaging script leaves it alone and it's not considered "part" of Zen by update status. Remove "project" from your subtheme .info file and you'll be a lot happier.

Finally get back to this...

Thanks dww, but just tried installing a completely fresh copy of Zen, and checked for 'project' in the sub-theme .info file, and its not to be found.

The problem persists - drupal 6.14 is telling me I need to update Zen in available updates, but it has been updated.

@saltspringer: Are you saying that when you check in the .info file of your sub-theme (which should not be inside of the Zen folder) there is nothing at the bottom?

I just had the same problem when updating to Zen 1.1 and I solved the issue by changing the value for "version" in the following bit of code (found at the bottom of my sub-theme's .info):

; Information added by drupal.org packaging script on 2009-02-13
version = "6.x-1.0"
core = "6.x"
project = "zen"
datestamp = "1234555897"

@dww: Should I be removing all of that info or just the line that says project="zen"?

@fei: especially the line that says "project=zen". The rest doesn't really harm you, and might help you know when you "forked" the STARTERKIT.info file.

.

I've just been able to get back at this, and it seems despite following advice to remove"

; Information added by drupal.org packaging script on 2009-02-13
version = "6.x-1.0"
core = "6.x"
project = "zen"
datestamp = "1234555897"

from the bottom of my sub-theme's info file, I still get the message that I need to update Zen.

As suggested, I first removed 'project = "zen" ', which didn't work.

I then tried changing ' version = "6.x-1.0" ' to ' version = "6.x-1.1", which didn't work either.

I then removed all of it, but that didn't work either.

Just did that, but no joy - still the same problem.

I've also tried cleared 'clearing cached data' under Admin > Site Configuration > Performance, but it hasn't helped.

@saltspringer and JohnAlbin: Can y'all kindly move this discussion to somewhere other than this closed bug report. ;) Thanks.

Sure, shall I post it as a new issue?