Theme's can no longer remove stylesheets in .info files by creating an entry for a file that doesn't exist in the theme. I think we need to fix this and I think it's critical because:

  1. Each attempt to do this results in a 404, which adds about ~100ms to the page load per file
  2. It would be wrong to expect novice themers to implement hook_css_alter() just to remove a file, when doing this was so simple in Drupal 6.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jacine’s picture

Here's a video demonstrating the problem: http://bit.ly/bdyUQX

tim.plunkett’s picture

Ouch. I'm guessing this was an unintended consequence, since it wasn't documented anywhere that I can see. Subscribing.

ygerasimov’s picture

Status: Active » Closed (works as designed)

I believe it is nicer to keep the altering of css files in one place (in hook_css_alter()) and not make a back compatibility in this case. This helps to narrow the problem if something goes wrong with wrong loading css files.

Regarding novice themers, the approach should be used is to see example default theme (for example seven, where this hook implemented). So it shouldn't be a problem. I have added a note to http://drupal.org/node/263967.

tim.plunkett’s picture

Status: Closed (works as designed) » Active

First, you spelled href wrong in the doc, so that link doesn't work.
Second, I think that this regression adds a major barrier to themers that aren't comfortable with PHP. template.php can be a scary place for beginners, and allow them to override stylesheets in the .info file was really handy.
Third, because of the lack of documentation, I can only assume this was unintended, which warrants some discussion before a "won't fix".

seutje’s picture

simply amending it to the documentation doesn't rly work, as it now kinda contradicts itself...

also, the example referenced in the documentation only shows how to swap out a stylesheet, not how to remove it entirely

I don't rly see any benefit in removing this functionality aside from the ability to add a bunch of stylesheets with the same name, which is not a road we want to push new people down imho

but uhm, what's with the "Needs design review" tag? there's no visual change here of any kind o.O

seutje’s picture

Issue tags: -Needs design review

people following the "Needs design review" tag asked me to remove it, as they don't want to subscribe to do so

Jacine’s picture

@seutje Good to know. Thanks :)

Mark Trapp’s picture

Isn't the current functionality unintuitive and counterproductive? Everywhere else, adding entries to the .info file indicates to Drupal that you want to add a file. So, it would stand to reason that adding the following line:

stylesheets[all][] = style.css

would indicate to Drupal that you wanted to add that style.css to the list of loaded style sheets, and if it's not in your theme it's a mistake, so throwing a 404 is the right thing to do.

With hook_css_alter() removing a CSS file is explicit, and not taking advantage of a quirk of the theming system.

As to not expecting themers to implement hook_css_alter() if they want to modify the loaded CSS files, what's the reasoning behind this? Drupal expects themers to use PHP to override almost everything else in a theme.

Jacine’s picture

The logic is that this is how it worked in Drupal 6, and we (themers/designers) like it because it's easy. This is a very widely used and recommended technique for removing CSS files.

Themes get access to all alter hooks in Drupal 7, not just hook_css_alter(). Drupal does not expect novice themers to use PHP to override almost everything. The number of template files grows with every release and great effort goes into trying to make things easier for those don't want to use PHP.

If developers don't care about having decent functionality in .info files, I suggest we retire the whole idea of dealing with stylesheets in .info files entirely. However, it seems that is unlikely since modules are actually moving towards doing the same thing: #328357: Allow module .info files to add CSS/JS.

marcvangend’s picture

I use this feature all the time. In a sub theme, it's a good and easy way to disable style sheets from the base theme. It is definitely a regression and it has been documented at http://drupal.org/node/263967 since December 1st 2008:

Adding the override for a style sheet that does not exist inside the theme will omit the core or module style sheet. This is by design and this behavior has been corrected since the release of 6.0 in 6.3.

This behavior was also documented in the STARTERKIT.info of the Zen Starterkit in the 1.x branch. (For some reason this addition was never ported to the 2.x branch.)

  ; To prevent stylesheets of a base theme or of a module from being included in
  ; our sub-theme, we specify it in our .info file (and we don't need to have a
  ; stylesheet in our sub-theme with that name.) For example, we prevent the
  ; zen.css file in the base theme from being included by specifying it here.

In other words: let's fix this.

Mark Trapp’s picture

Edit: this used to contain (incorrect) information about the issue that introduced this functionality. Still looking to see where it was introduced.

marcvangend’s picture

