Allow CSS files to be removed from queue

Somehow I thought one could do this already in 5.x, but don't see it now.

Anyhow, this doesn't seem like quote the right approach, since you need to be able to do this after all modules/themes have added their CSS, right?

Maybe the approach that drupal_add_js() is using could be of use for you?

This is an API change so I would call it a feature. It needs to go in the development version and will not hit the stable 5.x versions.

The patch seems to only really handle the case of media == 'all'.

I'm not sure what the possible concerns are of having such code in template.php, but that's also not a good way to handle this- not all themes have such a file that's included.

One possible approach (requires a module) to this would be to add a call to drupal_alter to drupal_get_css():

function drupal_get_css($css = NULL) {
  $output = '';
  if (!isset($css)) {
    $css = drupal_add_css();
  }
  drupal_alter('css', $css);
  $no_module_preprocess = '';
  $no_theme_preprocess = '';

...

If you want themes to be able to do this - that's harder. The principle of theme functions is to return HTML, not manipulate data. however this seems like a case where something like theme('alter_css', $css) would be justified?

This could be called in function template_preprocess_page()?

$variables['styles']            = drupal_get_css(theme('alter_css', drupal_add_css()));

As shown above. A simple one line patch to let modules alter the css queue before it's rendered.

+1, but it seems like you'd also want to allow the theme to alter it. In fact, it's much more valuable to let the theme alter it, I would think.

+1 from me on this one too... I agree that this should be possible at theme level, it's where it's needed the most I would think. It's important from a performance POV too since instead of overriding CSS, themers could disable modules' CSS files and replace them entirely with their own.

After discussion with merlinofchaos in IRC, I went ahead an implemented the part to allow the theme to alter too. This diverges a bit from the normal use of theme functions (it has to return an array) but that seems to be the only reasonable way to let themes do this. I tested both parts of the patch. With the latter part you can make your site look ugly by putting in Garland's template.php:

function garland_css_alter($css) {
  unset($css['all']['module']);
  return $css;
}

yes

+1

Just quickly adding that this functionality will be extremely useful from a themer's perspective.

The negative, of course, is that this is not particularly intuitive for a non-coder. But that is a wider and more difficult problem with theming that shouldn't hold up this commit.

hmm, not sure how that happened. Changing back the status.

I checked out the latest code from cvs head, applied the patch with no problems. I ran two tests.

1. I created a custom module with a hook_css_alter, enabled css aggregation, modified the css differently for different nodes, and confirmed that the correct css was written.
2. I then ran the same tests with theme_css_alter and confirmed that the correct css was written. The only confusion I ran into is that the themes are now cached (not related to this patch) and I had to clear my cache to get it to recognize my new theme functions.

+1

improving the title

This works fine for me, and complements this patch wonderfully. (That one adds CSS, this one removes it.) My only problem is the name. theme_* functions are expected to behave in one way. *_alter functions are expected to behave in another. theme_css_alter behaves like... which? My first guess was like an alter hook, but it's actually a theme function. That's confusing.

I'd prefer a name other than "alter_css" for the theme function to avoid confusion with the alter workflow. Otherwise this is ready to go.

Like I said, it compliments this patch. *sigh*

+1

I found some workarounds that works well within themes but this would simplify that.

As the write of the original patch that added drupal_add_css() to core I don't like this approach.

The cleanest approach to do this is to create your phptemplate_variables() and add:

     $new_css = drupal_add_css();
     // removed unnessecary CSS styles
     unset($new_css['all']['module']['modules/poll/poll.css']);
     unset($new_css['all']['module']['sites/all/modules/contrib/cck/content.css']);

     // rebuild CSS
     $vars['styles'] = drupal_get_css($new_css);  

This patch doesn't really make that process any simpler and adds more overhead IMO.

I think this approach is less overhead - since you alter the css array before the files are loaded and aggregated. With your appreach, won't every file have to be loaded again?

It's just manipulating an array with that code and then it rebuilds the CSS cache.

I agree -- it's not the best, but I don't think this method here makes it any simpler... or maybe I'm reading it wrong (perhaps too tired ;-))

Two issues have been raised (comments #16 and #19) which need to be resolved before we can say this is ready, so I'm pushing this back to 'needs work'.

I'm totally in favour of this feature, but I'm no longer convinced this short term solution is a good response to the problem.

Re: solid ground (#23): I'm not sure this is true. The implementation of this looks like a clever hack to me, and I'm not sure it's really something we can build on. Unless we are gaining something substantial from this in the short term, I'm against it. This is part of the API that themers will have to learn, and I don't want them to have to unlearn something if we can avoid it. This just feels like we are rushing in the hope of getting it into Drupal 6.

I personally think the sister patch (http://drupal.org/node/141725) has a good strategy for this kind of thing. Using the .info file is not only easy, it is arguably easier than coding it with plain old HTML. This is a very positive move. The more theme related stuff we can take out of PHP functions the better.

To avoid a ping-pong match I'll leave the status alone.

Looks like this has hit the code freeze.

I suggest we first make sure there is a clear workaround documented in the handbook and then postpone this for d7.

Hopefully by then I'll have be more comfortable with the core code, so will actually be more useful ;)

A clarification - I thought that naming the theme function using the same pattern as the _alter function would simply things - but perhaps not. The theme function could be named anything (e.g. theme('ihateothercss', $css)).

Past code freeze.

@sun - well if you feel strongly enough that it's a bug, change the version back and make your case.

Here's a shot at it -- it's definitely kludgey, but it could be a real boost for CSS-only .info file driven themes that just want to, say, turn off all of Drupal's default css files and control stuff like ULs and such themselves.

I assume Eaton is still hoping for 6.x.

Anyhow - I don't think this code is good, since themes cannot predict the path where contrib modules may be installed.

Perhaps a simpler patch could provide a mechanism for removing only core CSS or core required module CSS? Or replacing them rather than removing them?

Anyhow - I don't think this code is good, since themes cannot predict the path where contrib modules may be installed.

Well, that's one of the reasons it's doing a string comparison -- if you put in modules/system/system.css, it will find that anywhere there's a modules folder. Your point stands, though. I'm not particularly HAPPY with it, but it's cleaner than any other method I was able to come up with.

It is a relatively small change. If it (or any similar improvements) go in, it should go in under the principle that 'anything that prevents users from making a reasonably simple CSS-only theme, without writing PHP code, is a bug in Drupal 6.'

On pwolanin's suggestion, I'll avoid hijacking THIS issue thread and point directlly to the more 'focused' issue: http://drupal.org/node/163785. Thanks.