Add dependency checking for themes (before enabling)

[indigoxela]

Description of the need / bug

The module installer does a dependency check and even also downloads required modules for the selected one.

There's no such check for themes.
And it's also possible to enable a theme and set it as default if the base theme is missing, which can lead to all sorts of warnings.

Actually, at first I thought, this should be a feature request, but having a closer look: the ability to enable a sub theme if the base theme isn't there is a bug. It causes loads of PHP warnings.

Steps to reproduce

1. Get a subtheme, but do not download its base theme
2. Go to admin/appearance
3. Enable and set that "orphan" as default
4. Go to the home page (or any other page, where the default theme applies)

Besides a broken display (depends on theme CSS), there are also loads of warnings. Either displayed, or logged.

Proposed solution

If possible, also download the base theme when downloading a depending theme via installer (UI). That seems more like a feature request.

At the very least, prevent enabling a sub theme if the base theme's missing. 👈 This is a bugfix.

Alternatives that have been considered

People have to check the theme's README or info file, to figure out, what they need.

Additional information

That's what you see on the home page if the base theme isn't there and error display is turned on. That's massive.

Backdrop version: latest dev (the problem isn't new, though)
PHP version: 8.3 on that testing site.

My suspicion is, that with PHP 8.4 this might even end with a hard error. It doesn't, at least not in usual setups.

[indigoxela]

Here's now a PR with the minimal solution: it prevents enabling the "orphan" sub theme if the base theme isn't there. Only via UI on admin/appearance. (Ignore the silly theme names.)

Instead there's an error message:

IMO more helpful than a massive wall of warnings on every page (or a filled dblog). 😉

[stpaultim]

For what it's worth. This is a related issue, although it does not specifically address the bug here.

#5316 Allow themes to declare dependencies on modules and layout templates

We should definitely fix this bug first, but keep some of the bigger issues around themes and dependencies in mind to address later.

[indigoxela]

@stpaultim I belief, the other issue's only loosely related. The dependency here is the one to the base theme - that's available info, it's just not evaluated where it should be.

The other issue (dependency on modules or layouts (or even core version) is valid, too, but IMO goes beyond what we have to fix here.

Even this issue here actually addresses two different problems. I'd suggest to start simple (prevent breaking the site) and evolve from there (possibly automatic base theme download...).

[argiepiano]

I think this is a good fix for a very specific problem. It makes sense that, if a theme has a base theme, Backdrop should check if it exists. The base theme is an implicit dependency, and while the other issue pointed by @stpaultim could possibly also be used to solve this problem, I believe this is a simpler and better solution for this specific situation, and is also backward compatible, as it would apply to existing subthemes without the need of modifying them.

@indigoxela, a suggestion to consider: we could modify system_theme_page() where the links to enable etc are shown, and check (in lines near 194) if the theme listed in $theme->info['base theme'] exists in the $themes array before deciding to show the links (or perhaps show something like "This theme can't be enable, as its base theme is missing").

[indigoxela]

we could modify ... before deciding to show the links (or perhaps show something like "This theme can't be enable, as its base theme is missing").

That makes sense. Why showing a link, when we know it won't happen... 👍

I'm also considering to catch some of the warnings shown in my screenshot - because a theme could be enabled and for whatever reason the base theme gets deleted. In case this would really end up with a hard error with PHP 8.4 it seems like a good idea to nag into dblog (but not too much), but still prevent to break the site.

@argiepiano what do you think? Does it make sense to catch more problems, even if not caused by UI clicking?

[indigoxela]

New approach: backdrop/backdrop#5004

Most things happen on admin/appearance (error / warning message, useless links removed), but there are some attempts to prevent the "wall of warnings" altogether. It's probably not possible to catch everything, but at least the nagging should be reduced to something that helps to quickly fix.

Testing has to happen locally. I tested with a stub theme named "Orphan":

type = theme
name = Orphan
base theme = not_there
backdrop = 1.x

As it's no longer possible to enable a sub theme with missing dependency via UI, first comment the dependency:

type = theme
name = Orphan
;base theme = not_there
backdrop = 1.x

Then set it as default and uncomment again. Visit some pages and then see what's showing up in dblog (admin/reports/dblog).

Then fix the problem on admin/appearance by following the instructions.

Edit: good news, BTW, this doesn't cause a hard error on PHP 8.4.

[indigoxela]

Here's a screenshot of admin/appearance after uncommenting the "base theme" in the info file:

[argiepiano]

@indigoxela thanks for the additional work. I've tested and it works for me (haven't had time to do a code review).

1. I installed Shaperrific without bedrock
2. Checked that it can't be enabled 👍🏽 The message, however, could use a bit more info (see below)
3. Downloaded bedrock. shaperrific can now be enabled and set as default 👍🏽
4. Removed bedrock from the folder
5. Warning is shown as expected ("Shaperrific is a sub theme of bedrock, but its base theme is missing. Set another theme as default theme or download the base theme.")
6. I set another theme as default, and got the message: "Shaperrific is a sub theme of bedrock, but its base theme is missing. Either download the base theme or disable it." 👍🏽

I have 2 suggestions:

Regarding 2 above, this is what I saw when the parent theme was missing:

I first missed the "(missing dependency)" next to the name - I was looking for a link to enable, and it was a bit confusing not to see it... until I saw the missing dependency message next to the title. My suggestion would be to provide a more visible message, and include the name of the parent theme, as in:

The second suggestion: while the word "base theme" in the warnings seems obvious to someone familiar with the code, I wonder if perhaps "parent theme" may make more sense to a site builder?

[indigoxela]

Many thanks for testing @argiepiano 🙏

I first missed the "(missing dependency)" next to the name...

Yes, there's room for improvement. However, showing the info below requires a bigger code change. But it might make sense.

...perhaps "parent theme" may make more sense to a site builder?

I agree, that's clearer from site builder perspective. On the other hand we get some inconsistency in wording. What's more important? Consistency or comprehensibility?

[indigoxela]

FWIW... why don't we simply display the dependency. 😁

Latest changes, and with a dependency tree:

Basis -> Orphan -> Other one (which works):

A missing parent -> Orphan -> Other one (which won't work):

For now I stick with the term "base theme", as discussions re renaming something (or wording in general) tend to get stuck quickly. 😬

And I didn't use red color - for good reasons 😉

[yorkshire-pudding]

@indigoxela - thank you
I have tested this and it works well - I've never come across the issue before but the bug is ugly when it happens.
I've reviewed the code and it all makes sense; I have suggested a minor wording change on a comment for clarity.

[indigoxela]

@yorkshire-pudding many thanks for having a look. Wording change committed. 👍

I've never come across the issue before but the bug is ugly when it happens.

Yes, definitely ugly, and especially puzzling for newcomers, who never considered a theme to have a base theme (dependency).