@Mark Trapp: there used to be a check if the css file exists in drupal_get_css():

        // Only include the stylesheet if it exists.
        elseif (file_exists($data)) {

In current D7, it's not there anymore. That change was made in revision 1.949 of common.inc (http://drupalcode.org/viewvc/drupal/drupal/includes/common.inc?revision=...). The patch is in #92877-55: Add numeric weight to drupal_add_css.

ygerasimov’s picture

Status: Active » Needs review
FileSize
799 bytes

We can put the check of file existance back. Please review the patch.

Jeff Burnz’s picture

Status: Needs review » Active

Sub

Jeff Burnz’s picture

Status: Active » Needs review
sun’s picture

Status: Needs review » Active

Frankly, I'd prefer a solid separate stylesheets-remove[] .info property vs. that wack handling of unknown stylesheets[]

marcvangend’s picture

Sun, I agree that stylesheets-remove[] makes sense, but wouldn't that be considered an API change? I doubt if we can/should still do something like that.

IMO, the good thing about using stylesheets[] is the linearity. A core style sheet can be overridden by a theme with a non existing file (thus removing it) which can in its turn be overridden by a sub theme. That's a quite effective way of doing things because they're all working with the same property.
So, what if a base theme would remove node.css with stylesheets-remove[all][] = node.css while its sub theme adds it again? The sub theme would not automatically override the removal of the stylesheet in the base theme, so I guess we more code logic to get that right.

sun’s picture

Point being that we're removing yet another instance of file_exists() in JS/CSS handling over in #575298: Provide non-PHP way to reliably override CSS

Those deeply hidden and automated file_exists() are a recipe for poorly written code. You never know whether the .info properties you are doing are still correct or totally broken, because there literally never appears an error message, because we hide it from you. As if we'd say that themers are not eligible to see and know whether they made a mistake, because they don't understand it anyway. WTF?

ygerasimov’s picture

Regarding introducing stylesheets-remove[], what should have priority stylesheets-remove[] or stylesheets[]? Just in case someone will add both options in the same file?

Jeff Burnz’s picture

#17 since its a regression rather than going back can't we move forward? webchick?

#18 I would concur with Suns findings - I have on one or two occasions experienced this wtf.

#19 looks like hand holding, but one assumes the order in which they come dictates priority?

marcvangend’s picture

Re #18: OK, I never had that myself, but I can see how file_exists can cause a WTF. Returning a 404 isn't very verbose either, but it should be enough.

Re #19: I would say that stylesheets[] should have priority over stylesheets-remove[]. In a (sub) theme's .info file, stylesheets-remove[] is about removing a stylesheet from upstream (Drupal core or a parent theme) while stylesheets[] is about defining your own. Removing your own stylesheets makes no sense.

Re #20: Sure, I'd also rather improve than only prevent regression, but only if it has a chance of getting in.

sun’s picture

Of course, stylesheets-remove[] has precedence. We simply skip the entire file and file handling if we know that we don't need the file at all.

Thus, it's totally easy and intuitive: if you explicitly the property, then it is applied. If you don't set it, then it is not applied.

The property should only be supported for themes.

Jarek Foksa’s picture

+1 for solving this issue by adding stylesheets-remove[] property to the info file.

Personally, I find the old behavior to be very confusing - disabling stylesheets by enabling them in info file does not make much sense.

ygerasimov’s picture

Status: Active » Needs review
FileSize
2.05 KB

Please review the patch.

sun’s picture

+++ includes/theme.inc	3 Sep 2010 17:09:30 -0000
@@ -604,6 +625,13 @@ function list_themes($refresh = FALSE) {
+        foreach ($theme->info['stylesheets-remove'] as $media => $stylesheets) {

Is there a point in differing by media? I don't think so, but I may be missing something.

Powered by Dreditor.

marcvangend’s picture

I guess differing by media should not be needed, but if you have $media available it's easier to unset $final_stylesheets[$media][$name], right?

pfrenssen’s picture

Status: Needs review » Needs work

Review of patch #24:

_drupal_theme_initialize():

  • The stylesheets-remove[] are removed before adding the stylesheets[]. Isn't it better to do this the other way around, like proposed by sun in #22?

list_themes():

  • The $theme->info[stylesheets_remove][] array is not consistent with $theme->info[stylesheets][], which has the name of the stylesheet as key, and the path to the stylesheet as value. This is set in _system_rebuild_theme_data().
  • Typos: $theme->info['stylesheets-remove'] instead of $theme->info['stylesheets_remove'].

Some other thoughts:

  • In this patch the current behaviour of generating 404's for missing stylesheets stays intact, which I think is indeed better than the way it was done in D6 as it facilitates debugging.
  • stylesheets-remove[] or stylesheets_remove[]?
sun’s picture

stylesheets-remove[] it is (hyphen). Already discussed elsewhere.

Mark Trapp’s picture

ygerasimov’s picture

Status: Needs work » Needs review
FileSize
2.21 KB

Patch rewritten not to use media in stylesheets-remove[].

I believe the stylesheets[] should have priority over stylesheets-remove[]. Lets think about novice person creating his subtheme. He needs substitute parent stylesheet with his own. He can think of disabling parent stylesheet first (using stylesheets-remove[]) and then adding his own (using stylesheets[]). If we implement stylesheets-remove[] as priority this will cause complete disabling of both stylesheets: base theme and own subtheme. This will cause confusion as it is quite logical to disable parent first and than add new one. This is the reason I implemented priority for stylesheets[].

Regarding 404. I have added file_exists() check in list_themes() as it has been done with $theme->scripts. So I hope this will be ok.

ericduran’s picture

FileSize
1.39 KB

Ok so this patch add the stylesheets-remove[] option without the media type, as the names are supposed to be unique.

This patch only supports removing stylesheets from the parent theme using the stylesheets-remove[] = name.css .

I believe this is the implementation we want right?

ericduran’s picture

@ygerasimov, we must had been working on it on the exact same time :-)

ericduran’s picture

so I just looked at #575298: Provide non-PHP way to reliably override CSS, they seem to be adding a stylesheet_overwrite, and here where adding stylesheet-remove, is this the functionality we want?

We want to explicitly say which are being overwritten and what's being remove?

ygerasimov’s picture

FileSize
868 bytes

@ericduran :) same time indeed

Regarding #575298: Provide non-PHP way to reliably override CSS I believe it is about a bit different topic. That is about conditional css being overridden. Anyhow we have different types of stylesheets loaded (conditional and permanent). I think there should be one approach to override / remove / additions for stylesheets in order not to make themers completely lost with the system.

So lets close this issue with regression just adding file_exist() check for stylesheets in list_themes(). And than create separate issue for introducing stylesheets-remove for all kinds of stylesheets.

Please let me know your comments about this.

Status: Needs review » Needs work

The last submitted patch, 901062-35.patch, failed testing.

ygerasimov’s picture

Status: Needs work » Needs review
FileSize
790 bytes

I have added check in _drupal_theme_initialize().

sun’s picture

Status: Needs review » Needs work

Now we went in circles a bit.

Let me try to explain the intended end result by taking all of the current .info file issues and patches into account:

stylesheets[all][] = style.css
stylesheets[all][] = node.css
stylesheets-override[] = vertical-tabs.css
stylesheets-remove[] = system-menus.css

If not totally obvious already, let me also explain what this translates into:

  1. We unconditionally (always) load style.css from this theme.
  2. We unconditionally (always) load node.css from this theme, which additionally happens to override Node module's node.css.
  3. We conditionally override vertical-tabs.css with a replacement file from this theme, which is only loaded when the original vertical-tabs.css would normally be loaded.
  4. We never (ever) load system-menus.css for this theme.

This means we want to go back to earlier patches here.

ygerasimov’s picture

Yes. This is great explanation. Lets do it.

Also I would like to clarify inheritance for stylesheets[] and stylesheets-remove[]. If theme A has stylesheets-remove[] = system-menus.css but its subtheme B has stylesheets[all][] = system-menus.css the file system-menus.css will be added in subtheme B.

Another case is: if theme .info file has both:

stylesheets[all][] = system-custom-menus.css
stylesheets-remove[] = system-custom-menus.css

file will not be added.

sun’s picture

re #39:

1) That's one key detail we need to discuss and agree on. I'm not sure whether there should be any inheritance at all. I.e., only account for stylesheets-remove[] of the active theme. Ignore base theme definitions entirely. Throwing this into the discussion as potential option.

