Move discovery of theme information to .info files (and improve theme inheritance)

A lot of that theme registry code could use some inline comments. It was a bit bare before, but after this patch it's even more opaque. Just a couple of words here and there to say what the code is doing can help a lot.

Certainly; it should be no trouble to go back and add some more.

How exactly does the new inheritance work? Do info fields from the sub-theme override those of the parent (seems not, judging from the code), or is it something at the registry or phptemplate level? Can a subtheme have a different engine for example? I imagine there might be a problem with having to call both phptemplate_variables() and smarty_variables() for example.

As you point out later, it is single level inheritance, and a sub-theme inherits the engine or .theme file of the base theme; it thus inherits all of its templates. The stylesheet is directly replaced by the sub-theme's sheet, so that is not inherited. I don't think a sub-theme could possibly have a different engine, that would get really, really ugly.

What really happens is that the theme info is loaded; if the .info file claims it is has a base theme, it looks up the info for the base theme copies over the engine (owner) to so that is kept. When initializing the theme, base theme is checked and, if it exists, *both* themes are initialized, and two extra steps are placed into the hook registry to discover themes (with the sub-theme's hooks having the highest priority).

Theme inheritance is a one-level thing now, i.e. you cannot inherit from an inherited theme. I could be wrong, but it seems multiple inheritance would just come down to trying a couple more foo_ function prefixes or foo/ paths. It is not inconceivable that with a base template like Zen, one theme (which inherits from Zen) is overridden by another theme (think layout variations like Garland and Minnelli).

I thought this to, and then I ran into path_to_theme(). Theme functions need to be told their path, which means you need to know what theme is currently 'active'. Hmm. Though, actually, the hook registry could store that path. Right now I added an argument to path_to_theme(), but actually, if I fix that, multiple inheritance *could* be possible. I'll look more deeply into this. Apparently (lack of) sleep may've found the solution there.

It makes sense to change the stylesheet value to an array IMO. That way, themes can have multiple stylesheets (and the default value is appropriate for most themes anyway, so most themers don't deal with the syntax). And to be really nice, you could accept both an array (multiple files) and a string (single file). Be flexible in what you accept, and all that.

My original notes called for this, and then I didn't do it -- the reason is that sub-themes replace the basic stylesheet. But if you have more than one stylesheet in a parent theme, chances are very good that additional stylesheets should not be replaced, but instead be added to. I therefore decided to leave this as an additional feature that needs a little more thinking to do right. Especially if we go the multiple inheritance route, we have to think about the implications of stylesheets stacking up, and how we decide if a stylesheet is added or isn't.

For themes to work without databases, you would need to parse the info file of both the theme and its base theme for that request, because right now this information comes from the system table. The parser was never built for speed (although it's regexp based, so still relatively fast), and currently only repopulates that table when you change modules or themes.

That's correct. While this conversation goes elsewhere, my thinking was something like this: settings.php or the .profile could set $conf['maintenance_theme'] (or a global, either way) to the real path of the .info file. drupal_maintenance_theme could then read in that .info file, run it through basic processing (possibly splitting out some of what's in system.module now and moving it to theme.inc) which should come up with enough data to pass to _init_theme(). Then, we could change theme_maintenance_page to theme('maintenance_page') and, with the OTHER patch I have, that function that sets all the variables fairly cleanly translates to theme_variables_maintenance_page(). There are only a few other theme functions -- I think doing this properly would require going through the maintenance process and evaluating what themes are used, and documenting that any implementations of those themes needs to conform to the broken API spec, meaning they may be expected to run when there is no database and only a small portion of the Drupal API loaded. I was originally wanting to push that stuff back through theme('page') but that now seems like a bad idea; theme('maintenance_page') gives us maintenance_page.tpl which is basically what we want.

Oh and bug: color.module needs to be updated to not hardcode the stylesheet filename(s).

I really need to actually look at that module. Will fix this in the next re-roll. Will also do a grep and make sure there aren't other instances (though I doubt there are).

For a minute or so, I was confused by the variable name 'owner'. I initially thought you meant the author of the theme. Maybe there is a better name for it, I don't know. Maybe use info_file and theme_file instead of filename and owner? Not sure ... Doesn't matter much, but if we can prevent author people from being confused for a while, that would help.

->owner is what it is called in the database at this time. For the purpose I'm using it, it is not the most descriptive name, but it's already named. I have the option of changing this name in the db and thus all occurances of it, changing it mid-stream (-1 to that) or leaving it and providing some documentation. I'm open to fully changing it or leaving it.

It probably helps newbies if you explain what the 'base theme' is. Right now, there is no context for them to understand what it means. Add a bit more documentation (and the bigger picture like you did in the issue), and things will become easier to grok.

For the most part, the real meat of that documentation needs to go wherever we are documenting .info files, which isn't the code, but I will look and see if there is a decent place to put some more documentation about what's going on in this file here in the code. As we said earlier, more inline doc-ing can't hurt.

Here's a new version of the patch.

1) It provides true theme cascading. At this point, 'base theme' may not be the right term. But a theme may have any number of ancestors. path_to_theme() is smart -- it provides the path to the theme for the hook that is currently active (set by theme()) or to the regular theme if called from outside of a theme() call. As a side effect of this, *modules* can use path_to_theme() in their .tpl.php files and get the module's directory.

2) I found a couple of bugs (yay) while doing this.

3) I expanded on the inline documentation. Documentation for theme cascading needs to largely go elsewhere; I couldn't find a really appropriate place to put it here. It's going to deserve a handbook page all its own to discuss the implications of having derivative themes of derivative themes.

