Overriding module CSS with theme CSS [#189568]

According to the API docs for drupal_add_css():

"Themes may replace module-defined CSS files by adding a stylesheet with the same filename. For example, themes/garland/system-menus.css would replace modules/system/system-menus.css. This allows themes to override complete CSS files, rather than specific selectors, when necessary."
(http://api.drupal.org/api/function/drupal_add_css/6)

However, this does not seem to be the case. Instead, both CSS files are included.

To reproduce:

Create a module that makes use of a theme CSS file (included with drupal_add_css()). For example, a module named 'my' that uses a CSS file called 'my.css'.
Create a CSS file in your chosen theme (I used bluemarine) that also contains a file my.css
Modify the theme's .info file, adding: style[all][]=my.css
Do whatever must be done to refresh the style.

What is expected:
The my.css file from the module should not be included in the <link/> elements in the HTML. Instead, the theme's my.css file should be included.

What happens:
Both are included.

Either the API documentation for drupal_add_css() should be fixed, or the behavior of the theme system should be fixed.

Details:

Version: Drupal 6, beta 2
OS: Ubuntu Linux 7.04
Webserver: Apache 2.2
PHP: 5.2.1
Browser: Firefox 2.0

Relevant snippets from module named philquotes:

function theme_philquotes_quote($text, $origin) {
  $full_path = drupal_get_path('module', 'philquotes') 
    .'/philquotes.css';
  
  drupal_add_css($full_path);
  $output = '<div id="philquotes-text">'. t($text) 
    .'</div><div id="philquotes-origin">' . t($origin) . '</div>';
  return $output;
}

And the CSS in the module's philquotes.css:

#philquotes-text:first-letter {
  	font-size: 18pt;
  	font-weight: bold;
}

In bluemarine, I added philquotes.css:

#philquotes-text {
    color: pink;
}

And modified bluemarine.info, adding the following two lines:

stylesheets[all][] = style.css
stylesheets[all][] = philquotes.css

The resulting HTML included link tags for all three of the following:

href="/drupal/sites/all/modules/philquotes/philquotes.css" 
href="/drupal/themes/bluemarine/style.css"
href="/drupal/themes/bluemarine/philquotes.css"

Based on the API docs, I did not expect the first CSS file to be included.

Comment	File	Size	Author
#27	css_compress_d6.patch	908 bytes	desbeers
#22	style_aggregation.patch	894 bytes	dvessel
#16	style_override_from-add-to-get_b7.patch	5.5 KB	dvessel
#11	style_override_from-add-to-get_b.patch	5.51 KB	dvessel
#10	style_override_from-add-to-get.patch	5.22 KB	dvessel

Comments

Comment #1

mbutcher commented 5 November 2007 at 21:26

Minor correction:
Step #3 above should modify stylesheets[all][], not style[all][].

This is correct in the given code, and (obviously) the unanticipated result still obtains.

Comment #2

jjalocha commented 4 December 2007 at 03:25

This bug seems to be related to http://drupal.org/node/197124.

Comment #3

dvessel commented 4 December 2007 at 07:51

Status:

Active

» Postponed (maintainer needs more info)

I can't reproduce. I tried with system-menus.css and the one from the theme is the only one present.

Comment #4

jjalocha commented 4 December 2007 at 17:46

I can't reproduce here with the following files:

themes/newtheme/
themes/newtheme/newtheme.info
themes/newtheme/style.css
themes/newtheme/system-menus.css

'newtheme.info' file:

name = NewTheme
description = Test New Clean Theme
core = 6.x
engine = phptemplate

stylesheets[all][] = style.css
stylesheets[all][] = system-menus.css

The 'style.css' file is empty.

'system-menus.css' file:

ul.menu a { color: red; }

Result is as it should be: The browser's default bullets are applied, and links are displayed in red.

'modules/system/system-menus.css' is not in <head> section:

<link type="text/css" rel="stylesheet" media="all" href="/modules/node/node.css" />
<link type="text/css" rel="stylesheet" media="all" href="/modules/system/admin.css" />
<link type="text/css" rel="stylesheet" media="all" href="/modules/system/defaults.css" />
<link type="text/css" rel="stylesheet" media="all" href="/modules/system/system.css" />
<link type="text/css" rel="stylesheet" media="all" href="/modules/user/user.css" />
<link type="text/css" rel="stylesheet" media="all" href="/themes/newtheme/style.css" />
<link type="text/css" rel="stylesheet" media="all" href="/themes/newtheme/system-menus.css" />

Comment #5

jody lynn

she/her

English

commented 7 December 2007 at 00:45

Status:

Postponed (maintainer needs more info)

» Closed (fixed)

Can't reproduce in Beta 4.