2) Not sure whether that needs explanation... it simply does not make sense to say "add this" and "remove this" at the same time. Therefore, I'd refrain from specifying and ensuring any expected logic for that, because it simply is illogical.

Jacine’s picture

We unconditionally (always) load node.css from this theme, which additionally happens to override Node module's node.css

I think stylesheets-override[] is part of the solution here: #575298: Provide non-PHP way to reliably override CSS. Then it becomes crystal clear to everyone what is going on:

stylesheets[all][] = style.css
stylesheets-remove[] = system.css
stylesheets-override[] = node.css
sun’s picture

Right. Though I purposively wanted to differentiate between the two options in my example:

stylesheets[all][] = node.css
stylesheets-override[] = node.css

The first is always loaded AND always aggregated (if aggregation is enabled).
The second is only loaded instead of node.css, when node.css would normally be loaded; aggregation and all other options depend on the module/call that loads the file.

Jacine’s picture

Yes, exactly. That makes perfect sense. After all, when you override node.tpl.php in a theme, Drupal doesn't load it on pages without nodes, just because it's there. LOL.

mason@thecodingdesigner.com’s picture

sub

marcvangend’s picture

I think the combination of properties proposed here makes total sense, but I'm not sure if we can just neglect the media type on stylesheets-override and stylesheets-remove. Just a couple of imaginary situations: What if...
- I want to override node.css only for print media;
- I want to remove node.css only for print media;
- I want to remove node.css for all media and then add my own node.css for print media only?

