Import @import commands when building CSS cache

Here's how the Zen theme solved this same issue: http://drupal.org/node/113184

I am working on a patch to move the CSS (and JS!) API to a params array setup. I am thinking of having an optional weight parameter.
This could inherit some sensible defaults based on the execution state of Drupal (e.g. core -10, contrib 0, themes +10), but then be overridden if required.

The CSS processor should just include the imported stylesheet directly when generating the cache. That way, no re-ordering needs to take place. Color.module already does this.

It really makes sense that the CSS cache grabs /all/ CSS, even imported :).

Patch attached:

• Based on the code in color.module, I added a drupal_load_stylesheet($file) that loads a file and imports all related stylesheets. Code reuse++.
• I fixed a bug where @import commands with url() would not work in color.module.
• Because I had to alter the path prefixing anyway, I added a quick snippet that gets rid of "../" segments in paths where possible. For example, before, we had paths such as "/drupal/themes/garland/minnelli/../images/bg-content.png" and "/drupal/modules/system/../../misc/menu-expanded.png". These are now much shorter.

I don't have time to test this patch, but a quick review, it looks good. Fixes a number of related issues :-)

The patch looks nice, but I think we need to improve upon the PHPdoc comments.

1. I'd better describe the difference between drupal_add_css().
2. "resolve CSS imports" is too vague -- I'd describe the behavior as well as provide an explanation as when to use this function.

patch fixed my theming problems (with my own theme) in 5.1

Thanks!

This fixes another bug I was about to post.

The current aggregator wrongly includes @import when formatted as:

/*
@import "debug.css";
*/

While this is obviously a problem that can be worked around by simply removing the @import. Even so it is a bug. This isn't the expected behavior and can cause problems on sites and completely break themes. (scared me to death when I enabled it on a live site only to see my debug primary red and green divs).

A big +1 to committing this and hopefully a +1 to backporting this to the 5 branch based on the fact that it fixes both erroneous behaviors will being backwards compatible.

subscribing...

