Simplify adding CSS in Drupal

Oh dear. Looks like you and I are tackling the same problem at the same time in two entirely different ways. :-)

http://drupal.org/node/73949

How does it not allow for overriding? The patch I submitted allows a theme author to override a CSS file for a given module with no PHP code needed. As I recall, there was talk of using the exact same method for template files (split all theme files out to template files, then copy/paste/modify to create a theme).

After looking it over, this appears to be pretty flexible.

Am I correct in assuming that the real benefit here is consistency in function naming? IE, we don't have modules calling 'invisible' theme functions to do something that's not output related? Other than that, it seems to be a different way of doing what can be done now...

I think you're right; maintaining things in a list of properly organized CSS files is a win, and moves things closer to the emerging Drupal standard: 'store stuff in structured arrays until the very last moment necessary.'

If I understand correctly, *previously* themes would override theme_get_style() and pull out stylesheets they didn't want included. With this patch, they would (in template.php, or on a page-by-page basis in page.tpl.php) just reach into the variables collection and do whatever they like to $css['core'][] or $css['module'][] arrays. Then, when the styles are output, those changes are reflected.

Is that correct? I think the new way makes more sense and avoids the strange 'use this theme function even though it doesn't output anything' effect. My only question is whether the benefits outweigh 'yet another task for module developers in their 4.7->4.8/5.0 conversion list.'

I lean towards yes.

Wow, I didn't realize something as simple as adding a new CSS file was currently so complicated. This approach looks to be much more straight forward.

*subscribe*

I am all for this.

I tested the patch. It works. +1 for this.

I dare to set this rtbc. It's a small change but one that can remove a lot of confusion and inconsistency.

There is still an issue: theme_stylesheet_import() is called *before* any theme can override it, thus making the override function worthless. The creation of the HTML code should take place in drupal_get_css() rather than in drupal_add_css().

This is awesome. +1.

Rock on \m/ +1

I'm so never setting anything RTBC again. ;)

That said, the new approach to allow theming of the individual items is good.

Looks solid. I can't find any errors. Ready to commit!

Good job, m3avrck!

Usage question here.

Where is the "correct" place in a module, then, to queue up a CSS file? Does this fix the "dump it into hook_menu()" problem?

If a module adds a CSS file, then when overriding it do I need to know where the module is located, so I know its path?

The override example in #8 is PHPTemplate centric, if I understand it correctly. How would other themes handle it?

Are we going to properly slice and dice drupal.css finally? :-)

The proper place is either 1) when you know you need the CSS file or 2) in hook_menu if you want it to always be there.

Yes, this is a prelude to splitting up drupal.css

Would it work in hook_init()? And what about the override ability?

Generally a module has just one CSS file it tosses into the mix. Themes need to be able to easy cherry pick that list, regardless of where it's coming from.

I did look at those, which is why I'm asking. You have:

  $css['core']['misc/drupal.css'] = '';
  print drupal_get_css($css);
  //print $styles ... don't need this anymore since we customized this list

That is explicitly specifying the path to drupal.css, relative to drupal root. Does that mean that the code to disable the default location module CSS file (for instance) will be different if the location module is installed in /modules/ or /sites/all/modules/ or /sites/default/modules/? That's what I'm asking about.

If no, and you can override the incoming CSS regardless of where the module is, great. If you do have to know the path, however, then it means you can't make portable themes that override a module's CSS file, since they'd be dependent on each installation's path setup. That would be a show stopper.

I haven't worked with non-PHPTemplate themes enough (read: at all) to know if there is any impact on them. Someone else will have to chime in on that front.

(I'm not trying to be a stick-in-the-mud, really! :-) )

Assuming the file I'm looking for is SOMEPATH_I_DON'T_KNOW_ABOUT/foo.css and that is in the variable $path:

basename($path) == 'foo.css';

Thus:

if (basename($path) == 'foo.css') {
  $path = path_to_theme() . 'bar.css'; // cause I'd rather have bar than foo
}

drupal_get_css(drupal_add_css())

- PHP code in default implementations of themeable functions should be as simple as possible, this isn't.
- This doesn't follow the convention of similar functions in Drupal.

I'm guessing the goal here is the ability to modify the CSS data structure before it gets to drupal_get_css(). I don't see that as necessary since there is theme_stylesheet_import() called later. But to maintain this functionality, I'm okay with keeping the argument with a default value of NULL that signals it should default to drupal_add_css().

Changed drupal_get_css($css) to drupal_get_css($css = NULL) and if $css is null, then $css = drupal_add_css() as drumm suggested.

How's this look now?

Oops. Patch doesn't apply. I'll have to reroll... That's what you get for simply editing a patch file instead of making it properly! :)

Try this one.

+ RewriteBase /~rowan/drupal-cvs/HEAD/drupal

Probably not what you want in drupal =)

Hmm yeah right :) ... removed that .htaccess chunk.

Several changes:

• Moved the drupal_get_css() and drupal_add_css() functions to common.inc as they aren't theme functions anymore.
• Removed the call to drupal_add_css in phptemplate.engine as it isn't necessary anymore.
• Changed theme_get_styles() to drupal_get_css() in chameleon.theme (which has been forgotten).

In chameleon we have:

$output .= drupal_get_css();
$output .= theme('stylesheet_import', base_path() . path_to_theme() ."/common.css");

Is that correct? Looks like we're almost able to remove theme_stylesheet_import as well?

You're right. I remove the call to theme('stylesheet_import') in chameleon.theme and replaced it with drupal_add_css(..., 'theme');.

I also removed theme_stylesheet_import() and replaced the only remaining call to this function with the source code itself. Now that we can control what styles are added, we don't need to theme it anymore. We don't theme link and script tags either.

Added an (optional) to some arguments and explained what it does when no argument is given.

In the code comments and PHPdoc some sentences end with a point, while others don't. Please try to make this consistent/professional. Otherwise, great work refining the documentation! Rock on. Given the documentation gets some additional love, this appears to be ready.

Changed the documentation a bit.

Committed to CVS HEAD. Marking this 'code needs work' until the upgrade instructions in the handbook have been updated. Set as 'fixed' once this is properly documented and communicated.

Good job, guys.

Added very brief comments to the theme and module converting pages:
Converting 4.7 themes to HEAD
Converting 4.7 modules to HEAD

I wrote a patch to port the same system (core, module, theme) to JavaScript files: http://drupal.org/node/76637.

Which themes does this work in? Tested on? I'm using a customized blue theme
[mostly shopping cart and user membership changes, plus graphics.]
seattleonly.com

I followed the instructions: patch -p0 < css_13.patch

result- site unusable:

Fatal error: Call to undefined function: theme_add_style() in /virtual/seaonly/dev_site/modules/nice_menus/nice_menus.module on line 26

Thankfully I ran a backup before I patched. I now have to reload the entire site.

Thanks :)

democritus: this is a patch for the current drupal developer version NOT for drupal 4.7
You can't use this patch for drupal 4.7

it has no effects on you current site - only if you disabled drupal.css...

You don't have to reload the entire site. Just use the -R (reverse) argument to "unapply" a patch:

patch -p0 -R < patch_file.patch