Comment #6

oadaeh commented 10 February 2008 at 15:24

Status:

Closed (fixed)

» Active

Apparently, it matters when drupal_add_css() is called. If it is called in hook_init(), as is the case for system-menus.css and several (all? -- I only sampled them) other core modules, it works as expected. If it is called at some other time, like in hook_block() or a theme() function, you end up with both CSS files being included. FYI, I didn't test all functions to find out if the absolute of only hook_init() and all other functions was true. It could be there are other times when it works correctly.

Comment #7

dvessel commented 10 February 2008 at 17:47

Just did a quick look over the flow and this is definitely a problem. If the module styles are added into drupal_add_css() first, it's not a problem. The problem arises when setting override styles through the .info file. They are added in at _init_theme() so a module pushing styles outside of hook_init() (which runs before init_theme()) will get added in a second time. Not good.

Either more complexity is added to drupal_add_css() or we delay the adding of styles defined in .info so it happens around template_preprocess_page(). Or can anyone think of a better idea?

Comment #8

moshe weitzman commented 11 February 2008 at 22:17

delay adding of styles sounds right to me.

Comment #9

dvessel commented 12 February 2008 at 22:45

I just had a thought. How about moving the overriding into drupal_get_css. It happens much later and it doesn't run as often so it could save a few cycles.

I'll have a patch later tonight.

Delaying the .info style inclusion would make a mess of template_preprocess_page. The other option would be to hold the styles in a new function to be called in template_preprocess_page. The latter would be too much for 6.

Comment #10

dvessel commented 12 February 2008 at 23:20

Status:

Active

» Needs review

Status	File	Size
new	style_override_from-add-to-get.patch	5.22 KB

This fixes 2 issues actually. The one mentioned here and themes overriding module styles where RTL files need to be explicitly set for it to override the module RTL style.

- *   If the original CSS file is being overridden by a theme, the theme is
- *   responsible for supplying an accompanying RTL CSS file to replace the
- *   module's.

This is no longer needed. I also removed the mention of 'core' type for styles. It was considered while developing the CSS aggregation feature during 5.x-dev but it never got in. It's only 'module' and 'theme'.

Comment #11

dvessel commented 13 February 2008 at 18:20

Status	File	Size
new	style_override_from-add-to-get_b.patch	5.51 KB

Correction on the theme overriding module RTL styles. I forgot there was a good reason for that. RTL styles depend on cascade order so I put that back in.

This should behave the same as before. It's only done on get. I tested it and I think it's ready. If someone could verify and update the status, that'd be great.

This really simplifies drupal_add_css(). Count the number of style sheets on your pages, and that's the number of times it's called while drupal_get_css() typically gets called only once.

Comment #12

oadaeh commented 15 February 2008 at 07:15

I'm not sure if this was an intended side effect or not, but after including this patch, it is no longer necessary to have "stylesheets[all][] = stylesheetname.css" for the sheet to be included, as is stated in http://drupal.org/node/171205#stylesheets. The file just simply needs to be in the theme directory to be included. Other than that, this seems to work as expected.

Comment #13

dvessel commented 15 February 2008 at 15:07

oadaeh, the patch does nothing to automatically add style sheets. By default style.css does load on it's own but that's what always happened. http://drupal.org/node/171206#default-stylesheets

If it's not named style.css, then it could simply be the .info file being cached which can be cleared by resubmitting the theme select form. Not to be confused with the theme registry.

Comment #14

oadaeh commented 15 February 2008 at 16:10

Status:

Needs review

» Reviewed & tested by the community

Okay. I re-installed my Drupal testbed (dual installation for this bug) and re-ran through all the tests I did before, and this time it worked as expected. Maybe I did have a cached .info file (though I was seeing the updated change on the screen right away), or maybe my test install was tweaked from my testing. One thing I noticed during the testing for this bug was that clearing the cache didn't always get me an updated page. I sometimes needed to go to the modules and/or themes admin pages for the theme changes to be reflected on the page.

I'm giving this the big RTBC, unless we want someone else's eyes on it.

Comment #15

dvessel commented 16 February 2008 at 23:30

Component:

base system

» theme system

Thanks for testing. This should apply to 7 also.

Comment #16

dvessel commented 20 February 2008 at 05:38

Version:

6.x-dev

» 7.x-dev

Status	File	Size
new	style_override_from-add-to-get_b7.patch	5.5 KB

Forgot this has to be marked for 7. And the patch due to fuzz.

Patch #11 still applies to 6.

Comment #17

dries commented 20 February 2008 at 13:37

Version:

7.x-dev

» 6.x-dev

I've committed this to CVS HEAD because it looks sound. I think it is sound for D6 as well but it could use another review.

