There are just too many issues when trying to aggregate CSS with regular expressions. As already raised in #494498: Add CSS parsing capability to Drupal we need a real CSS parser!

As we are already in code freeze this probably won't happen in drupal 7 (When this patch however goes in I am happy to work real hard on this one). So we need to fix what is fixable in the current approach of CSS aggregation. And we need to make it as easy as possible to replace core CSS aggregation with a contrib module, which may result in a better version.

I suggest we move current aggregation into a seperate module.
* There already is a hook_css_alter(), so code in common.inc only gets cleaner.
* For users nothing changes but they have to enable the module.
* Performance wise I think this won't make much difference, it may even be faster.
* CSS aggregation isn't really a core task and shouldn't be in includes/common.inc. drupal_add_css() and drupal_get_css() are the only functions belonging there.

So to properly fix all issues involving CSS aggregation, it needs to be moved into a seperate module.

Patch contains a working css.module, check it out. I already changed some of the logic to allow better testing and added some extra tests.

Again, when this patch goes in I am happy to work real hard on a real parser to get killer CSS aggregation.

CommentFileSizeAuthor
css_module.patch66.2 KBcasey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

casey’s picture

Probably too late to get this in anyway, so I created a contrib module for this: CSS Preprocessor. Maybe Drupal 8...

casey’s picture

Title: CSS aggregation overhaulin » seperate module for CSS aggregation
casey’s picture

Version: 7.x-dev » 8.x-dev
catch’s picture

Status: Needs review » Needs work
Issue tags: +Framework Initiative

This is a good idea, we need to stop cramming stuff in common.inc when it's not needed. Tagging but patch will no longer apply.

Spelling Ninja’s picture

Title: seperate module for CSS aggregation » separate module for CSS aggregation
Jacine’s picture

Nice. Subscribe!

catch’s picture

I'm wondering a bit if this couldn't be a class in /includes rather than a module - closer to cache.inc or lock.inc. We could then make it swappable. That's just a thought though - as long as we move it somewhere out of common.inc, that will make it easier to move somewhere else at a later date too if we ever wanted to.

Wim Leers’s picture

Status: Needs work » Closed (duplicate)

Instead of making it a module, it just became pluggable: #352951: Make JS & CSS Preprocessing Pluggable. :)