Unable to ignore module’s and base theme’s stylesheets

the .info file is cached in the database. Whenever you edit it, you have to go into the theme select page and maybe submit the form.

I tracked down the problem to function list_themes in theme.inc. It was checking if (file_exists($path)) before adding css to the theme. This is inconsistent with the handbook which specifies "Note that if you specify a CSS file in the .info file that does not exist, it will not be loaded, but neither will the file it is overriding" on http://drupal.org/node/171205. We should either commit this patch or remove that section of the handbook.

Here is my patch

That's my fault. That was never intended to work that way. When I wrote the docs I got the idea from a post from the original issue. Was never tested. All it was meant to do was to override the module style with an existing one.

Lynn, the only problem with the patch is that if the theme added stylesheet was not meant to override, it would still be included outputting a dead link. drupal_add_css() does not check in that situation.

I'll fix the docs, since this would be considered a feature at this point. Sorry guys.

And thanks Lynn for finding the culprit.

dvessel: I know that the "don't specify to remove" effect *did* work in the original version of the multi-stylesheet patch, because I specifically tested for it and confirmed that it worked. It was somewhat accidental but in the end intended behavior. I am not sure if it was ever tested for module CSS, though.

If that feature doesn't work for theme inheritance anymore, that's a regression from an earlier version of 6.x-dev and it should be fixed. I'd say it should work for modules, too, and if it doesn't that's a bug, not a feature request. Please don't edit the handbook, as the handbook is correct on the intended behavior.

Right, your comment was for the intended behavior of sub-themes and it does work. When I wrote the bit about module overrides I thought that would work too. I agree, it does make sense to just support that same behavior but I don't remember discussing it in that queue.

I already edited the handbook, but it can be reverted.

Changing status to won't fix since the handbook was changed and no one else weighed in.

Jody, no one commenting on an issue doesn't mean its “won’t fix”. See http://drupal.org/node/156119

Talked to Crell yesterday evening after getting bit by this bug. Apparently, you can't knock out a module's stylesheet or a base theme's stylesheet.

I need this fixed, so I'll start looking at it.

John, sub-themes canceling out base theme styles is supported and it should work. The issue was with themes canceling out *module* styles. Module overrides work but redefining it will only force it to use the themes version which must exist.

There is another issue of CSS aggregation causing the module style to be picked up even if the theme overrides it. Is it this bug your encountering?

http://drupal.org/node/189568

sub-themes canceling out base theme styles is supported and it should work

Right, it should work, but it doesn’t. I’m not using CSS aggregation because my D6 system is only a testbed anyway.

And I’m already following #189568: Overriding module CSS with theme CSS since that's the issue where some of the knockout code moved from drupal_add_css() to drupal_get_css(). (Very nice idea, btw, Joon!)

Steps to reproduce:

Create a rubyred/rubyred.info file with these contents:

name        = Ruby Red Slippers
description = Bug testing platform

core       = 6.x
base theme = garland

; try to knock-out the menu styling
stylesheets[all][]   = system-menus.css
; try to know out both of garland's stylesheets
stylesheets[all][] = style.css
stylesheets[print][] = print.css

Make the new theme the default theme. The following HTML snippet is displayed in the header.

<link type="text/css" rel="stylesheet" media="all" href="/modules/system/system-menus.css?U" />
<link type="text/css" rel="stylesheet" media="all" href="/themes/garland/style.css?U" />
<link type="text/css" rel="stylesheet" media="print" href="/themes/garland/print.css?U" />