Comment #18

dvessel commented 22 February 2008 at 18:40

Could we get someone to test this? It'd be nice to get this in before 6.1.

Comment #19

gábor hojtsy

he/him

Hungarian

Hungary

commented 27 February 2008 at 11:53

Status:

Reviewed & tested by the community

» Fixed

I reviewed all the report and the code and it all looked fine. I stopped for a considerable time on the last hunk while trying to understand how things happen there, but it became understandable after looking at the code around. Committed to 6.x.

Comment #20

vladimir.dolgopolov commented 4 March 2008 at 04:38

Here is the subsequent test for the issue I'd made for D6.


class TestCase189568 extends DrupalTestCase {
  function get_info() {
    return array(
      'name' => t('[189568] Overriding module CSS with theme CSS'),
      'desc' => t('Themes may replace module-defined CSS files by adding a stylesheet with the same filename. For example, themes/garland/system-menus.css would replace modules/system/system-menus.css. This allows themes to override complete CSS files, rather than specific selectors, when necessary.'),
      'group' => t('Drupal 7 Tests'),
    );
  }
  
  // There are 2 similar CSS. One belongs to a module, second belongs to a theme.
  // The module's CSS should not be showed.
  function testIssue() {
    $in_module_css = 'my_test_module/my_test_module.css';
    $in_theme_css = 'themes/general_theme/my_test_module.css';

    drupal_add_css($in_theme_css, 'theme'); // order here is important!
    drupal_add_css($in_module_css, 'module');

    $result = drupal_get_css();

    $this->assertNoUnwantedPattern("|$in_module_css|", $result, t('Module\'s CSS should not be here'));
  }

}

Comment #21

dvessel commented 9 March 2008 at 04:15

Version:	6.x-dev	» 7.x-dev
Status:	Fixed	» Active

Arg! The bug comes back when aggregation is turned on.

Continuing on the loop isn't good enough since $types is passed to drupal_build_css_cache. Just need to unset it & continue but must account for RTL styles.

Patch coming..

Comment #22

dvessel commented 9 March 2008 at 05:11

Status:

Active

» Needs review

Status	File	Size
new	style_aggregation.patch	894 bytes

Well, rtl is already accounted for. Just needed to unset. This'll apply to both 6 & 7.

Comment #23

floretan commented 22 March 2008 at 17:50

Status:

Needs review

» Reviewed & tested by the community

Successfully tested on 7.x-dev.

Comment #24

dvessel commented 10 April 2008 at 03:44

This is still a problem.

Comment #25

dries commented 10 April 2008 at 10:19

Version:

7.x-dev

» 6.x-dev

I've committed this to CVS HEAD. Still needs to go into DRUPAL-6 so I'm lowering the version.

Comment #26

damien tournoud commented 23 April 2008 at 23:24

#250481: css optimizer uses both core and override css was a duplicate.

Comment #27

desbeers commented 9 May 2008 at 16:29

Status	File	Size
new	css_compress_d6.patch	908 bytes

Attached a patch for Drupal 6.

Comment #28

gábor hojtsy

he/him

Hungarian

Hungary

commented 19 May 2008 at 08:04

Status:

Reviewed & tested by the community

» Fixed

Committed to 6.x. Thanks!

Comment #29

Anonymous (not verified) commented 2 June 2008 at 08:35

Status:

Fixed

» Closed (fixed)

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

Comment #30

egfrith commented 5 February 2009 at 10:59

I think the patches here may have caused a regression: #335106: sub-theme doesn't import parent's css

Comment #31

MGParisi commented 30 March 2009 at 04:04

Issue http://drupal.org/node/417874 seems to be related to the issue. The

drupal_add_css(drupal_get_path('module', 'author_pane') . '/author_pane.css');

is in the template_preprocess_author_pane(&$variables function. Should be easy for the module developer to change the code given this information, but this "seems" to be a continuation of the problem listed. Not sure, just sharing my experience...

Note: All Cache is OFF! No RTL is being used (currently).

I can fix it with inserting this into the themes .info file...

Will confirm with another user ASAP (Its sunday at Midnight, I doubt I will find a Advanced Forum user on the Drupal IRC site, but I will see.

Comment #32

pteague commented 30 March 2009 at 05:35

Just installed drupal 6.4, author_pane-6.x-1.0-rc1 & advanced_forum-6.x-1.0-rc4.

Copied & modified the author_pane.css to the garland theme directory & modified a couple things. The author_pane.css from the garland theme isn't even being included, only the author_pane.css from the module directory.

Comment #33

MGParisi commented 30 March 2009 at 16:10

Update: The original .css is removed if you put the css file name in the info. So I guess this works as stated... issue remains closed...