That said; I think we all want a system that is as flexible as possible, but we should be aware not to create a system that is too complex to use. Let's keep the D7UX principles in mind and design for the 80% (of theme developers, in this case).

chichiMi5’s picture

sub

ygerasimov’s picture

I'm not sure whether there should be any inheritance at all. I.e., only account for stylesheets-remove[] of the active theme. Ignore base theme definitions entirely.

I think inheritance should be done for everything. Otherwise themer expect his subtheme working same as parent theme but it will not if parent has some stylesheets-remove[] options.

catch’s picture

Issue tags: +API change

I don't think we should be making changes to the API this late, but if we do, it needs to be tracked.

ygerasimov’s picture

Status: Needs work » Needs review
FileSize
2.72 KB

Are we going to join the patches with #575298: Provide non-PHP way to reliably override CSS to have one instead of two?

Here is patch for stylesheets-remove[] with file_exist check from #37.

Jeff Burnz’s picture

I'm not sure what direction this patch is taking with regards to inheritance but from my POV subthemes must inherit the base themes definitions, but can override them.

Nine times out of ten our base themes do not even get enabled - for most sites the base theme is akin to a giant "reset" and a repository of common resources. The subtheme draws on these resources, inherits the "reset" (which consists of CSS, tpl files, template.php stuff and so on), and is merely a place to add design style. Any required logic is carefully assessed as whether (a) is site or section specific or (b) not, if not - is pushed back to the base theme to reduce duplication. I would think that removing or overriding core stylesheets is very much a (b) scenario - where you would do this in the base theme and have it inherited by all subthemes.

It would seem to me that if you were going to override something like node.css or block.css that would be a pretty big decision to make and would seem odd that you suddenly wanted to do that in a subtheme - you might, and you should be able to, but more often I would do that in a base theme. That said - the subtheme should be able to easily override this and add the core stylesheet back in if required, although I would think that is less important and more of an edge case.

I think it depends on how you view base themes - what they are, what they might consist of - for example I would contest that traditionally its been viewed that base themes are small, lightweight things that maybe consist of basic CSS reset and a layout. In this scenario the heavy lifting goes in the subtheme. However more so the base themes have become very large and complex things (the "big reset" + repository of logic and resources) - the base theme does the heavy lifting and subtheme is very light-weight. In my thinking this more Drupaly way of working (think core and Stark) and is pretty much how most popular base themes work (e.g. Zen).

sun’s picture