And before any asks, yes, I know how to clear the theme registry. (I’m also using this patch: #239958: Clearing cache does not immediately reload theme's .info file Which helps.)

Damn! It wasn't long ago when it was working. I just tested and can confirm this bug.

Maybe Crell can comment. The feature of overriding the base style was a fortunate accident so I'm having trouble finding the chunk of code that made it happen.

I think it might be due to how the theme data is saved. The retrieving part has changed a lot but it doesn't look related but there's a very good chance that I'm completely off.

For anyone who wants to jump in. John, anyone else.. My time here is spotty so I'll have to look on and off.

Save:
system_theme_data & _system_theme_data.

Retrieve:
init_theme > list_themes

The other option is to forget how the side-effect occurred and supply a new approach designed explicitly for this purpose. I'd go for this. Clean up Lynn's patch and take it from there.

Changing title/status

Joon, does this mean you were able to get a theme to ignore a module’s stylesheet? My test in #13 showed I couldn’t knock out the system-menus.css.

Oh no.. that was the original point of this thread but it was never designed to do that. I'm looking at it in terms of getting the base style ignored since that was there already but that wasn't designed in either so I guess it could be argued to get them both inline with the same behavior. It's confusing as it is.

If you want to take this on and enable the same behavior for modules and base styles I would support it. It is just that this thread died off prematurely. I would have been happy to revert the docs if this got in.

My understanding of how it worked originally is this: The theme registry build process looks for file references in the .info files, and makes overrides as necessary, ignoring whether or not a file actually exists. It then saves that stylesheet array. When queuing those files up on each page request, files that do not exist are not added but are skipped. At least, that's how it behaved when I wrote the original patch. I presumed that was a feature of the new theme system. If it isn't, or if that was somehow reverted, then that is a bug that we should just go ahead and fix.

I'll see if I can find time tomorrow to look into this, but it's a holiday weekend so I'm not certain if I can.

subscribe

The theme registry build process looks for file references in the .info files, and makes overrides as necessary, ignoring whether or not a file actually exists. It then saves that stylesheet array. When queuing those files up on each page request, files that do not exist are not added but are skipped. At least, that's how it behaved when I wrote the original patch. I presumed that was a feature of the new theme system. If it isn't, or if that was somehow reverted, then that is a bug that we should just go ahead and fix.

Here's a small patch against HEAD that allows you to knockout a module's CSS file by specifying it in your theme's .info file (although the CSS file doesn't exist in your theme folder). This will completely skip the inclusion of the module's CSS and not just replacing it. This is how it's intended to work (as Crell mentioned above), and shall be considered a feature.

What the patch actually does is that it moves the checking for existence from list_themes() to drupal_get_css(), after where the overriding takes place. list_themes() should only take care of theme listing, imho.

The patch still needs some code style and documentation corrections. But I attach the file for a technical review.

And no, this is not a duplicate of #189568: Overriding module CSS with theme CSS as that only fixed overriding with files that exists in your theme folder.

Here is a new patch that also applies to coding standards. I've tested it and it works perfectly against the D6 branch.

Almost applies to coding standards. I did notice that "+ // Only include the stylesheet if it exists" could use a period. Comments should be complete sentences, capitalized, ending in periods.

Yes, got that! Here's a new patch with the missing period ;)

Patch applies and works as advertised. Tried several combinations of stylesheets and all worked as expected. Should be ready to go.

Shouldn't this be applied to 7.x first?

I think bug reports for the DRUPAL-6 branch has priority. We then apply the patch to the DRUPAL-7 branch when this has been fixed.

If the bug is present in D7, it has to be fixed there first, and only then backported.

Also, I don't see why this is critical.

patch needs to be re-rolled for 7.x:

Hunk #1 FAILED at 1716.
1 out of 1 hunk FAILED -- saving rejects to file includes/common.inc.rej

If the bug is present in D7, it has to be fixed there first, and only then backported.

Also, I don't see why this is critical.

Okey, my bad. I'm new to the process here :) But I really think the bug is critical because this is a key feature in the new theme layer that isn't working.

The bug seems to be present in 7.x. So here is a patch that is re-rolled and now applies to HEAD.

And this applies to head and works as advertised as well.

I've committed this to CVS HEAD. Doesn't break any tests, but could use some tests IMO. Making an exception for it.

We should be able to commit #24 to 6.x now

Tests that involve the presence of files on disk are not really feasible at the moment, AFAIK. (If they are, someone please correct me.) Given that a fair bit of behavior happens that way, that's a gap in testing we'll need to sort out sooner or later.

Yes indeed, heyrocker.

The patch in comment #24 is tested and should be ready to go in with the DRUPAL-6 branch.

OK, I have committed #24 to Drupal 6. I'd like to add that file_exists() each drupal_add_css() call is not exactly a cheap solution (in terms of performance). Especially since most of the time we know the file exists. I'd suggest coming up with a cached (registry based?) solution for Drupal 7 to avoid calling out to the file system with each CSS file added.

Granted high traffic sites will just turn on their CSS aggregation, but that still does at least one file_exists() check on a file we know is there. Maybe opening a new issue to performance optimize this would be better. Or reopen this one :)

Thanks for the commit, Gábor! And thanks for the patch, Dixon! :-D

And I would like to point out that the patch only moves a file_exists() from list_themes() to drupal_get_css() (not drupal_add_css). So I don't think there is any change in performance since drupal_get_css() is minimally called.

The committed patch is preventing the cascading of base theme styles when the active sub-theme does not have a override.

It was brought up here: http://drupal.org/node/281489. The issue is described better there.

The changes:

