Only process CSS if the browser is IE

Switch on aggregation, and you won't see any @import statements.

If you switch it off, it means that you don't care that much about performance. Or is there any good reason for switching it off?

I think it is an interesting patch, considering that it can just speed up the development (I tend to leave the module enabled even if I use Firefox). Reviewing...

Ok, development performance is an argument.
Should we make this optional?
I'm not sure how reliable this browser sniffing is. Somewhere I read people having doubts, but I have little idea myself.
Did you test the speed improvement?

I would not make it optional since we only need the module when using IE. About reliability of the sniffing method, that's indeed the only issue I see and the reason why I want to review it before to commit to HEAD. Any input on this can be of great help.

We can base our discussion on the following links:

http://www.sitepoint.com/blogs/2009/05/31/why-browser-sniffing-stinks/
http://www.useragentstring.com/pages/Internet%20Explorer/
http://msdn.microsoft.com/en-us/library/ms537503%28VS.85%29.aspx

Anyway, after having a quick look to them, I would say that looking for "MSIE" well suits our need.

We could have false positives:
- IE 9 might support > 31 stylesheets.
- Opera can masquerade.
- Some others browsers might send a string containing "MSIE" somewhere.

This is all not a severe problem, because the worst that could happen is that we send @import instead of <style>.

The other problem would be if a browser refuses to send any string. I have no idea if this ever happens.

http://php.net/manual/en/function.get-browser.php

So:

if (number of stylesheets > 20 AND (IE OR sniffing fails)) {
  use @import;
}

The other, bigger problem is caching.
Caching (by boost or other means) would need to happen per user agent. Otherwise, once the page is cached for one browser, the same html will be sent to every other browser.

Such caches are often disabled in development time, but this is not a reliable fact. I often disable CSS aggregation to check which core CSS files are responsible for layout problem x, while other caches are still on.

There can also be other reasons why a file produced for Firefox is viewed in IE. For instance, if you use the browser's "Save as..", and then open that file in IE.

Another point mentioned in
#228818-342: IE: Stylesheets ignored after 31 link/style tags
A CDN !

I think the best way to do this is

if (number of stylesheets >= 20) {
  use @import;
}

Works with all caches and doesn't affect the output if css aggregation is turned on or low css file count. Make 20 configurable via variable_get().

Looks like there is already a file limit, this makes the limit configurable. I would make this work only if http://drupal.org/project/browscap is enabled & user is logged in. Patch is a work in progress for this goal...

decided to not use browscap after finding this script, http://chrisschuld.com/projects/browser-php-detecting-a-users-browser-fr... took the IE detection part from it.

This will selectively use import for logged in users; if they are IE it will use @import. If user is not logged in, then it will always use @import if the number of css files is greater then variable_get('unlimited_css_count_threshold', UNLIMITED_CSS_COUNT_THRESHOLD) with the default set to 22.

variable_get('unlimited_css_logged_in_ie_detect', UNLIMITED_CSS_LOGGED_IN_IE_DETECT) default is set to TRUE. This is an option so this new functionality can be turned off if using something like http://drupal.org/project/authcache

Ignore

Patch with this applied #755586: Fallback for CSS/JS aggregation for non-writable directories

As mentioned in #7, caching will break this.

If a page gets rendered for IE, then all future requests for that page (regardless of UA) will be rendered that way until the cached element expires. Worse, if the page is rendered by a non-IE UA then all future requests by any UA (including IE) will render in an incompatible fashion.

fixed some minor cosmetic issues with the patch

@mikeytown, did you read #7 and #14 ? Please, you can't just upload one patch after the other and ignore what others have to say.

As I understand, your patch does three things:
a) browser sniffing.
b) make the 20 stylesheets limit configurable.
c) Split things up in smaller functions.

I think (a) is a bad and unnecessary idea, and (b) should rather be discussed in a separate issue.

About (c) - I am usually in favour of splitting things up in small functions, but in this case I would not mind the module to remain the way it is.
If we do any of (a) or (b), or have any other serious plans, then ok, maybe it does make sense to start with this little refactoring.

What I don't understand is why you use a by-reference argument instead of a return value.

a) patch does browser sniffing only if your logged in. It can be turned off via setting (for things like authcache). If your logged out then it always uses import.
b) who cares... thought it would be nice.
c) prevents code duplication when using more complicated logic

we could use return. I'll have to double check but I don't see a problem with using return.

ok, thanks for explaining.
I still don't see how it is relevant if someone is logged in..

About (b), my point is, if we are undecided about (a), then it would be a bit strange to have a b+c patch rtbc'd in this issue. I always like to have things nicely documented and easy to find in the issue queue.

And (c) makes a lot of sense in combination with the rest of your patch.

Btw, in another issue with JohnAlbin we had a little controversy about mixed import+link vs just import (this is all about the case where we have 22+ stylesheets). I still don't know which one is better.

But let's finish our debate about (a) first.

I created two new issues,
#855038: Make the "magic number 22" configurable..
#855040: A bit of refactoring to make future work easier..

I do have some ideas about the configurable magic number, and I think there is still some open debate for the IE conditional stuff. So it would be a good idea to get the refactoring rtbc'd, and then continue from there. Do you think you can do a patch for #855040 that does only the refactoring and nothing else?

NOTE: My english is awful.

I'll try this option "$is_ie = strpos($_SERVER['HTTP_USER_AGENT'],'MSIE') !== FALSE;" but with other method.

I add this library (or class as you wish call it).

http://chrisschuld.com/projects/browser-php-detecting-a-users-browser-fr...

And then in the unlimit_css.module, require_once and in the function unlimited_css_preprocess_page(). add this line.

<?php
  $is_ie = FALSE;
  $browser = new Browser();
  if ($browser->getBrowser() == Browser::BROWSER_IE) $is_ie = TRUE;

  if (!$is_ie) return;

  $files = _unlimited_css_build_files_array($vars['css']);
  ........
?>

i'm using the 6.x-1.x-dev module and work pretty good.

...subscribing.