Status: Needs review » Postponed
+++ includes/theme.inc	7 Sep 2010 06:50:50 -0000
@@ -160,11 +170,23 @@ function _drupal_theme_initialize($theme
-      drupal_add_css($stylesheet, array('weight' => CSS_THEME, 'media' => $media, 'preprocess' => TRUE));
+      if (file_exists($stylesheet)) {
+        drupal_add_css($stylesheet, array('weight' => CSS_THEME, 'media' => $media, 'preprocess' => TRUE));

@@ -600,7 +622,14 @@ function list_themes($refresh = FALSE) {
-          $theme->stylesheets[$media][$stylesheet] = $path;
+          if (file_exists($path)) {
+            $theme->stylesheets[$media][$stylesheet] = $path;

This needs to be reverted. We do not want any file_exists() check, neither for CSS files nor for JS files.

+++ includes/theme.inc	7 Sep 2010 06:50:50 -0000
@@ -600,7 +622,14 @@ function list_themes($refresh = FALSE) {
+      if (isset($theme->info['stylesheets-remove'])) {
+        foreach ($theme->info['stylesheets-remove'] as $stylesheet) {
+          $theme->stylesheets_remove[$stylesheet] = $stylesheet;

This should go into _system_rebuild_theme_data().

However, I'm going to postpone this issue on #575298: Provide non-PHP way to reliably override CSS now, because this issue needs to take those changes into account, and we should really try to move most of all this code into _system_rebuild_theme_data(), so that it is cached and not redone from scratch on every single request.

Powered by Dreditor.

juan_g’s picture

The blocker of this critical D7 bug, #575298: Provide non-PHP way to reliably override CSS, is RTBC.

Both are API change, and important for the Drupal designer community (see the names following them: the maintainers of Skinr, AdaptiveTheme, Zen...). It seems reasonable that API change bugs should be fixed before Drupal 7 beta, to facilitate that D6 modules are ported to D7.

BTW, since the status of this bug is Postponed, it does not appear in the list of critical D7 issues linked from Drupal.org. And neither the blocker of this critical bug, a major bug, and RTBC since three weeks ago.

Jacine’s picture

Status: Postponed » Needs work

This #575298: Provide non-PHP way to reliably override CSS, and many others probably have a less than 1% shot of making it into D7, so I am switching this back to active.

The above issue sat RTBC for weeks, only to be shot down for making changes to .info files, so we cannot let that happen here. We need to just fix the bug and forget about stylesheets-remove and move on. I believe that takes us back to the patch in #13?

Jacine’s picture

Status: Needs work » Postponed

Err, sorry... Never mind. Disgard my last post. Setting this back to Postponed until we hear back from Dries.

juan_g’s picture

Issue tags: +blocked

From Status settings for an issue:

Postponed

May mean either 1) that the issue is valid and should be fixed, but other related issues (blockers) need to be dealt with first, or 2) that the issue is removed from current active work but the intent remains to return to it. In the first case, please tag the issue "blocked" and say what issue(s) it's blocked on in a comment.

This regression bug is a critical D7 issue blocked by #575298: Provide non-PHP way to reliably override CSS.

juan_g’s picture

The Drupal 7 co-maintainer, webchick, has just explained what's a critical bug. A brief excerpt:

(...) that we can't possibly release without it being fixed. (...)

As a general rule, these include:

* (...)
* Serious regressions from behaviour that worked in previous releases
* (...)

That's right. For example, this critical D7 bug is a regression from D6.

The quote is part of a new writeup on Drupal core's release cycle.

webchick’s picture

Status: Postponed » Needs work

Ok. #575298: Provide non-PHP way to reliably override CSS has been moved to D8. #13 - restoring the same functionality as D6 - has the latest patch for review. Looks like testbot doesn't like it.

juan_g’s picture

webchick wrote on #575298:
> #2: This patch doesn't hold up #901062 at all, from what I can tell. The minimal required to fix that issue is to restore the removal functionality from D6 which is a 766 byte patch. And even the "maximal" fix depends on nothing in this patch, apart from the two will conflict because they touch the same general area of the code, at least from my 10-second scan.

All right. Removing the blocked tag.

And about the API change tag, it depends on which fix is going to be implemented, file_exists or stylesheets_remove. It looks like there are few days left for any API change: Drupal 7 beta is knocking at the door.

webchick’s picture

Issue tags: -blocked

stylesheets-remove is off the table, for the same reason stylesheets-override is off the table. The job of this patch in this issue is simply to fix the regression.

What we should probably do is combine the later patches/discussions in this issue + #575298: Provide non-PHP way to reliably override CSS into a nice "Don't bludgeon themers in the face with PHP" feature request for Drupal 8.

But in terms of closing this critical, we need something along the lines of #13.

juan_g’s picture

Issue tags: -API change

Removing the API change tag then.

webchick’s picture

Thanks, juan!

reglogge’s picture

The patch in #13 doesn't work.

Test-case:
- I want to remove /modules/search/search.css
- I put stylesheets[all][] = css/search.css in Bartik's .info file without having the actual file there.

Current behavior without #13:
- Drupal tries to load /themes/bartik/css/search.css which results in an expensive 404

Behavior with the patch from #13:
- Drupal loads /modules/search/search.css

I think this issue should be closed with won't fix. Here's why:

a) If a themer wants to completely remove a stylesheet provided by Drupal core, he can still just put an empty file with the same name as the core stylesheet into his theme's css directory or wherever. If he does this plus then puts stylesheets[all][] = css/[same-name].css into his .info file, everything works. The empty file gets loaded, the core stylesheet is in effect removed. No error 404 is thrown. No patching needed.

b) The technique of putting stylesheets[all][] = css/[same-name].css in the .info file is *hackish* to begin with. If you want to *hack* Drupal's standard behavior (even if it has been done the traditionally), why not require the person doing the *hacking* to create a simple empty file along with his/her *hack*? (Sorry for the *hack* thing, I just lack a better word)

Plus I can't really see why this is critical at all anymore.

mfer’s picture

sub.

juan_g’s picture

The D7 regression from the D6 behavior, according to comment #12, seems to have happened -probably inadvertently- when a patch removed the following file_exists code from function drupal_get_css (file includes/common.inc):

-        if ($type == 'inline') {
-          $no_inline_preprocess .= drupal_load_stylesheet_content($data, $preprocess);
-        }
-        // Only include the stylesheet if it exists.
-        elseif (file_exists($data)) {
-          if (!$preprocess || !($is_writable && $preprocess_css)) {
-            // If a CSS file is not to be preprocessed and it's a module CSS file, it needs to *always* appear at the *top*,
-            // regardless of whether preprocessing is on or off.
-            if (!$preprocess && $type == 'module') {
-              $no_module_preprocess .= '<link type="text/css" rel="stylesheet" media="' . $media . '" href="' . base_path() . $data . $query_string . '" />' . "\n";
-            }
-            // If a CSS file is not to be preprocessed and it's a theme CSS file, it needs to *always* appear at the *bottom*,
-            // regardless of whether preprocessing is on or off.
-            elseif (!$preprocess && $type == 'theme') {
-              $no_theme_preprocess .= '<link type="text/css" rel="stylesheet" media="' . $media . '" href="' . base_path() . $data . $query_string . '" />' . "\n";
-            }
-            else {
-              $output .= '<link type="text/css" rel="stylesheet" media="' . $media . '" href="' . base_path() . $data . $query_string . '" />' . "\n";
-            }
-          }
-        }

This means that now a greater number of lines like:

<link type="text/css" rel="stylesheet" ... />

are now added in D7 to the html source, for css files called by every Drupal page, even if they do not exist. Hence the 404 errors.

There are two different ways to solve this:

  • Designers creating empty css files for each line in the .info file to remove stylesheets. This also adds more link lines in the html source of every Drupal page, calling the empty files to load them.
  • Or fixing this D7 regression, recovering the D6 behavior, with the same lines in the .info file of the other solution, but with no need for creating empty css files. This also completely removes all unnecessary file calls from the html source.

Even when the two possibilities are documented, about the first one (empty css files) mentioned by reglogge, I think that in fact this is the hackish workaround and not the other (D6), being the D6 behavior not only easier and faster for designers but also more logical and efficient for Drupal performance. In my opinion, the first solution can be only a workaround until this D6 -> D7 regression is fixed.

juan_g’s picture

There is little online documentation about the first possibility. On the drupal.org documentation, I can only find one mention to the empty css files, which has been recently added on April 19, 2010 to the handbook page about sub-themes.

The other method is more documented, and the one generally used by themers for D6, as described for base themes such as Zen. On February 23, 2008, the D6 handbook page Adding style sheets was "rewritten for clarity" to say:

.info - Overriding the base theme's style sheets:

This only applies to sub-themes. To prevent a style sheet from a base theme from being carried over to a sub-theme, you can redefine the style sheet inside the .info file.

The base theme and the sub-theme must have the same entry:

  stylesheets[all][] = masterStyle.css
       
If the file exists inside the sub-theme, then it will be used while omitting the file will prevent it from loading.

The overriding info was moved on December 1, 2008 to Overriding style sheets from modules and base themes, saying:

Adding the override for a style sheet that does not exist inside the theme will omit the core or module style sheet. This is by design and this behavior has been corrected since the release of 6.0 in 6.3.

This refers to an interesting bug that was fixed for Drupal 6.3 (July 2008):

#197124: even though documented and intended, themes could not remove module stylesheets by specifying their name with a non-existent file

Basically, the fix was:

+        // Only include the stylesheet if it exists.
+        if (file_exists($file)) {

which has been removed in the D7 regression (the current bug).

JohnAlbin explained about that fix (July 2008):

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.

On that critical bug, they mentioned:

the bug is critical because this is a key feature in the new theme layer that isn't working.

Etc...

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
6.39 KB
The technique of putting stylesheets[all][] = css/[same-name].css in the .info file is *hackish* to begin with.

I disagree. I think it's completely legitimate for a custom theme to want to completely remove some module's or base theme's css file. I don't see how that's hacking. Theme's should always have full control over all markup, in as non-php-programmer friendly a way as possible. We don't always achieve that goal 100% in Drupal, but it's what we should constantly strive towards.

This is a regression from D6, and is therefore in-scope for D7. Critical? I don't know, but doesn't really matter: here's a patch that fixes the regression without any down-side. Note that we're already checking file_exists() when aggregation is enabled, via drupal_load_stylesheet(). This adds the same check when aggregation is disabled. Consistency++.

sun’s picture

What that comment is referring to is that it is completely illogical to specify a file within an array of to be loaded files to *not* load it. Can we agree on that? :)

I have serious troubles to understand why we prefer to re-introduce this hack, while a perfectly logical, elegant, and working solution has been proposed and worked out with stylesheets-override and stylesheets-remove, which everyone on this planet is able to understand without having to look up handbook pages or reading lengthy code comments. It's beyond me. But anyway, http://drupal.org/project/edge will introduce the required sanity (and already contains a patch).

If we really want to go this route, then effulgentsia's patch looks fine (though the comments read a bit lengthy). Thanks for moving this forward.

Status: Needs review » Needs work

The last submitted patch, drupal.css_file_exists.66.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
7.3 KB

Test failures was because CascadingStylesheetsTestCase::testModuleInfo() was relying on module.info lines leading to CSS inclusion tags being output, but the corresponding files weren't there, so if we're adding the file_exists(), we need to add the files to the test folder, which is what this patch does.

Yeah, code comments could probably be shortened. If someone wants to re-roll or suggest changes, please do. Otherwise, perhaps that can be bikeshed in a non-critical follow-up?

I too am sad that stylesheets-override and stylesheets-remove syntax didn't make it into D7, especially the former, which would have been quite enjoyed by many theme developers. But oh well, we can't have everything. Cool that it's in the Edge project, and it will be neat to see how many contrib themes list Edge as a dependency. Meanwhile, let's at least solve the regression from D6 and inconsistency between aggregation enabled and aggregation disabled.

juan_g’s picture

Thanks, effulgentsia and sun! Reading what webchick has said on the two issues, in fact she supported stylesheets-override and stylesheets-remove... but for D8 core, and if possible a D7 module. And now there is Edge...

I think they are now trying to release Drupal 7 as soon as possible, and are quickly fixing criticals, but are afraid of any possible delays that API changes could cause. In my opinion this is exaggerated, and I think that fixing as many issues as possible is the way to go, even with API changes before beta. But I can be wrong, naturally.

Anyhow, looking at the issue queue, I would bet that a revolution is coming this next week, with a name beginning with b... ;)

sun’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome work, effulgentsia!

Tested and confirmed that it restores D6's bizarre, yet working functionality. Great that we have tests for this now, too, so that we don't accidentally remove it again. The book of comments explaining how this weird mechanism works is a very nice touch. :)

Committed to HEAD! Thanks!

juan_g’s picture

Thank you!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

sun’s picture

#967166: Content rendered via AJAX does not respect stylesheets removed in .info files explains why the originally proposed solution is required. I'd be happy if core maintainers would re-consider the decision on stylesheets-remove[].

Damien Tournoud’s picture

Agreed that this feels really extravagant. Plus it requires a file_exists(), which is really bad for performance.

I would welcome letting this "feature" go.

Taiger’s picture

It is an important feature though stylesheets-remove[] would be even better.

Personally I use hook_css_alter(). However, there should be an easier way for people new to Drupal theming. It is a lot to ask of someone coming from Wordpress.