Index: includes/theme.inc
===================================================================
RCS file: /cvs/drupal/drupal/includes/theme.inc,v
retrieving revision 1.428
diff -u -p -r1.428 theme.inc
--- includes/theme.inc	25 Jun 2008 09:12:24 -0000	1.428
+++ includes/theme.inc	1 Jul 2008 15:12:20 -0000
@@ -449,9 +449,7 @@ function list_themes($refresh = FALSE) {
     foreach ($themes as $theme) {
       foreach ($theme->info['stylesheets'] as $media => $stylesheets) {
         foreach ($stylesheets as $stylesheet => $path) {
-          if (file_exists($path)) {
-            $theme->stylesheets[$media][$stylesheet] = $path;
-          }
+          $theme->stylesheets[$media][$stylesheet] = $path;
         }
       }
       foreach ($theme->info['scripts'] as $script => $path) {

I checked the styles that are attached when the theme system initializes. The sub-theme, even when it doesn't have an override adds its own "style.css" which knocks out the base themes style sheet.

As for a fix, the only thing I can think of is to not include a "style.css" by default. Let the theme declare it explicitly. The other is to roll this back.

I never liked this duel behavior of adding the .info entry to either *add* or *override* but it seemed like the easiest approach at the time.

That approach was taken mostly because we got it for free, with no additional syntax in the info file. :-) I'm open to a better mechanism in D7.

For now though, hrm. Changing the default to be "no stylesheets" rather than style.css would be very easy to do. That's a one line change. Whether or not we can get away with that now that D6 is released, I don't know. That's a question for Dries and Gabor.

I consider it an acceptable change at this point as my themes all specify a stylesheet explicitly anyway as I consider that a best practice. Dries and Gabor may disagree, however. :-)

Yeah, I'd guess removing style.css as a default would do more harm than good. I don't think Dries or Gábor would agree with that. :)

Looking at this further, the bug I mentioned would only be an issue if the sub-theme included no styles at all. This is due to the fact that defining a style sheet automatically wipes any .info defaults (style.css in this case). A sub-theme with no "stylesheets" entries in .info seems unlikely in most situations.

If anyone thinks this isn't a problem, feel free to close this. I have tried to work around it but it targets "style.css" and it reveals another problem of not being able to suppress style.css when you really want to.

Well, it depends how many D6 themes do not define a stylesheet and rely on the default.

A workaround for the style.css issue is to just define your CSS like this:

stylesheets[all][] = fake.css

That will not trigger the default style.css, but will not add a CSS file either, since non-existent stylesheets will not get added.

Right, so I think this will work for now.

docs have been updated. http://drupal.org/node/171209

This is quite humorous. I'm sitting here this evening working through Chapter 3, The Theme System of Matt Butcher's book "Learning Drupal 6 Module Development" and discovered that the patch for this problem breaks the theme tutorial in the book. I only have to wonder if it also "breaks" any other books. I'm sure this is unintended!

I'm using Drupal 6.3 and had to unpatch theme.inc in order for the sub-theme to correctly inherit the files from the base theme (and thus continue through Chapter 3). You can go to Packt Publishing's website and download the code for the sub-theme in the book and see for yourself how it doesn't work.

Respectfully, the patch for this bug needs review and further discussion.

@Sborsody, see #43 on the fake entry. I can't think of a easy way to have it both ways. It previously caused other problems but I think this is the lesser of two evils.

I guess I'm not understanding the issue correctly (which is very likely). Here is how I see it. The person who started this bug report said they couldn't override system-menus.css by specifying it in the .info file. The person needed to specify it _and_ create a blank system-menus.css file in order to override the one from the system module. What's wrong with doing that? IMHO making the blank css file is the lesser evil to the mess of breaking subtheme inheritance.

And as it turns out, it's awfully hard to patch a book once it's been released!

I'll post an erratum on the Packt site for the book. At least I have an excuse for a second edition, now.

I'm coming over from Sub Themes do not inherit CSS and images, since it says there it was a duplicate of this issue.

I'm using Drupal 6.4, and subtheme inheritance is still broken: if you add a my_subtheme folder in sites/all/themes, with only a my_subtheme.info and a template.php file in it, the subtheme does not get the parent css and images at all. For reference, here's my .info file.

name = Customized Garland
description = "Customized Garland theme."
core = 6.x
base theme = garland

Why was this issue marked fixed when it's not? Am I missing something?

If you don't specify a stylesheet at all, it gets a default of stylesheets[all][] = styles.css. That knocks out the garland parent stylesheet. This is a known bug but we can't fix it in D6 without changing the API. :-(

To fix it, add this line to your theme's info file:

stylesheets[all][] = fake.css

And then do not have such a file. Then all works as it should.

Also, please do NOT open issues that are closed. Open a new issue. Opening closed issues just makes things harder to keep track of. Thanks.

Thanks Crell for the tip, and sorry for reopening the issue, I believed it was worst to create a duplicate issue than to reopen an old one.