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:

subscribe

First attempt. This definitely shows updates for an infinite level of disabled base themes.

But, I need to add some code that shows why a non-active theme is being shown as having an update. An end-user may not realize why they need to update a theme that is non-active. I'm thinking something similar to "includes: " but for sub-themes. Maybe "base theme for: "? I'll roll a new patch within the next day to attempt that.

bah… I can't seem to get the patch attached right now. Networking issues. Will attempt later. Just wanted to let you know I have a working patch.

Letting the testbot validate this first attempt.

Ok, new patch now shows why an admin user would want to update a particular base theme. Underneath the "includes" line is a new "sub-themes" line which shows any enabled sub-theme for the base theme.

Patches for both D7 and D6.

@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

The last submitted patch failed testing.

Hmm… on D6, we could stick the sub-themes in with the “Includes:” text, but the project download doesn’t include the sub-theme.

But, I suppose, I could live with that inaccuracy if the string freeze breakage is a big deal; I'll defer to Gábor. As long as the D7 patch still uses the “Sub-themes:” text.

blarg. Testbot just changed the test result to failed. Hmm… when I run the tests on my system, I don't get any exceptions. Requesting re-test.

FYI, the re-test worked. This still needs review.

Adding tag.

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:

;) 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:

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?

Yeah, I think “disabled theme” is going to confuse people more. Yes, its not enabled, BUT all of its code is being run because its the base for an enabled theme. I fear people will see “Disabled theme” and think they don't need to worry about it since its disabled.

Ok. Just had a good conversation with dww and yoroy in IRC about this issue. We need to:

1. Change the "Sub-themes:" text to "Required by:" so that its clearer why a disabled theme is showing up on the available updates page.
2. Add enabled sub-themes to available updates page and text to show that a sub-theme "Depends on: [basetheme]"
3. Instead of calculating the basetheme/sub-theme ancestry inside _update_process_info_list(), we move that code to the .info cache building code in system.module. This will both improve performance for update module and help in other issues that need access to the theme ancestry data.

We're going to tackle #3 in #489762: Add theme lineage to .info cache (and prevent WSOD on admin/build/themes), so I'm marking this postponed until that issue is committed.

#489762: Add theme lineage to .info cache (and prevent WSOD on admin/build/themes) has been committed.

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?

Any progress on this one?

This is on the top of my to-do list. I was purposefully waiting until after code freeze to finish it up though to save Angie some sanity. See http://webchick.net/node/69 :-)

I'll roll a new patch this week.

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.

After #489762: Add theme lineage to .info cache (and prevent WSOD on admin/build/themes) landed, this patch is much more straight-forward to write.

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:

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:

I'm gonna hit the "Request re-test" link then while I review the patch.

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

First of all, I have to say I love bazaar for code developement! I have a feature branch were my patch in #25 is committed. I took #25 patch and reverse applied it and then applied the patch in #43. And now bzr is showing me the diffs between my code and dww’s. woot!

So, the modifications to the part of the code that I wrote are very minor (but crucial). I don’t doubt it took a while to figure out where to make those minor changes. They make total sense to me.

As for the things dww added. The modifications to the update status report look good. The addition of the theme_update_status_label() function makes sense since we need to call it from two places now and I agree that base themes with security issues (UPDATE_NOT_SECURE, UPDATE_REVOKED, UPDATE_NOT_SUPPORTED) should get parenthetical reasons for it in the sub-theme’s report line.

Testbot says all is good. And I just reviewed the code, so back to RTBC!

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:

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):

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

Just to be clear: Zen 6.x-1.0's starterkit theme had a bug-in-waiting.

That version of the starterkit had extra stuff added by the drupal.org packaging script. Namely these lines:

With those lines left in your sub-theme's .info file, Drupal gets confused and thinks that the version of Zen you have is exactly what it says in your sub-theme's .info file: zen 6.x-1.0. Your sub-theme's incorrect version number will override the actual version number specified in Zen's .info file.

If you remove those lines from your sub-theme's .info file, then Drupal will use the data in Zen's .info file. (And, yes, dww is correct; only the project = "zen" line is critical to remove.)

The starterkit theme in Zen 6.x-1.1 no longer has those troublesome lines added to it by the packaging script. But, obviously, that fact won't automatically remove those lines from sub-themes' built with the old starterkit.

In Drupal 6.15 (not yet released), you can leave Zen disabled and it will still check for updates of Zen since your sub-theme uses Zen as a base theme.
In Drupal 6.14 (the current version), you would have to enable Zen before Drupal will check for updates of Zen.