4) I left the 'stylesheet' directive singular; in part because I think that feature needs a lot of thinking, especially for how it interacts with the cascading and it may well deserve to be its own thing in a separate patch. It could have some interesting and long code paths, all on its own.

5) This patch is going to collide slightly with http://drupal.org/node/137236 -- given the positive response to that patch, is there any objection to combining them? If nothing else, that patch affects the cascading, and the base_theme stuff has an affect on how *_variable_HOOK functions are called. I'd separated them, thinking that the functionality was different enough it wouldn't matter; also, because I've got a clock ticking.

Here's a new version of the patch.

1) It provides true theme cascading. At this point, 'base theme' may not be the right term. But a theme may have any number of ancestors. path_to_theme() is smart -- it provides the path to the theme for the hook that is currently active (set by theme()) or to the regular theme if called from outside of a theme() call. As a side effect of this, *modules* can use path_to_theme() in their .tpl.php files and get the module's directory.

2) I found a couple of bugs (yay) while doing this.

3) I expanded on the inline documentation. Documentation for theme cascading needs to largely go elsewhere; I couldn't find a really appropriate place to put it here. It's going to deserve a handbook page all its own to discuss the implications of having derivative themes of derivative themes.

4) I left the 'stylesheet' directive singular; in part because I think that feature needs a lot of thinking, especially for how it interacts with the cascading and it may well deserve to be its own thing in a separate patch. It could have some interesting and long code paths, all on its own.

5) This patch is going to collide slightly with http://drupal.org/node/137236 -- given the positive response to that patch, is there any objection to combining them? If nothing else, that patch affects the cascading, and the base_theme stuff has an affect on how *_variable_HOOK functions are called. I'd separated them, thinking that the functionality was different enough it wouldn't matter; also, because I've got a clock ticking.

Slightly updated patch: Based on something Adrian said, I realized that it is not necessary to require a child theme to have its own style.css. I removed that requirement; now the first parent in the chain to have a stylesheet gets it. (this still leaves questions about how to deal with multiple stylesheets, which I think is important, aside).

Side effect of this: One could create multiple versions of Garland with their own color settings just by creating a .info file. You would still need it in a separate directory, but you could put a ton of them in a directory with nothing else in it (but beware mixing in templates/stylesheets, since they'd all then share them).

This patch should fix the installation issues.

I looked at adding system_theme_data() to the updates, but the last update (that just went in a couple of days ago) runs it, and it seems silly to run it again since it will affect only people who update between the .info patch going in...and this one. (However long that may be).

If people deem it necessary to add another update I will do it, but at this point I don't think it's necessary.

This fixes the bug with color.module hardcoding style.css. It is not an ideal fix, I don't think; but I also think color.module is going to need to undergo a bit of work to make it support cascading themes nicely, and I know Steven has some ideas for improving color.module anyway. So I don't think I want to touch that beyond retaining minimal functionality. I will say though, that to take proper advantage of this, color.module is going to need to store color information in the database, not in variables, because I can see a use case where every user gets their own variant of the theme which is just a .info file.

I also added 'screenshot' as something that can be set in the .info file.

Whoops, you're right; I didn't take out the file_exists; since $screenshot will be NULL if one was never found, that's unnecessary. Good catch.

n system_theme_data(), it seems to me the snippet for inheriting the 'owner' assumes that subthemes are processed from least inherited to most inherited.

No, the $base_key in that snippet is already the most-distant theme, and it doesn't care what came in between. If the theme hierachy is a -> b -> c then $base_key will be c; if the theme hierarchy is a -> c -> b then $base-key will be b -- and what matter is that $base_key will always point to a theme that *doesn't* have a higher dependency.

The comment is incorrect and persists from the single-level hierarchy. In the new scheme, the .owner file isn't copied around at all for non-engine themes, and engine themes are required to copy the owner file from the top level parent (so regardless of the processing order, they'll all end up with the same owner). In fact, that those two ifs can be reduced to one now that the logic there is simpler.

Attached patch fixes the above mentioned problems, and it also restores theme_menu_local_tasks which disappeared somewhere in the menu work and has not yet re-appeared, but is still needed.

No really. Patch attached. :P

This patch is rerolled for updates to HEAD.

This patch still has problems with color.module -- but otherwise, I think all we need are some testing reports.

riccardoR is correct.

Patch rerolled with his commentary.

Ok, I believe this is RTBC, but I need Steven's opinion on that -- this essentially breaks color.module, but I know color.module was a key motivator for .info files in the first place, so I'm highly tempted to let color module break. (it works ok for base themes, so Garland was fine the last time I looked, but it breaks completely for sub-themes because of the assumptions it makes).

I think all it really needs is to be able to inherit color information properly rather than the assumptions it makes now; and since all of the assumptions it makes now are likely to change because they'll be in the .info file, I'm disinclined to fix those assumptions only to have that code completely removed. But in the end, that is Steven's call.

Ok, I spoke with Steven, and he is not sure color.module 2 is going to make it into Drupal 6 (sniff). Also, what I thought was wrong that was color.module turned out not to be. After discussing what's really happening, it seemed like it was probably pretty simple -- and it was.

This patch makes color.module work for Minelli again; however, color.module spews a whole ton of notices right now. This happens with or without this patch, but I want people to be aware of it, lest it gets tested and people go "Hey!"

With that in mind, I think this patch is ready to go. It's seen a fair bit of testing -- possibly not really *enough* but if we get it in now, we're more likely to see the problems. Once again, this patch is going to come with a big documentation need, but that's almost completely for the handbooks.