CSS preprocessor

• Typo: " @import rules must proceed any either style, so" - you probably "any other".
• Typo: "If any modules implement hook_compress_css," - make this hook_compress_css() for linkage when Doxygen is generated for api.drupal.org.
• Inconsistent sentence termination - some comments have an ending period, others do not.
• file_create_path('css/') .'/'. looks funny - wouldn't this cause double slashes?
• "Additionally, this additional HTTP request " sounds retarded. Change to "these HTTP requests".

Note: this review does not constitute my personal acceptance of the patch.

Regarding file_create_path('css/'), you have inconsistent usage - some times you use the slash, sometimes you don't. Pick and choose one, matey.

All I did was fixed the comments to be in sentence form as stated somewhere in the style guidelines. I put about 5 periods in there so I think my name deserves to be included in the Drupal credits sequence please. :P

I still think we need to look at the overall best solution for CSS in Drupal 5, so I've split off the "better conditional css loading" in a separate issue, but with the goal of integrating well with the CSS preprocessor. I.e., make sure there is a good base of common CSS files to cache, but don't include CSS files that are only used once in a blue moon.

http://drupal.org/node/100563

We should evaluate this CSS preprocessor patch with good preloading behaviour rather than what we have now.

This patch is a bit confusing around $filename = drupal_build_css_cache($types); this call: you pass in a variable which at the calling place $types but at the receiver end it's called $css, it should probably be $types as well, it would result in a foreach($types as $type) which reads nice. And also, and in the calling place you have $filename = md5(serialize($types)) .'.css'; and the first thing that function does is $filename = md5(serialize($css)) .'.css';

Also, I would do the whitespace removal before the hook invoke. There is absolutely no reason to not do whitespace removal, if someone has ideas how to compress more, sure.

Patching system_settings_form_submit like this is simply unacceptable. Use form API, define your own #submit handler (you will need to add system_settings_form_submit too because the form API automation won't happen).

subscribing :P

I'll test it out when it needs review.

Small request, could the md5 checksum be checked against only the compressed styles and not everything? Can live with it either way. Thanks.

Core committers: please don't commit this patch until we investigated the conditional CSS loading. See http://drupal.org/node/100563. I haven't look at the latest incarnation of this patch yet, so keep on working on it -- if necessary.

OK, So I did some benchmarking of #100563 and #100516. I used the same method as in my original bug report.
I am posting these results in both places, since they are both looking at fixing the same issue.

Here are the results, see below for explanation:

          HEAD
          Total       Transfer Duration       Page Duration
Test 1    12798       8756                    8454
Test 2    12984       8907                    8162
Test 3    12730       8718                    7966
Average   12837       8794                    8194
Baseline  100%        100%                    100%
                      
          conditional_css_include_2.patch
          Total       Transfer Duration       Page Duration
Test 1    10633       7111                    6828
Test 2    11225       7332                    7530
Test 3    11047       7639                    7357
Average   10968.33    7361                    7238
Faster By 15%         16%                     12%
                      
          cache_19.patch
          Total       Transfer Duration       Page Duration
Test 1    9160        5453                    5175
Test 2    9417        5712                    4961
Test 3    9056        5306                    5290
Average   9211        5490                    5142
Faster By 28%         38%                     37%