@Grugnog2: i already asked in 5.0-beta1 for a weight feature and got a "by design" only :-((( - see http://drupal.org/node/92877. i will be very thankful to see this feature implemented.

I think this moving up @import's is a critical 5.x bug and should be fixed long time before 6.x is out... my theme is broken with aggregation, while it makes much use of cascading features and i need to keep the correct CSS order intact.

Additional what i really missing in current aggregation, is a logic that follows my relative *internal* @import path's. In this case it would be possible to not loose the cascading order and additional have *all* files aggregated and not only the style.css that have only one @import in my theme. Let me say i have around 20 nested CSS files in a very modular structure.

This patch is buggy and not working. i have an import inside my themes "style.css". This one will be moved to the top of the aggregated CSS file. therefor the overwrite of drupal core styles isn't working. i thought this has been fixed with this patch!? See the following optimized CSS file as an example what happened (shortened in the middle).

@import url(/drupal5/sites/all/themes/yaml/css/layout_3col_fixed.css);.node-unpublished,.comment-unpublished{background-color:#fff4f4;}.preview .node,.preview .comment{background-color:#ffffea;}#node-admin-filter ul{list-style-type:none;padding:0;margin:0;width:100%;}#node-admin-buttons{float:left;margin-left:0.5em;clear:right;}td.revision-current{background:#ffc;}.node-form img{height:1.2em;}}.localizer-flag{margin-right:3px;}#block-localizer-translation a.active 

...

.localizer-flag{filter:Alpha(opacity=50);-moz-opacity:0.50;}@charset "UTF-8";

Additional this shows another BUG. In my style.css is a

@charset "UTF-8";

in the first line. this line is added to the last line of the aggregated CSS and therefor ineffective... this one must be added in the FIRST line of the CSS file and not on the end!

found another bug. If an import is excluded with

/* @import url(test/test.css); */

this line is not ignored. the commented line is added to the top of the file.

using the solution provided in http://drupal.org/node/113184, there comes up another bug:

22.04.2007  13:56            24.645 abc964bfb6cddde5dea9d7df5edaf880.css
22.04.2007  13:56             7.162 ee7538134497c98f3fb0bff723c94135.css

as you can see there are two files generated in my files/css folder. the "abc964bfb6cddde5dea9d7df5edaf880.css" is linked inside HTML output. the other file is not linked. i don't know why it is created, but the smaller file looks like a part if the bigger one.

i reported this @charset "UTF-8"; issue and yesterday one user asked me why my themes are broken with Safari and CSS compression activated.

I found out that Safari starts to ignore everything after a @charset "UTF-8"; declaration if not placed in the first line. After i removed the 6 lines of @charset "UTF-8"; Safari browser is working as expected.

This is very critical, while i'm unable to remove the @charset "UTF-8"; from code. If i do nevertheless, Eclipse is unable to detect the UTF-8 and all on German comments in the CSS files get broken. So it is not wrong to have the charset inside the file and therefor Drupal should remove the charset line or move it to top of the CSS as first line, what may be the best.

hass' problems probably stem from the fact that Steven's patch was not recursive. That's very easy to resolve though, I replaced the file_get_contents in _drupal_load_stylesheet with a drupal_load_stylesheet. I also added ample comments as per Dries' request.

@chx: have you fixed http://drupal.org/node/113607#comment-232494 in this patch?

Nope. That'll be a different issue. Still this is a useful fix.

patch no longer apply. this is a rebuild... please test!

Additional it removes the following @import regex scan, after this refactored build_css_cache function have been replaced all import's and we no longer have any @imports in the files and therefor we don't need to move any import on top of the files. So we can savely removed this for speed purposes.

    // Per the W3C specification at http://www.w3.org/TR/REC-CSS2/cascade.html#at-import,
    // @import rules must proceed any other style, so we move those to the top.
    $regexp = '/@import[^;]+;/i';
    preg_match_all($regexp, $data, $matches);
    $data = preg_replace($regexp, '', $data);
    $data = implode('', $matches[0]) . $data;

Tested the patch and it isn't working as expected. The compressed CSS file contain the @import's.

looks like the following line do not find the imports and therefor never call _drupal_load_stylesheet. I have no idea what's wrong with this regex.

// Replace @import commands with the actual stylesheet.
$contents = preg_replace_callback('/@import\s*(?:url\()?["\']([^"\')]+)["\']\)?;/', '_drupal_load_stylesheet', $contents);

Ok, found the bug and hopefully corrected correctly. The regex have had two missing question marks... url's might not have single or doubles quotes around! Additional moved the charset fix for Safari to drupal_load_stylesheet().

I have tested this bugfix with different url import's and charset declarations in all files with a theme that add's ~15 CSS files and it looks very good now. I'd like to please someone with more complex theme to verify this or review this patch.

@charset "UTF-8";

@import url('css/modules/example1.css');
@import url("css/modules/example2.css");
@import url(css/modules/example3.css);

Here is a backported patch for D5.

I think we get hurt by the bug http://drupal.org/node/139117. I saw some strange things in files that are correct and this is caused by the referenced bugreport. Addition i remember some discussion about speed and regex caching regarding my charset fix already committed. All this in mind i moved the "Perform some safe CSS optimizations." into the drupal_load_stylesheet() to make sure the bug #139117 won't get bigger and bigger with every comment in a imported file.

So, i have new patches...

that's it.

"Color Module: Don't touch" is broken...

New Patch for D6. This fixes the bug from last patch.

Additional i'd like to please all to apply the patch in http://drupal.org/node/175121, too. The patch in #175121 depends on this patch must get in for color module RTL support.

If someone knows how to fix the following error, we can solve this small notice, too. This notice has already existed before our patch... so different bug.

notice: Undefined offset: 1 in modules\color\color.module on line 307.

I think i've got a good idea how to solve a general bug in color module (only style.css is used for rewrite). So i have overhault my last patch a little bit. I hope this get's all your love and we can fix this CSS aggregate and compress bugs together with the refactoring of color module that allows us to add RTL support very easy (~6 lines only).

To make testing easier i'd like to sum up what this bugfix includes:

• CSS Aggregate and Compress feature does not allow us to have @import commands inside. If @imports have existed, they have been moved in the compressed file on top and therefor break the cascading order of css. What is does here is simple, we load every css files step by step recursively. Every time we find an @import it is replaced with the content of this linked file. In this way we keep the order inside the compressed file 100% intact. This is fixed with this patch.
• By this change we are made to fix color module to use a new logic for loading their files for rewrite. I took over the great ground work from chx made in drupal_load_stylesheet() to _color_rewrite_load_stylesheet. The required difference in this function does not remove all comments and allows us to do different optimizations to files processed by color module and keep the comments intact for color modules split "Color Module: Don't touch".
• As a side effect - the color module give all themers the ability to reuse the color module functions for their themes
• ...and finally made it possible to add RTL support - see http://drupal.org/node/175121 for a follow up patch.

How and what to test:

• Add different type of @import rules to your css files and review of they are compressed together and in the correct order. I have tested @import url('css/modules/example1.css');, @import url("css/modules/example2.css");, @import url(css/modules/example3.css); and everything was correct. The outstanding @import "css/modules/example3.css";, @import 'css/modules/example3.css'; will only test of the regexes from chx are correct, but i believe - they are correct.
• You can or cannot add @charset "UTF-8"; on top of the files. Make sure the resulting compressed file only have no @charset "UTF-8"; inside.
• Test the above with recoloring and if Garland will recolor after your changes :-).

I'm sure all will now work like a charm. I have tested all this above together with the follow up patch and Themes containing LTR and RTL styles and ~20 recursive @import's rules with success.

New patch that fixes:

1. absolute path's like "/drupal6/sites/all/*" are not going to be broken by build_css_cache
2. deep nested CSS files with relative url's inside are no more broken
3. RTL support integrated #175121
4. Page load performance optimizations integrated #145369

This two small patches have been integrated to make testing much easier.

Subscribing

1. Changed naming of some variables in _color_page_alter after reviewing patch for #163785.
2. Added a break in _color_page_alter for more speed like #163785.
3. Added some more documentation.

Could we stick to one issue per queue? Hass, your patch is mighty complicated and I'm not sure about the looping over the style sheets inside "_color_page_alter". Why is that even needed?

To simply saying that this patch works or not I don't feel comfortable with. It's way too complex for me to follow.

Could we stick to one issue per queue?

Well, i know it's not the easiest patch, but it couldn't be much smaller. The primary problem is - if we fix the many outstanding @import issues - we must change color module, too. There is no way around :-(. Additional there seems no way to fix color module without solving some color bugs first to make this module modular and reusable. Only as side effect we get a color module that help themer's very much and allows to add RTL support in an easy way. The only thing i could remove from this patch is the RTL stuff, but i feal like this makes no big sense. If i do - all people needs to re-test the complete stuff again... i think we don't like to do testing the same stuff twice, isn't it!?

Hass, your patch is mighty complicated and I'm not sure about the looping over the style sheets inside "_color_page_alter". Why is that even needed?

Yep, but that's not so difficult as it looks like.

What happens here is (example filename):
1. Theme adds a stylesheet named /sites/all/themes/mytheme/css/navigation/nav_shinybuttons.css.
2. This one gets rewritten by color module and saved in /files/color/theme-12456/nav_shinybuttons.css.
3. _color_page_alter is called within THEME_preprocess_page()
4. The loop looks in var['css'] and find /sites/all/themes/mytheme/css/navigation/nav_shinybuttons.css and unset this file, while we have a recolored we can simply add instead!
5. After unset is done we insert /files/color/theme-12456/nav_shinybuttons.css.

That's really all... but we don't add - blind - all re-colored files a theme might have. Don't forget we might have 10 re-colored stylesheets in the "files/color/theme-12456" directory, but the list of stylesheets the page have added is only 6. And it may kill the layout if the other 4 stylesheets get added or one these other files are only different navigations that should be added dynamically in THEME_preprocess_page().

To simply saying that this patch works or not I don't feel comfortable with. It's way too complex for me to follow.

It took me some hours/days to understand color module and how buggy and non-reusable it is and why is is so *G*. I really hoped this patch could be less complex, but i think it cannot if we want to fix this D5 "first day" bugs...

I will try to help you asap to understand what we have done here... I could add some more documentation lines in the loop with small examples if this helps!?

hass, let's see a few questions.

1. Why are you removing ../ with a preg? realname won't work?
2. __color_rewrite_load_stylesheet we never use __ and in overall, why you can't reuse the other functions which seems to perform the same duty? If it'd would reuse, the patch would become much simpler.

1. Why are you removing ../ with a preg? realname won't work?

Are you talking about _drupal_build_css_cache $path = preg_replace('`(^|/)(?!../)([^/]+)/../`', '$1', $path);? This has been introduced by Steven in #7 and clean's directory names. This think we should do this while we are rewriting all relative paths to absolute paths. I haven't changed this...

2. __color_rewrite_load_stylesheet we never use __ and in overall, why you can't reuse the other functions which seems to perform the same duty? If it'd would reuse, the patch would become much simpler.

I fighted with me about changing the function name or simply use this double underscore... do you have a better name/idea? We can change this... I hoped to reuse this function... but the following regex must be used only in the CSS compressor and not in color module.

  $contents = preg_replace('<
    \s*([@{}:;,]|\)\s|\s\()\s* |  # Remove whitespace around separators, but keep space around parentheses.
    /\*([^*\\\\]|\*(?!/))+\*/ |   # Remove comments that are not CSS hacks.
    [\n\r]                        # Remove line breaks.
    >x', '\1', $contents);

is required for drupal_load_stylesheet() to remove the comments, but must not be used in __color_rewrite_load_stylesheet(). If i move this regex back to drupal_build_css_cache i get other problems. As example - if there is a commented backslash hack - like Drupal core currently have - this results in incomplete deleting of comments in following css files and this must be avoided.

can't you just add a parameter to the function and execute or not the preg depending on that?

So i reduced the patch and added a second parameter to drupal_load_stylesheet() that allows switching the optimization on or off.

FYI: If you'd like to remove the "notice" displayed after save settings is pressed, use patch in http://drupal.org/node/183374.

PLEASE get this in...

Hass, this patch still seems to be largely under debate, so it is not yet close to "getting in".

If none is testing we cannot get this in... but it will fix TONS of ~1 year old open bugs and should - ahm - no *must* go in... :-)

hass is right, this patch fixes a killer feature that I've been waiting for since I knew Drupal (color.module in RTL languages)
I tested the patch and it works really well....
It must be included in the 6.0 ...that's an extreme killer feature for me.

verb3k, if you tested, and it works, please explain how you tested it, then mark the issue "Ready to be committed".

Although Garland/Minnelli does not yet use the CSS cascading addition supported by Drupal (because it is also used as a maintenance theme and it would be broken there), Drupal 6 supports adding themes in cascading order, and using @import is not required anymore. This invalidated the original argument for this fix, so we let's get a list of issues this patch tries to solve and see whether it is really critical. I don't see this right now.

@Garbor: Sorry, but how can you be sure no one needs @import support? There are numerous themes that use them and are broken today in aggregated mode. This is why this patch have been original written. Are you arguing with the configuration of .info files as @import replacement? In my opinion we shouldn't limit people here. @import is a valid CSS feature and it sounds very strange to me if you say we don't support @import commands in D6 at all!?

The patch is also written to fix tons of bugs in color module that make it completely unusable in many ways and finally add the RTL support for color module. While RTL support is critical this patch should kept critical. See #30 about the issues.

hass: the more bugs you try to fix, the harder is it to review. This issue seemed to be about fixing @import, which was piggy-backed by fixing color module. Now these might be separated with different changes in files, but it is hard to tell whether fixing @imports in general is good without touching color module. Having too much to review here essentially leaves you without reviewers and not getting this fixed at the end. I don't know where to start.

You tried to explain in http://drupal.org/node/113607#comment-500016 that this is not possible to break into two issues and you needed to piggy back this fine issue. Please explain to me, why would color module break if we commit the changes to common.inc only. Color module does not seem to use drupal_build_css_cache(), and only uses _drupal_build_css_cache() *after* this patch. Why do we need to have three stuff (@import fixing, color module fixing and color module RTL support) in one issue?

Why do we need to have three stuff (@import fixing, color module fixing and color module RTL support) in one issue?

Really i understand this big patch problematic, but we should get this bugs fixed however hard it is and we work on this to make many many many people happy and MORE themes working with core aggregate and compress - not to mention this *argl* suxxx buggy and unusable color module...

Reviewed my patch about this color module split stuff and the thing is - we might possibly split this in general - long time ago I've worked on it - but it will result in a unfixed color module! Color module should use the same functions and logics as core does. If i put this in mind we must change both together for not getting code using different functions for the same stuff (core aggregate with @import works and color module with @import is broken as in past).

So color module stuff is around ~12% of this patch and RTL patch is ~3% only... getting the RTL stuff working with color module and without cleaning up and reusing the @import logic is horrible! We could do this split, but then we must test both patches 2 times and i think it is difficult enough what this does together and we don't like to test two times the same (the much testing this is major point of keeping this code in one patch). We could split - but we must test everything twice. I don't want try to argue to much, but i only complain about the much much testing work and we need to do twice then and might end up in a crippled color module... ok it's crippled today... but it could be fixed now for RTL support. Shouldn't we get RTL support in? :-(

Hass, the more time we take arguing on principles, the less likely we have time left to work on fixing this. You fail to provide examples on why color module's issues get better or worse with the original @import issue solved or not, so these are proved to be unrelated.

Setting back title from before you hijacked this thread. Awaiting patch for that issue, and your patches submitted in another issue.

I don't like your decision much, while it will produce senseless extra work. However here is only the common.inc part.

Please keep in mind - to get RTL support and the crippled color module fixed we need this patch first committed - only for the people thinking we may not need this patch to get in! I will post the follow up issue.

Follow up patch to get color module fixed http://drupal.org/node/193604.

Follow up patch to get color module fixed http://drupal.org/node/193604.

OK, there was quite some interest before hass take over here. Guys, can we get this simple patch reviewed?

Code review:

- document that in both cases, we need to static the previous parameter because the recursive calls from preg_replace() will not pass that over
- document the "helper functions" better, ie. their current one line comment is rather a secondary line explanatory comment, not a first class explanation on what they do

Otherwise this needs pretty heavy testing with all kinds of @imports. Hass suggests, that themers should not be regulated, so let's test how this fixed functionality matches up to their creativity.

Please take a look. There are no other changes.

A couple of questions:

What happens with external URLs? I can't tell for sure without hacking some code, but it looks like the URLs are passed around up to the file_exists, and then dropped. If they are dropped then that seems like it would definitely break any code that uses this (such as Yahoo CSS resets etc). If they are not dropped, then I don't see any logic to move them to the top of the file - and leaving them in the middle breaks the CSS spec (as noted in the original code).

Either way, unless we are going to start doing http requests to fetch these we are still only going to be able to fully support the @import spec. I guess we can live with that, but I wonder how much this really adds. Would it not be better to refactor color module to do a drupal_add_css() as it should be doing in the first place?

It might fix the color module but @import is common for sub themes as well as other uses. I think we should address the issue as we have been not work around it.

@Grugnog2: Yes, external links where broken. I have fixed this for now. I tested with following external @imports successful:

@import url(http://localhost/drupal5/sites/all/themes/mytheme/css/screen/test1.css);
@import "http://localhost/drupal5/sites/all/themes/mytheme/css/screen/test1.css" screen;
@import 'http://localhost/drupal5/sites/all/themes/mytheme/css/screen/test1.css';
@import url("http://localhost/drupal5/sites/all/themes/mytheme/css/screen/test1.css") print, embossed;
@import url('http://localhost/drupal5/sites/all/themes/mytheme/css/screen/test1.css');

one line isn't working here and currently simply get's removed. If i'm in /drupal6/* it seems not possible to to the "chdir" to this file so it would be impossible to aggregate and compress together...

@import url(/drupal5/sites/all/themes/mytheme/css/screen/test1.css);

I'd like to encourage someone with more regex experience to take over this case. I came to my limits with commons.inc line 1792. This is the source of this problem and may need fixed to get this working if required. In a perfect world we cannot live with this - in a 99% we could.

After the patch, all it does is copy a handful of style sheets uncompressed into the files folder and breaks a few styles.

I'm not sure why we are trying to handle all cases here. Steven's original patch would have been nice to move forward from.

And this isn't critical.. At best it's a quirk IMO that should be fixed. And the specific issue you spotted with Minnelli initially can be fixed with sub-theme cascades. The recent maintenance theming patch allows this.

Made a patch that by passes imports entirely for Minnelli.

http://drupal.org/node/197608

I want to get the import issue fixed. I've been afraid due to regex but I'll try to take a look.

After the patch, all it does is copy a handful of style sheets uncompressed into the files folder and breaks a few styles.

It is normal that styles are copied to files/color/[theme] folder. this is how this is designed and how it was designed in past. After you enable compression, all this files get compressed together. Nothing wrong with this.

What styles are broken??? I checked so many alternatives about paths and keeping cascading order to get nothing broken... i cannot follow you with this short and unspecific statement.

I'm not sure why we are trying to handle all cases here. Steven's original patch would have been nice to move forward from.

Stevens patch does not cover all required situations for sure and i'd like to get my themes fixed. It's missing some important things (e.g. recursive support - chx added this) and contains bugs. The recursive part is one of the most important part here and Stevens patch is far far away from a solution.

And this isn't critical..

The color module patch hardly depends on this patch and we need to get this in. So marking back to make clear we need this for final.

Can hardly be a breaker for beta 3, as that's been out for a little while.

Guys, there is no need to mark anything a breaker. If there is code ready, it will be in, otherwise the beta will be out without it. We are not delaying releases on issues without much of an interest.

No feedback from dvessel yet, so his problem seems something different.

Patch has windows line breaks but applied with a little offset.

There is no whitespace or comment removal with this patch applied, at all. Only aggregation. So CNW.

Uhhh - i see... damn - have been introduced while developing and testing external URLs.

I will provide a new patch later the day, but you can workaround now by changing line:
if ($_optimize && false) {

into:
if ($_optimize) {

Sorry... :-(((

Hass, I know it is supposed to move the aggregated css files into "files/css/xxx.css". The problem is, those file are not compressed. They are aggregated but the original spacing is there.

[edit: nevermind.. catch explained it already.]

Yes removing && false; works and allowed me to review this.

I made a test.css file with some basic rules. Put

@import url(test.css);
@import 'test.css';

at the top of minnelli.css

test.css was loaded in the correct spot - just before the rest of Minnelli's declarations at the end of the aggregated file. Good.

However:
@import url(/themes/garland/minnelli/test.css);

gets stripped.

@import url(http://drupal6.example.com/themes/garland/minnelli/test.css);
@import 'http://drupal6.libcom.org/themes/garland/minnelli/test.css';

Both get printed verbatim at the front of the file.

I'm guessing color.module fixes don't need absolute/external urls though right? So do we care?

Quick editor fix of the line endings and &&false

An external url() use case brought up earlier is usage of YUI files for example, where import-ing is the suggested and used norm (in the name of letting Yahoo host your CSS files). Given that we come from a completely @import breaking position, it might be more useful to us to fix the local links and leave the external imports moved to the beginning of the file (as before), thus fixing local imports but not fixing external imports yet - if this makes too much of a task for now. Doing external requests to grab the imported CSS files and going down recursive on this might take a lot of time, so I think we can go without fixing this for now to clean up our internal problems first.

@import url(/themes/garland/minnelli/test.css);

gets stripped.

I don't think this is critical. I know some will be upset about not supporting this but using absolute paths is bad for moving installations around and I think it should be avoided. I've seen people trying to share styles across themes and using paths like so. It would be confusing to leave it as it is but it should not hold this patch back, imo.

Would be better to use relative paths like so.. @import url('../../path/to/style.css')

@import url(http://drupal6.example.com/themes/garland/minnelli/test.css);
@import 'http://drupal6.libcom.org/themes/garland/minnelli/test.css';

Both get printed verbatim at the front of the file.

I'm guessing color.module fixes don't need absolute/external urls though right? So do we care?

I know I don't. drupal_add_css() doesn't support absolute paths either. Not that it makes the case any better but it's not a common thing to do. Maybe someone can explain why that's useful. It may also be more complexity than it's worth (if it's possible at all) to make a request for the remote file and aggregate the contents into the rest of the file in the proper order.

I tested a few imports and it works very well. I'm currently testing it more extensively.

Tested all possible itterations or relative paths. It also works with imported files importing even more imported files. Paths always resolve properly, except absolute paths of course. Pretty solid!

If we could get someone with more regex experience to fix the absolute paths, that'd be great. If not, maybe a follow-up patch? As the patch stands now, it's 90% there. Nice Hass.

Is there a reason for the $optimize parameter for "drupal_load_stylesheet()"? By the time the function runs, we know that we always want it optimized. I removed it.

Updated the docs and changed "_drupal_build_css_cache()" to "_drupal_build_css_path()". All it did was build proper paths.

dvessel: The rationale behind the $optimize parameter was that color module would reuse this code, and it needs the @imports to be resolved, but not the CSS to be optimized as far as the analysis ended up there. (I did not validate this argument myself).

@import url(/themes/garland/minnelli/test.css);

We don't like to touch absolute paths... if we do they might get broken. Aside the directory change is nearly impossible... maybe someone reference a file outside drupal :-)... for e.g. inside gallery or phpbb or whatever... we cannot be sure what we do if we try to correct this type of path. I tried it... and learned there looks no way.

Is there a reason for the $optimize parameter for "drupal_load_stylesheet()"? By the time the function runs, we know that we always want it optimized. I removed it.

very big fault... sorry. Do NOT remove this param, please - we *absolutely* need this for the RTL support in color.module if recolored files are uncompressed... i know why i said to test both patches together :-(((

@catch: thank you for the quick update. Patch looks fine for me... and i hope i found and finally fixed now this little bit "hidden" and default eclipse setting that converts line endings to the platform endings...

OK in that case I'll mark this as needs work - for adding docs/renaming function in #72 or putting $optimize back into #75 - however you want to look at it.

I 100% agree that external urls are a follow-up patch - if this fixes color.module and some themes, then it's done what it's supposed to.

@Catch: should i document in this patch that we need the $optimize parameter for color.module? No problem...

Aside - this patch support external urls...

hass: testing (reports above) did not seem to show that it actually supports external URLs.

Yes, document that it is used by color module too. IMHO that would help to not remove this "useless parameter" later.

here it is. My bad

@dvessel: sorry your patch is contain at minimum one bug ($contents = drupal_load_stylesheet($file, TRUE);)... please use #72 for testing... i'm very bussy tonight and cannot provide a patch within the next hours. catch's patch was fine... i don't know what you are changing in your patches, but it looks like you break the code... as catch said, we should add a comment that the optimize param will be used by color.module with RTL... that's all that need to be added to catch's patch.

Therefor all others... use patch in #72 only for review, please.

Christ.. It's now functionally identical to Hass's patch.

Quick re-test, works fine, should be good to go now I think.

I was trying to review this patch late last night, but I'm a bit confusled by the entire thread in this issue. Sorry to be a pain, but I don't understand if the patch that Hass called for earlier in #29 (node/175121) needs to be applied *before* this latest one from dvessel in #83. Should I, or should I not?

Senpai - this is a prerequisite for #175121, it doesn't depend on it. Which I think means back to RTBC but feel free to change back if you disagree.

@Senpai: We splitted the issues to make the patch smaller... the latest patch for this topic is in #83.

I'm fine with the function name changes and the updated docs. Thank you all for you help to get this RTBC.

Ok, I'll test this out later today if I can peel 15 minutes out of my day.

Senpai: I am looking forward to your testing results.

Ok! No way man. I applied the #83 patch to a fresh update of HEAD, and the first test I did was to copy garland into sites/all/themes and rename it 'grlandMod'. Upon visiting admin/build/themes, I was greeted with a WSOD, and the 19 watchdog entries all dealt with something similar to:

a:4:{s:6:"%error";s:7:"warning";s:8:"%message";s:182:"include(./sites/all/themes/garlandMod/block.tpl.php).....

I'm afraid I don't fully understand what's going on within this patch, but I can tell you that it doesn't work according to the way I understand Drupal. I'll be available for further testing if you guys can figure out why this one failed. Ping me in #drupal.

@Senpai: I think you missed something or this is something different... have you renamed the garland.info file to grlandMod.info and edited the internal theme name? I tried to repro this problem some minutes ago. without any problems at all.

Try this way:
1. copy themes/garland to sites/all/themes/garlandmod
2. rename sites/all/themes/garlandmod/garland.info into sites/all/themes/garlandmod/garlandmod.info
3. rename the theme name in sites/all/themes/garlandmod/garlandmod.info to name = Garland Mod
4. activate Garland Mod as standard theme.
5. success.

Well, I sat down and tested this patch:

- added an external @import to the garland style.css
- added a local (same directory) test.css @import as well

Observed the external one moved to the beginning, as expected, and the local test.css replaced exactly where the @import was.

- tested with both url() wrapped and simple string paths, all worked

Then went on to test recursive imports:

- added a relative import to a CSS file in the drupal root
- added an import to that CSS file (in the drupal root) to another CSS file in the same dir

All worked, all imports were resolved exactly where I placed them. So all in all, this seems to work as advertised.

Although I have some comments on the phpdoc in this patch, as it is RTL related, I'll continue on the RTL issue. More help is welcome there: http://drupal.org/node/193604

Thanks for everyone who participated.

@hass in #91: Yup, I definitely _did_ miss something. In my haste to check this recursive importing thing, I forgot to actually edit the innards of my garlandmod.info file! Doh! All is well now, I guess, so I won't do it again just for kicks.

guys I am not really into these technical posts, but I want to know one thing.
Will this patch fix the color module in RTL langauges ?

verb3k - that just went in in another patch:
http://drupal.org/node/193604

This should be at least partially backported to 5.x. I am marking http://drupal.org/node/172830 as a duplicate of this, which is specifically about absolute paths being improperly rewritten.

I used patch from http://drupal.org/node/113607#comment-499987 with 5.0

now upgraded and had to replace the 6.0 code with http://drupal.org/node/113607#comment-499987 again to fix my theme.

I think it is because of the order of the includes...

Patch in #83 is what we now have in D6 as I remember... not #7!

Plain drupal 6.8 did not work for me, I manually applied #7 and it works just fine (as it did in 5.x)

Can you provide a detailed analysis what does not work and what is broken? D6 keeps the CSS files order 100% intact and I really need this myself - It doesn't matter how deep the files are nested. This was not the case in D5... CSS compressor works not that good in D5 as does in D6. I'm not aware about any bugs (except very rare IE5 hacks may get destroyed) in the D6 CSS compressor code. See #139117: Remove outdated IE5-mac hacks for build_css_cache regex

I`m applied patch in #83 to my Drupal 5.10 (with SA) site.

@import seems to me resolved correctly, but url('relative/path') in parent theme (if using subtheme) not resolved correctly.

Example:
sometheme/style.css
sometheme/images/logo.jpg
sometheme/subtheme/style.css

in sometheme/subtheme/style.css:
@import "../style.css";

in sometheme/style.css:
background:#C7DCE1 url(images/logo.jpg);

After applying the patch and enabling aggregation, url now looks like this:
/sites/all/themes/sometheme/subtheme/images/logo.jpg
Which is wrong. ('subtheme' part)

Marking as won't fix as d5 is end of life.