• Testing was done locally, to eliminate the variable latency you get on live networks. The Charles web debugging proxy was used to apply a consistent throttle and latency equivalent to a typical 64Kbps connection.
• Browser caching was disabled completely, to simulate an initial page load. I can repeat, with caching enabled (to test http cache freshness checks) if that would be useful.
• Drupal page caching (css caching for #100516) was enabled, and the test was done as an anonymous user. All Drupal modules were enabled (a few of these would more likely be contrib modules in reality). The page cache was cleared before each set of tests, and 2 dummy reloads were made before starting timing.
• The first column 'Total' is the total time spent transferring data, as reported by Charles. This does not include the time saved by browser pipelining of http requests.
• The second column 'Transfer Duration' is the Duration between the first byte transfer and the last byte transfer, as reported by Charles
• The third column 'Page Duration' is the time between the end user hitting refresh and Firefox finishing building the page. This is occasionally less that the transfer duration, which is a little odd, but perhaps certain page graphics (favicons maybe?) are not included in the page build time.

- As you can see, conditionally loading CSS is approximately 15% faster than the current situation, whilst a preprocessing cache is almost 40% faster.

In real time conditionally loading CSS is saves about 1 second from the time taken to initially load the page for the user. This is certainly worth committing in my opinion.

Adding a preprocessing cache saves about 3 seconds from the time taken to initially load the page for the user. This is, in my opinion an enormous benefit (compared to the effort spent whittling 50ms off a database query!) and is well worth the added complexity.

Now that #100563 has been committed (w00t Steven!) we need to update this patch so that conditional calls to drupal_add_css() for the following CSS files use the $cache = FALSE parameter to prevent unnecessary duplicate cache files and downloads (as described in http://drupal.org/node/98819#comment-161026) $cache = FALSE parameter to prevent unnecessary duplicate cache files and downloads (see http://drupal.org/node/98819#comment-161026).

Not cachable (used for only a few pages each):
./misc/farbtastic/farbtastic.css
./misc/maintenance.css
./modules/block/block.css
./modules/color/color.css
./modules/help/help.css
./modules/locale/locale.css
./modules/profile/profile.css
./modules/search/search.css
./modules/tracker/tracker.css
./modules/watchdog/watchdog.css

@param $path is documented as "optional" in the last patch.

Patch fails on me. Parts point to non-core files.

Well, the patch applies cleanly for me on a fresh checkout, seems functional.

Why is cache set to FALSE for the search module? It seems that you only want this for rarely-viewed admin pages. Also, I think the conditional on line 1427 of common.inc should be changed to if (!$cache || !($is_writable && $cache_css)) so that the site doesn't break if /files is not writeable.

revised patch covers the above to concerns.

Also, if this goes in, it seems it should include a change of this admin menu item to something more comprehensive:
Page caching
Enable or disable page caching for anonymous users.

Finally, as best as I can tell from the code, this compressor run the md5() on all the CSS for every page view, right. Even for cached pages?

@pwolanin the search module css (and any conditionally included css) must not be cached, because otherwise it triggers generation of a complete new cache for the browser to download. See http://drupal.org/node/98819#comment-161026 for a description of why this is a problem.

-> I wonder if there are things we can do to make this easier to grok/code for contrib module developers? The way it is now, any modules conditionally including CSS will, until they are fixed by the maintainers, break the css caching (perhaps damage is a better word), by forcing a complete redownload of all the cached CSS.

Perhaps we could $cache be false by default, or (if we really want it to be true by default) we could do something like I suggest in http://drupal.org/node/98819#comment-160866 to make the collection of css files a separate (and more proactive) stage than leaving it up to modules to make sure their drupal_add_css() calls only set $cache = true where appropriate.

I thought I would do some quick testing to see how gzip compression would affect our performance and work with this patch:

          conditional_css_include_2.patch AND cache_19.patch
          Total       Transfer Duration       Page Duration
Test 1    9595        5615                    5333
Test 2    9909        5709                    5425
Test 3    9461        5824                    5537
Average   9655        5716                    5432
Faster By 25%         35%                     34%

          cache_19.patch AND gzip
          Total       Transfer Duration       Page Duration
Test 1    7429        3495                    2741
Test 2    7027        3306                    2758
Test 3    6915        3267                    3001
Average   7124        3356                    2834
Faster By 45%         62%                     65%

          conditional_css_include_2.patch AND cache_19.patch AND gzip
          Total       Transfer Duration       Page Duration
Test 1    7489        3157                    2395
Test 2    6699        3107                    2364
Test 3    7250        3013                    2255
Average   7146        3092                    2338
Faster By 44%         65%                     71%

Note that these percentages are compared to the times in my original HEAD benchmark (with no patches) at http://drupal.org/node/100516#comment-162025
Please read the notes on that comment if you have not done so already. Also note that all the conditional css includes patch is doing here is somewhat reducing the cached css size, because none of the conditions are met on the front page.

Here is a summary:

Test Set                     Faster By:  Seconds Saved:
HEAD                         100%        0
conditional                  12%         1
caching                      38%         3
conditional + caching        34%         3
caching + gzip               65%         5.3
conditional + caching + gzip 71%         5.8

Looking at this I am actually wondering if we should be looking at including gzipped css (and js) in core - if not for 5.0 - then certainly in 6.0. While the percentages are skewed by the fact that my test page is pretty lightweight (i.e. not much content, and no user added images) the seconds saved are real, actual seconds that would be saved by a user on a 64Kbps connection, and would be saved no matter what is added to the site and theme in the way of content and additional images. Also note that the conditional includes patch has now been committed to HEAD, but wasn't when I did my initial benchmarks - I am keeping it separate here for easier comparison.

Bearing in mind the statistic that most users will only wait 4 seconds before going to a different site, the application of aggregation/caching and gzip can take the initial page load time from a very poor 7 or 8 seconds, to a very respectful 2 seconds. The difference between these patches is extremely noticeable, the site goes from feeling pretty sluggish to appearing extremely fast.
At the very minimum we should IMHO make sure that it is possible for a contrib module to provide gzip compression.

There are, I think, 3 different ways of serving gzip css to the user that could be used:

• Saving a separate .css.gz file (which could be done using hook__compress_css) and adding a rule like:

    RewriteCond %{HTTP:Accept-encoding} gzip
    RewriteRule ^(.*).css $1.css.gz [L,QSA]
  

  to .htaccess to transparently redirect browsers that accept gzip to that CSS. This approach was the one I used, but is somewhat limited because of the inability to tweak the http caching headers and detect/work around any browser http quirks (I am not sure how much of an issue this really is in practice - any ideas?).

• Detecting if the browser accepts gzip, and if so adjusting the @import declaration to point to the gzip. I think this would cause problems with our page caching system, because the detection would no longer work correctly.
• Adjusting the @import to point to a short php script (totally outside of the Drupal bootstrap), which detects whether the browser accepts gzip or not, and loads the appropriate file. This is more flexible, because it can tweak the headers and work around any browser http quirks. I think it would be worth passing the $filename parameter through hook__compress_css, to allow contrib modules to redirect the css request through their own script.

Just for the record: my concerns are address, I am content with the code as it stands.

Excuse me, was trying to patch 5b2. Patch works perfectly with head.

@Grugnog2, that's amazing!! But should we not complicate this issue? 40% is great and anything more complicated might be better left for a contrib module for now. Thanks for the benchmarks.

I've tested it and so far as then end user goes, it works.

@Grugnog2 - why not always include the search CSS then? I guess I'm a little confused as to when the CSS should be included in a page-specific manner, and when it's always included.

That said, I think it's essential to change the conditional per #23.

Also, does this all break with private files turned on (as I think Garland does)?

gzipping sounds like a very good idea here - but maybe it is better to open another issue for that

Yes, just confirmed- I can break this code with private file downloads when the files directory is properly setup to not be accessible via HTTP. Looks like essentially the same bug as here with color picker: http://drupal.org/node/92059

I'm not sure if this is too much of a hack, but attached now prevents the use of the CSS pre-processor/cache if private downloads are enabled, and also prevents the site from breaking if the files directory is not writable.

Also, tweaked the settings form to disable the form element if necessary (system module line ~677):

  $public_downloads = variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) == FILE_DOWNLOADS_PUBLIC;
  $is_writable = is_writable(file_directory_path());
  $form['cache_css'] = array(
    '#type' => 'select',
    '#title' => t('Cache and compress CSS files'),
    '#default_value' => variable_get('cache_css', FALSE) && $public_downloads && $is_writable,
    '#disabled' => !($public_downloads && $is_writable),
    '#options' => array(t('Disabled'), t('Enabled')),
    '#description' => t("A lot of Drupal modules include their own CSS files. When these modules are enabled, each module's CSS file adds an additional HTTP request to the page, which can slow down the loading time of each page. These HTTP requests can increase the server load. This option is disabled if you have not set up your files directory, or if your download method is set to private. It is recommended to only turn this option on when your site is in production, as leaving it on can interfere with theming development."),
  );

color.module also disallows recoloring if private files are on. This is an acceptable fix for 5.0, which will be improved in 6.0. I don't see why we can't do the same here.

Well, some of us are forced to use private files in some cases which means we won't be able to use the CSS preprocessor if we solve this by simply disabling the feature. That would be too bad, but I guess it's the least bad solution for now, or?

Seems the only real good solution to this is to add a new file store for all UI related dynamic files, as described by Steven here (suggestion #1).

In whatever code is committed for this (and similarly for JS) I think it's essential that common oversights/mistakes /misunderstandings by users (like not making files writable, or turning on private downloads without testing first) don't ever break the entire layout of the site. That was my goal with the changes in above patch relative to the prior patch.

I have created a patch (which applies on top of this one), to provide gzipped CSS using the aggregated CSS.
Please take a look: http://drupal.org/node/101227

Works as advertised. ... I just improved help text a bit and reorganized page cache settings into own fieldset ... RTBC

I did a prototype implementation of a contrib module to gzip the CSS. I needed to make the following changes (included in the attached patch, which includes Moshe's fixes):

• hook__compress_css() was not actually allowing contrib to do any practical compression AFAICS. I added the filename as a parameter, to allow my prototype module to save a .css.gz copy of the file. I also made $data pass by reference, to allow modules to perform other textual compression (aggregating CSS selectors, etc) if they choose.
• I also added hook_css_cache_redirect() which allows modules to adjust the location of the cached CSS file, which can be used to insert a handler to send the gzipped file with the correct headers (if available).

Please check out my prototype implementation at http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/grugnog/gzip_... to see how this is all wired together (this is not part of this patch, however!).

Let's make sure our hooks allow us to do something useful before we commit this!

@gregnug2 - that crossed my "too many new hooks" threshhold ... by compressing more and redirecting you are basically creating a new css cache handler. i suggest making a single hook where you can override default css cache handler. you might refactor part of the current css cache so the alternate handler can reuse logic from current handler as needed. hope that makes sense. but really, if we don't stop tweaking this thing it won't get in.

IMO, the patch in #37 is RTBC.

I have done some refactoring:
-> After discussions with Moshe both hooks are now gone, replaced by a 'single hit' custom_build_css_cache() override in the style of custom_url_rewrite()
-> All the 'file related' code now lives in drupal_build_css_cache(), which simplifies things because we no longer create & check paths in 2 places

I would be happy if we committed #37 (sans the useless hook), so I am leaving as RTBC - I can put my changes to another issue if required.

However, if we have time for a couple of people to review this version that would be great. I think it is a bit more elegant overall - and the big plus is that it allows a contrib module to provide gzipping - I have updated my prototype code too.

@grugnug2's patch is a nice improvement. works well. the last patch, #40, is now RTBC. I hope we get Dries/Steven/Neil to available review it.

-1 on the hook to add gzip-support through a module. That's just cruft. gzip-support should be build into core, and shouldn't require an additional module.

The url 'page-caching' is technically no longer correct. It should probably be renamed to just 'caching'.

What is the technical term for this feature? Is it 'CSS caching' or 'CSS preprocessing'? The terminology used in this issue, and the terminology used in the patch don't seem to match.

Making this a generic framework that works for JS files as well as CSS files makes sense to me.

I'm tempted to set the category of this patch to 'feature'.

I'm tempted to set the category of this patch to 'feature'.

The additional hooks that've been proposed in more recent patches may be features, but the core patch here is indeed a fix for a serious problem that 5.0 introduces. The slowdown that comes from the proliferation of CSS files is a bug at the conceptual level rather than the code level, but it's still a bug, IMO...

-1 on the hook to add gzip-support through a module. That's just cruft. gzip-support should be build into core, and shouldn't require an additional module.

Are we prepared to commit gzipped CSS in 5.0? If so I am happy to add my code to this patch. However, I don't think it makes sense to add this if it will delay the patch past 5.0. The lack of CSS caching is the bug here, that needs to be fixed for 5.0 - gzip is just a free bonus.

The url 'page-caching' is technically no longer correct. It should probably be renamed to just 'caching'.

Moshe did that already:

-      'path' => 'admin/settings/page-caching',
-      'title' => t('Page caching'),
-      'description' => t('Enable or disable page caching for anonymous users.'),
+      'path' => 'admin/settings/caching',
+      'title' => t('Caching'),
+      'description' => t('Enable or disable page caching for anonymous users, and enable or disable CSS caching.'),

What is the technical term for this feature? Is it 'CSS caching' or 'CSS preprocessing'? The terminology used in this issue, and the terminology used in the patch don't seem to match.

Not sure here. Aggregation is another possible term, but perhaps incorrect.

Making this a generic framework that works for JS files as well as CSS files makes sense to me.

I suggested this three issues back. It definitely makes sense to do this long term - but not (IMHO) at the expense of getting the patch in to 5.0. JS files are not numerous enough to cause latency issues, therefore are not really a 'bug' in 5.0.

That said, I don't think it would hurt to experiment with this and, if it looks easy to run with it. However, lets keep the CSS+JS patch as a separate 'branch', so if it looks too complex we can still go back and commit a working CSS preprocessor.

I'm tempted to set the category of this patch to 'feature'.

Gzip support and JS caching are both features (strongly usability related however).

CSS caching however is a bug. Serving multiple module CSS files adds 2-3 full seconds to our initial load, this is just unacceptable IMO!

CSS preprocessor seems to be the most meaningful description.

I agree with Dries that applying this to JS in the future is definitely an option, but not suitable for 5.0.

I also agree with Dries that this patch should be as good as it gets. If we can add gzipping cleanly, then by all means, let's do it. The extra hook is unnecessary and the implementation is plain weird.

@grugnog: that looks like two endorsements for you to reroll with gzip built into this patch.

without gzip, our css file gets served by directly by apache, and no php is used. with gzip, php fires up, drupal bootstraps, we issue a gzip header, and then deliver the file (through php). you guys sure you want gzip in the core preprocessor?

Does serving .css.gz files with <style type="text/css" src="whatever.css.gz"></script> not work?

Found this link: http://www.phpbuilder.com/tips/item.php?id=1128
Shows how to serve gzipped CSS files using a little PHP script plus apache rewrite.

Given the availability of tricks like this, plus the fact that many servers can compress css or html files before sending them (e.g. mod_gzip), is there really a need to handle gzip in core?

@all - please look at my prototype gzip handler at http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/grugnog/gzip_... - this is what we are talking about including (after I clean it up, and add the module code to the patch).

This would involve the addition of some kind of handler, which sends the gzipped CSS out with the correct headers (and can also eventually be used to JS etc), and a setting which causes the file to be additionally saved as a gzip and the url to be redirected through the handler.

@moshe - we do not (AFAICS) really need a Drupal bootstrap just to serve the gzipped CSS - this can be a wholly independent php file (send_gzip.php in my prototype - which is added to the site root - or perhaps misc?). If the implementation of the same efficiency can be made with a fast bootstrap, then perhaps that is a more elegant option - but the advantages are small.

@steven - serving .gz files directly kinda works, but we need to decide what to send (gzip vs plain) on a per-client basis, so page caching might be tricky - I am also not sure that it serves the correct gzip headers. We could do some more tests if you think this option is worth pursuing.

Unless anyone objects, I'll have a go at rolling a gzip patch tomorrow. Please add any suggestions I should take into consideration!

I still feel we should push the JS caching off to 6.0, unless a really good reason to include it appears!
@pwolanin send_gzip.php is just a Drupalized version of http://www.phpbuilder.com/tips/item.php?id=1128. Using .htaccess is fine by me, but is not exactly user friendly!

ted, steven, and i agreed on irc that there is no good way to do gzip in core, and especially not for this release. so alas, that hook is removed in this patch.

i also reworded so that the page that this pref is shown on is called 'Performance'. On it are this preprocessor pref and the page cache prefs. i don't use the word 'preprocessor' in the UI becuase its bad geek speak. The pref is titled 'Aggregate and compress CSS files' ... Committers are free to wordsmith as desired, or send it back to me, your humble text monkey.

Remember to clear menu cache and reenable this pref as names have changed.

There is a problem with this patch.

Suppose you have 4 stylesheets: A B C D, where C is not cachable. The normal inclusion would be A, B, C, D. With the cache, we end up creating a single stylesheet that contains [A B D] followed by C. This means the CSS cascade is now broken or changed. In practice, this would mean that a module.css file that is not cachable, would come after a theme's style.css. This would mean things are unstylable and a bug.

But, we can't just split up the cache around non-cachable CSS files, because that would greatly reduce the efficiency of the cache. e.g. normally, "A B D and E" would be cached into [A B D E]. If we add C.css, suddenly new caches for [A B] and for [D E] must be served.

Because we really need to keep themes cachable, there is only one solution. To make it so that module.css files that are not cached, are always placed first, before everything else. This means the theme can still override. The only difference with what we have now, is that 'core' styles would override module styles. This isn't really a problem, because the core styles have very low specificity and are very generic. If a module specifies a style with an id, class or tags, it will definitely override core.

Sorry, that was phrased badly. Correction:

Because we really need to keep themes cachable, there is only one solution. To make it so that module.css files that are not cached, are always placed first, before everything else. This means the theme can still override. The only difference with what we have now, is that 'core' styles would override uncached module styles. This isn't really a problem, because the rule that later styles override earlier one still has to respect the CSS cascade. Core styles have very low specificity and are very generic. If a module specifies a style with an id, class or tags, it will definitely override core styles, regardless of its placement in the loading order.

looking at file.inc, there should be a check for is_dir() as well as is_writable().

Patch does not address Steven's concern, but builds off moshe's and could be used as a basis for the next iteration.

Well, there is no "core" stylesheet anymore, really, is there? There's just module stylesheets that just happen to be in core. Since modules shouldn't be colliding in the first place, their order shouldn't matter. Always sending uncached stylesheets first sounds like a reasonable plan then.

Just a quick comment (dinner is served, no review): I agree with the recent decisions being made (i.e. removing the gzip-stuff, renaming the page/link to 'Performance', dropping the 'core' and making sure cascading continues to work). I'll give it a better review later on though. Keep up the good work.

I admit I am jumping in late on this topic. I didn't know until yesterday that this was an issue being addressed by m3avrck and others. I noticed that all of these calls for CSS files were largely caused a bog down of processing and creating alot of file bloat. I think there is a processing savings to be had from putting file processing and file saving into the mix and removing the 15-30 request-responses that are going on per page.
In our project, we need a lot of custom functions and I started to build up a "comminit.module" (comminit-- short for Communication Initiative). In there, I put in a module to compile CSS and JS code. This code is executed in the theme in lieu of the standard way a theme outputs it information.

Program flow:
- build a file name from the $_GET string and the $theme. ($temp_name)
- look to see if a temp/html/ $temp_name.html file exists.
- if not
- read from the $head what CSS and JS files are being called
- load each of those into either a $css or a $js array.
- if there are one or more $css items, then note that $stored_head_html array.
- if there are one or more $js items, then note that $stored_head_html array.
- write the consolidated $css and the consolidated $js files to subdirectories in a temp directory

- load/use the html file from the temp directory. It, in turn loads two files-- one CSS and one JS file.

First time a page-- or type of page-- is loaded, there will be a lot of processing overhead; second time, it will use the static HTML file and its static CSS and JS files.

The problems:
- this will create a lot of temp files and I have ways to reduce that a little (e.g. reference numbers from the path so that similar part of the page are considered the same way).
- I have it implementing at the theme level-- not ideal for a widespread solution,

I am attaching a the comminit.module as it currently exists.

This is an example page.tpl.php that calls it.

<?php
	$get = $_GET;
	if ($get == null) {
		$get = "_";
	}
	if (is_array($get)) {
		$get = implode("_",$get);
	}

	if (comminit_file_exists(base_path().'/temp/html/header_'.urlencode($get).'.html')) {
		print file_get_contents(base_path().'temp/html/header_'.urlencode($get).'.html');
	}
	else {
		print comminit_get_html_head(array($head,theme('stylesheet_import', base_path() . path_to_theme() . '/style.php', 'all'),$styles));
	}

	/*
	print $head;
    print theme('stylesheet_import', base_path() . path_to_theme() . '/style.php', 'all'); 
	print $styles;
	*/
  ?>

This is not ready for Prime-Time, so please just consider it an addendum to my post.

All the best,

Mike DeWolfe

Patch under #57 works beautifully! I've tested it the best I could. Thanks m3avrck for adding the unprocessed theme order!

It looks RTBC but the only minor thing I'd add is a link to the 'file system' page when it's not already set so it points users in the right direction. But that's very minor at this point. Just get it in there and I'd be happy to pull a patch to add that.

I'm having bad luck with the patch:
- using three Windows apps (patch, CGIwin and TortoiseSVN) on my local copies of my files
- a couple attempts to files on my Linux box

all of these hang when attempting to apply to patch from #57.

C'est la vie. I'll wait until these are committed.

All the best,

Mike

The latest set of changes look good to me - +1 RTBC!

As I said before - despite liking the potential for gzip I don't think it makes any sense for gzip delay this patch. It will still be easy enough for a contrib module to use a .htaccess rule to redirect cached css to a php handler. Hopefully by 6.0 this will all have been well tested in production and we can expand to include JS too.

patch in #57 also fails for me against clean checkout of HEAD.

CVS error messages:

patching file includes/common.inc
Hunk #1 succeeded at 1383 (offset 21 lines).
Hunk #2 succeeded at 1421 (offset 21 lines).

patching file modules/profile/profile.module
Hunk #1 succeeded at 422 (offset 12 lines).

patching file modules/system/system.module
Hunk #1 FAILED at 25.

patching file modules/watchdog/watchdog.module
patch unexpectedly ends in middle of line

applies cleanly, works as advertised.

I can't stand the wait any longer, what's happening with this patch? :)

Verified to be working. CSS inclusion order is consistent and useful, and the caching works fine. There is a noticably faster load time everywhere. The settings page is clean and consistent.

Good job everyone! Committed to HEAD. Woohoo. 2 down, 2 to go!

Perhaps an offtopic, but searching for this revealed no results. There is quite a few module with JavaScript addons, are there plans for a JavaScript preprocessor of the same sort?

Im not certain that a JS preprocessor will be provided for Drupal 5 but definately for Drupal 6 as more and more modules take advantage of JQuery.

I had another question of my own actually. Would this script work if my theme has a style.php rather than a style.css?

Update: I've tested this, and it discarded all CSS from my style.php

So I've had to turn preprocessing off for this individual file but the rest works as advertised.

BioAlien: try to add a "style.css" which has @import url(style.php) in it...
maybe this works...

Have a look at this solution, I think it may be more elegant than the work that was committed... just an idea.
http://rakaz.nl/item/make_your_pages_load_faster_by_combining_and_compre...

I understand it might have been somewhere earlier in the thread, but wonder if there is a chance of a 4.7 backport...

Those who use *lots* of contributed modules are hit by a double-whammy. On one hand many modules mean many files loading. On the other hand this means it will be a long time till everything ready to move to 5.0.

So could anyone provide any direction on how to solve this problem in 4.7?

Setting the status to 'active'. Did not realize 'active (needs more info)' does not show up on "my issues" lists...

I am not opposed to hacking every module that provides CSS/JS if the hacks are not very extensive (a handful of lines). This is much better than having your production site slow to a crawl.

If this can be done, would greatly appreciate any help.

An easier method than backporting this patch to 4.7 is simply to manually aggregate (and, optionally compress) all the module css into one big file and include it directly in your theme. You could then hack the multiple css includes out of $head either using a regexp, or my nixing the appropriate lines in Drupal core.

I am already using this method to eliminate the smallest CSS files. However, this seems too risky and too hard to maintain for bigger modules. Whenever modules have to be updated or themed the number of patches to manage could get daunting.

Hope some limited version of this pre-processor could be made available for 4.7...

It is a fair request for a great feature, but to get this feature into D5 took 2 or 3 threads with hundreds of responses, and an unknown multitude of IRC conversations.

As Grugnog2 says, it is easier for you to do it manually.

Setting this back to fixed as per the original issue, please open another ticket if you'd like to try further. You might be lucky and a talented dev will provide a patch here, but my advice is that's unlikely.