OS: FreeBSD 7.1-RELEASE-p4
Webserver: lighttpd-1.4.22
PHP 5.2.9 with Suhosin-Patch 0.9.7
PCRE version: pcre-7.9 *!

When using a zen-based theme, the webserver returns an "HTTP Error 500 - Internal server error", because the php cgi process ( both fastcgi and normal cgi versions tested ) segfaults.

I traced the segfault in the php-cgi.core dump file with gdb at the match routine in the pcre library. I also tried increasing and decreasing the pcre.backtrack_limit and pcre.recursion_limit, but it didn't help. ( also: memory_limit = 128M )

I traced the drupal php code which triggers this segfault to this preg_replace call in the drupal_load_stylesheet function:

Lines 1066-1073:
    if ($_optimize) {
      // Perform some safe CSS optimizations.
      $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);
    }

I assume the problem lies at the regex and this version of pcre, because the exact same configuration of (drupal,theme,database) doesn't cause the crash on another system ( debian-lenny libpcre3 (7.6-2.1) ) .

The crash only happens on a certain pattern in the css ( here in $contents ), and that's what i'm trying to find out now. I'll post it when I pinpoint it.

I found the same issue here, but it's slightly different, since only one comment has the exact same issue: http://drupal.org/node/331915

Excuse my bad english.

Greetz,
Raul

CommentFileSizeAuthor
#185 common.inc-185-D6.patch2.71 KBjonne_jvl
#182 common.inc-181-d5.patch2.72 KBTri
#181 common.inc-181-d6.patch2.8 KBTri
#178 444228-kill-the-mac-ie-regexp.patch129.01 KBDamien Tournoud
#171 common.inc-170.patch130.55 KBcasey
#162 common.inc-161.patch131.09 KBTri
#162 common.inc-161-d6.patch2.8 KBTri
#162 common.inc-161-d5.patch2.72 KBTri
#159 common.inc-159.patch131.46 KBTri
#159 common.inc-nh-159.patch129.68 KBTri
#159 common.inc-159-d6.patch2.88 KBTri
#159 common.inc-159-d5.patch2.8 KBTri
#120 process_comment_improved.txt950 bytesridgerunner
#117 common.inc-117.patch128.86 KBTri
#117 common.inc-117-d6.patch2.66 KBTri
#117 common.inc-117-d5.patch2.68 KBTri
#112 common.inc-112.patch128.87 KBTri
#112 common.inc-112-d6.patch2.68 KBTri
#112 common.inc-112-d5.patch2.7 KBTri
#107 rr_common.inc-107.patch5.01 KBridgerunner
#105 rr_common.inc-105.patch5.1 KBridgerunner
#105 minify_css.php_.txt4.43 KBridgerunner
#101 common.inc-101.patch128.74 KBTri
#98 common.inc_.patch1.75 KBdicreat
#90 common.inc-90.patch124.36 KBTri
#80 common.inc-80-d6.patch1.26 KBTri
#61 common.inc-61-D6.patch1.26 KBTri
#61 common.inc-61.patch1.22 KBTri
#61 css_test.zip1.35 KBTri
#59 stylesheet-optimize-segfault-444228-59-D7.patch565 bytesJohnAlbin
#56 common.inc-56-d6.patch1.26 KBTri
#47 common.inc-47.patch1.22 KBTri
#47 css_test.zip1.35 KBTri
#45 drupal-444228.45.patch1.32 KBTri
#45 drupal-444228.45-d6.patch1.32 KBTri
#45 css_test.txt1.46 KBTri
#44 drupal-444228.44.patch1.32 KBTri
#44 drupal-444228.44-d6.patch1.32 KBTri
#44 css_test.txt1.46 KBTri
#40 test.php_.txt3.11 KBdonquixote
#37 drupal-444228.33.patch1.03 KBmikeytown2
#37 drupal-444228.33-D6.patch1.06 KBmikeytown2
#37 drupal-444228.33-D5.patch1.04 KBmikeytown2
#34 drupal-444228-32.patch1 KBTri
#31 drupal-444228-31.patch1.1 KBTri
#22 drupal-444228-22.patch1.02 KBTri
#19 common.inc_.patch828 bytesTri
#13 test.php_.txt1.9 KBdonquixote
#8 strip_comments_without_pcre.patch1.4 KBraulgigea
#4 CrashHang_Report__PID_5020__PID_5908__04242009205858726.zip37.36 KBlaurentchardin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

asb’s picture

In the forum is a general overview about this issue, including some more information about the server environment:

D6 on FreeBSD 7.0 & Lighttpd - 500 Internal Server Error

(it's the same D6 site, and we could really need some help with this!)

Greetings, -asb

raulgigea’s picture

Hi,

first, i want to make a little correction:

The Php Code which triggers the segfault is located at lines 1966-1973, in the file includes/common.inc .

second, i pinpointed the php code which always reproduces the bug. It's:

$contents = "
/* aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
*/
";  
   $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);

Just put that code in a separate php file, run it and it should crash. ( Eventually increase the number of aaa's, feel free to insert malicious machine code, etc. ;) )

This DoS only seems to affect php version 5.2.9 built against pcre 7.9, since i tested it on following machines:

  • FreeBSD 7.1-RELEASE-p4, PHP 5.2.9 with Suhosin-Patch 0.9.7 , pcre-7.9. Result: segmentation fault
  • FreeBSD 7.1-RELEASE-p4, PHP 5.2.8 with Suhosin-Patch 0.9.6.3, pcre-7.8. Result works
  • FreeBSD 7.1-RELEASE-p4, PHP 5.2.8 with Suhosin-Patch 0.9.6.3, pcre-7.8. Result works
  • FreeBSD 8.0-CURRENT, PHP 5.2.6 with Suhosin-Patch 0.9.6.2, pcre-7.8. Result works
  • MacOS 10.5.6, PHP 5.2.6, pcre 7.6. Result works
  • So, a Drupal Theme must only include a css file with that comment lines somewhere in it, and than drupal/php/webserver crashes ( given it has pcre 7.9 & php 5.2.9 ). It Seems the problem is when there are too many characters inside a multiline comment.

    raulgigea’s picture

    Seems the standalone pcre version i gave was irrelevant, since another version is bundeled with every php release.

    Also, i managed to reduce the php code to the core of the problem:

       $contents = 'd' . str_repeat('a', 1900) . 'b';
       $contents = preg_replace('/d(a)+b/', '\1', $contents);
    

    And, I observerd, the segfault doesn't occur if I disable mhash.so extension of php from extensions.ini.

    laurentchardin’s picture

    I have the same exact symptom on another totally different system: vista, apache 2.0.x and latest version of php (5.2.9-2)
    Debugging the dieing apache process shows a bunch of 'match' commands before the stack overflow occurs.
    (actually i have 64Mo memory limit)

    Here is a trace.

    Disabling css&js preprocess fixes it.

    Damien Tournoud’s picture

    @raulgigea: nice work on investigating the issue, but this is doesn't seems to be a Drupal bug. Why not opening a bug on the PHP bug tracker?

    raulgigea’s picture

    Hi, well because I wasn't really sure if the development (trunk/head) version of php still has this bug, in which case i should report the bug to the FreeBSD package maintainers, and still not here.

    On the other hand, this post might help some users which upgrade to php 5.2.9, which i presume will happend soon, because 5.2.9 ( as a new version) is reaching the unix distros anytime now. Unless i can stop it with a bug report ;).

    But i'm compiling the development version of php as i type, and will either open a php bug or post here the result, if it works.

    raulgigea’s picture

    Okay, it actually does crash with the cvs snapshot version of php. So here's the new php bug: http://bugs.php.net/bug.php?id=48153 . <- everybody vote for that, and maybe they fix it in the next version :)

    raulgigea’s picture

    Hi,
    as you might have read, the php guys didn't accept it as a bug aswell.

    For a big enough pattern, the preg_match function simply takes too much stack memory. It's not a bug, its the same as coding a recursive function that goes too deep into recursion.

    It is the programmers responsability to check that his code doesn't eat up all the stack and choke.

    In this case, Lines 1966-1973, in the file includes/common.inc works in most configurations of php, but some systems don't have big enough stack size, or other php modules eat up more stackmemory so that the left one goes out faster than usual.

    In any case, if we construct unusual test cases - for example:
    - insert >60000 bytes into a multiline Comment in an css file, than it almost surely segfaults on any configuration ( os, libs & php ).

    The problem is where the os & php & libs are poorly configured, this case occurs with much less bytes and it can happend that a usual longer multiline comment is able to trigger this stack overflow.

    For those cases i provided a patch, that is equivalent to that regex, but withouth using PCRE functions thus without the stack overflow.

    Greetz,
    Raul

    scoorch’s picture

    Raul, great work. I had the 500 Error here too when enabling css compression. Your patch fixed the problem. Thanks so much!
    Thomas

    asb’s picture

    Is there any chance to get this patch into core, or will we have to maintain this bloody patch for every Drupal release?

    Thanks -asb

    poshat’s picture

    Hi!
    I have same problem. I tried turn on optimization CSS and JS and site working ok.

    asb’s picture

    Version: 6.10 » 6.14
    Priority: Normal » Critical

    This bug is still killing Drupal everytime core is updated (and thus our patch is being overwritten).

    Rauls patch from #8 again has to be modified, this time we're getting:

    Parse error: syntax error, unexpected '*' in /usr/local/www/drupal/includes/common.inc on line 1927

    This is extremely annoying and an absolute showstopper to run Drupal on FreeBSD. Updating the applicable version of Drupal core to 6.14 and setting the priority to "critical" since it forces us to not install security updates.

    Greetings, -asb

    donquixote’s picture

    FileSize
    1.9 KB

    I have the same, or a similar error on my windows xp localhost.

    I was able to reduce the regex and the "offensive" CSS, and make a standalone php script to reproduce the error.
    I get the error on localhost, but not on my webspace. So I guess it depends a lot on the configuration.

    In the "offenive css", it is enough to remove a few letters (no matter which exactly) to make it work.

    mattyoung’s picture

    donquixote’s picture

    Could be, who knows..
    I will link it from there, hope it helps!

    Damien Tournoud’s picture

    Status: Active » Closed (won't fix)

    The default settings.php file now ships with (commented-out) configuration parameters that will allow you to increase the PCRE limits on your system. Just comment out the parameters and set the limits that make sense for your particular case. Closing this bug.

    Tri’s picture

    Status: Closed (won't fix) » Active

    Hi.
    The solution from #16 is to add this to your settings.php

    ini_set('pcre.backtrack_limit', 200000);
    ini_set('pcre.recursion_limit', 200000);
    

    I have tried with values up to

    ini_set('pcre.backtrack_limit', 40000000);
    ini_set('pcre.recursion_limit', 40000000); 
    

    without any luck.

    I am on
    FreeBSD 7.2-RELEASE-p2
    Lighttpd 1.4.23
    PHP 5.2.11
    Suhosin-Patch 0.9.7

    I am switching status back to active, since the solution from #16 doesn't seem to do it.

    asb’s picture

    @Damien (#16):

    Please have a look at the inital bug report from Raul; there he writes on April 24, 2009: "I also tried increasing and decreasing the pcre.backtrack_limit and pcre.recursion_limit, but it didn't help".

    Fiddling with pcre.backtrack_limit and pcre.recursion_limit is where we failed before filing this bug report half a year ago. The whole point of the patch from #8 is that modifying pcre.backtrack_limit and/or pcre.recursion_limit does not solve this bug, at least not on FreeBSD.

    It would be very kind if the powers-that-be could have a look at the instructions from #2 and would try to reproduce this bug. It shouldn't be too hard. Alternatively we would be very glad to receive instructions where else we could file this bug report; since this weakness probably could be used to execute malicious code, it might be useful to inform the security team?

    Greetings, -asb

    Tri’s picture

    Status: Active » Needs review
    FileSize
    828 bytes

    Hi.

    I have found out that the regexp that was striping out the comments wasn't so well crafted, and that's why it was segfaulting PHP through endless recursion.

    The attached patch fixes the problem: I have tested it with comment chunks of 50K+ and it works fine and quick.

    I hope it will help the other FreeBSD+Lighttp fellows in this thread ;)

    Please test it and report back.

    donquixote’s picture

    Wonderful!
    The modified regex solves the problem for me on Windows XP (for the test script I created). I did not do other tests yet..

    stmh’s picture

    Nice patch -- worked for me too. Thanks!

    Tri’s picture

    FileSize
    1.02 KB

    Here is the same patch as #19 against Drupal head.

    If some more of us can test it and verify that it's working as expected, then it can get committed to core.

    Damien Tournoud’s picture

    Title: Performance: Optimize CSS option causes php cgi to segfault in pcre function "match" » Optimize CSS option causes php cgi to segfault in pcre function "match"
    Version: 6.14 » 7.x-dev
    Priority: Critical » Normal
    Status: Needs review » Needs work

    I have found out that the regexp that was striping out the comments wasn't so well crafted, and that's why it was segfaulting PHP through endless recursion.

    Could you provide a sample input that triggers the segfault?

    Tri’s picture

    @Damien

    This code

    <?php
      $contents = '/*' . str_repeat('a', 60000) . '*/ Test1';
      $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);
      print $contents;
    ?>
    

    produces this
    http://www.webistas.gr/segfault.php

    while this code

    <?php
      $contents = '/*' . str_repeat('a', 60000) . '*/ Test1';
      $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);
      print $contents;
    ?>
    

    produces this
    http://www.webistas.gr/no-segfault.php

    Both the above are running on
    FreeBSD 7.2-RELEASE-p2
    Lighttpd 1.4.23
    PHP 5.2.11
    Suhosin-Patch 0.9.7.

    I hope it will help you verify it.

    snorkers’s picture

    I had PHP/Apache crashing with CSS/JS compression on a Win 2003 server. Just applied the patch #19. With compression applied and server still functioning, my YSlow grade has jumped 2 levels. Thanks for patch and I'm +1 for an update to core (no brainer IMHO).

    Not knowing much about this, is there still any merit in also making the PCRE increases for backtrack and recursion? I have the luxury of my own server with loads of memory so have no problem bigging up resources.

    Tri’s picture

    @snorkers
    I am sorry but pcre.backtrack_limit and pcre.recursion_limit will not help you get faster... In fact, your YSlow grade increased because you managed to activate css compression/aggregation without crashing the server by applying the patch, and not because of the patch itself.

    Talking about speed, the patch - besides fixing the original issue - runs also faster. Some rough-cast benchmarking yielded:

    Comment block size -> Patched version times faster than core version of regex
    100b -> 3,3
    500b -> 17,5
    1Kb -> 19,4
    2Kb -> 38,3
    5Kb -> 55,6
    10Kb -> 65,2

    Offcourse, this speed benefit applies only when drupal_load_stylesheet_content() function runs, so only the first visitor of a page will notice, nonetheless it's a welcomed sideffect.

    @Damien
    I am wondering what is the meaning behind status needs work. Is there something that needs to be worked out about the #22 patch?

    donquixote’s picture

    @Tri:
    Could you explain what was wrong with the old regex, so we can all learn something? That would be great!

    asb’s picture

    @donquixote:

    It wouldn't hurt too much to read the whole thread, especially the initial issue's description, before requesting "explanations" in #27, wouldn't it?

    Thank you!

    donquixote’s picture

    I did read the full thread, I do understand that the regex caused problems, and I am very happy that Tri found this solution.

    But, most of what has been said here was plain empirical tapping in the dark, until Tri found that a different regex exists that does the job and doesn't break.

    What I would be interested to know: Is this really a poorly written regex, or is it just the PCRE regex engine that does not like it? And if it is poorly written, then what exactly is wrong with it? Is it an invalid regex? Does it have performance problems? Maybe I can learn something for the next time that I have to build a regular expression myself - if Tri does not mind to write an explanation.

    Damien Tournoud’s picture

    /\*([^*\\\\]|\*(?!/))+\*/
    

    That code is really poorly documented, but it seems like the objective of this regexp is to capture all the CSS comments that do not end with \*/ (which is the old IE 5 mac hack).

    I'm not really sure that it makes sense to continue supporting this hack nowadays, but if we want to, this regexp could probably be simplified this way:

    /\*(.+?)(?<!/)\*/
    

    For D7, I would argue removing the support for IE 5 Mac hacks altogether.

    Tri’s picture

    Assigned: Unassigned » Tri
    Priority: Normal » Critical
    Status: Needs work » Needs review
    Issue tags: +Quick fix, +CSS aggregation
    FileSize
    1.1 KB

    @donquixote

    is it just the PCRE regex engine that does not like it?

    Regular expressions is a programming language in itself, considered by many as a "dark art", because of the complex internals that are hidden behind its condensed - often cryptic -syntax. It's very powerful, but with "power comes responsibility", as the old computing moto is saying.

    By feeding this code to PHP

    <?php
    function foo() {
      foo();
    }
    foo();
    ?>
    

    you won't blame the parser when it will segfault, because it's an obvious coding mistake. Doing the same though, through a regexp isn't so obvious...
    So no, it doesn't have to do with the engine.

    Is it an invalid regex?

    It's not an invalid regexp, because an invalid regexp doesn't even get parsed.

    if it is poorly written, then what exactly is wrong with it?

    Going in great detail about regexp theory is out of this thread's scope.

    In sort /\*([^*\\\\]|\*(?!/))+\*/ is matching super-linear, a regexp programming jargon for describing situations where the number of possible matches grows exponentially by the length of the fed text. The construct "anything not star, not backslash" combined with "or" and "lookahead" inside the "one or more", leads the engine to try all possible permutations, which means lots of backtracking.

    Does it have performance problems?

    First of all it has stability problems, which is why this thread was opened in the first place. After stability comes performance (see #26)

    I hope it's clearer now.

    @Damien Tournoud

    For D7, I would argue removing the support for IE 5 Mac hacks altogether.

    I believe that talking about css techniques and support for IE 5 Mac hacks is out of this thread's scope also. A fundamental concept of the Drupal architecture is never alter a user’s original data, so silently striping code that someone has chosen to include in his css through a core function is out of the question.

    Besides that /\*(.+?)(?<!/)\*/ doesn't work as expected since it's striping out \*/ hacks and doesn't strip multiline comments.

    I still don't understand the status switch from #23.

    So
    I am switching priority back to critical since this bug crashes Drupal if someone has long enough comment blocks in her css or it's preventing people to enable a core function.
    I am switching status to "needs review" since I don't see what needs to be worked out in patch from #22 and I am assigning this to myself.

    I am attaching the #22 patch against Drupal 6.14.

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    donquixote’s picture

    @Tri:
    Thanks a lot! This is exactly the kind of explanation I was hoping for. I am still scratching my head about the details, but this is now my own problem..

    About your latest patch: I think you need to suffix the filename with "D6" or something to avoid it being tested for D7?

    Tri’s picture

    FileSize
    1 KB

    Reroll against head.

    Tri’s picture

    Status: Needs work » Needs review
    Damien Tournoud’s picture

    I believe that talking about css techniques and support for IE 5 Mac hacks is out of this thread's scope also. A fundamental concept of the Drupal architecture is never alter a user’s original data, so silently striping code that someone has chosen to include in his css through a core function is out of the question.

    Supporting the IE 5 Mac Hack *is the source of the performance problem*. Discussing if we want to continue supporting this outdated (and as a consequence useless) hack if perfectly in the scope of this issue.

    Besides that /\*(.+?)(?<!/)\*/ doesn't work as expected since it's striping out \*/ hacks and doesn't strip multiline comments.

    The look-ahead expression ((?<!/)) is specifically designed not to strip out \*/ hacks. I'm not sure how the previous regexp managed to strip multiline comments.

    Anyway, we need to add tests for all that regular expression voodoo.

    mikeytown2’s picture

    mikeytown2’s picture

    Status: Needs work » Needs review
    Damien Tournoud’s picture

    Status: Needs review » Needs work

    We need tests, and I still don't know what you think is wrong with my simple /\*(.+?)(?<!/)\*/.

    donquixote’s picture

    Status: Needs review » Needs work
    FileSize
    3.11 KB

    We need tests

    Absolutely. Here is one.

    Result:
    - Tri's regex keeps the first part of the ie5 mac hack, but not the second part that ends the hack.
    - The old regex had the same problem (so it was quite pointless with ie5 mac hacks)
    - Tri's regex as standalone does not completely remove the comment. It needs the part that says "remove whitespace".
    - Damien Tournoud's regex removes the ie5 mac hack (as expected).
    - none of them causes a crash.

    EDIT:
    The things I pointed out about Tri's regexp are not relevant int he context of the patch: The patched version works the same as the unpatched version - this is what matters.

    donquixote’s picture

    Supporting the IE 5 Mac Hack *is the source of the performance problem*.

    This statement is inaccurate. The regex that is causing the problem was intended to support the IE 5 Mac Hack. This does not mean that supporting this hack requires a poor regex. If we can find a fast regular expression which successfully preserves the IE 5 Mac Hack, we shall use it.

    <?php
          $content = preg_replace('<
                  ((/\*.*\\\\\*/([^/]|/[^\*])*/\*([^\*]|\*[^/])*\*/)|/\*([^\*]|\*[^/])*\*/)   # Remove comments that are not CSS hacks.
                  >x', '\2', $content);
    ?>

    It works for my simple examples, but some other tests would be helpful.
    This one tries to preserve the trailing comment as well. If that is not what we want, we can further simplify.

    EDIT:
    Added a few characters to allow /* in the trailing comment.

    Btw:
    Noone cares about the length of the regular expression, as long as it is fast, and there is a documentation somewhere.

    Tri’s picture

    Status: Needs work » Needs review

    The ie5 mac hack comes in two flavors.
    The first is that if you insert a comment containing a backslash MacIE5 would ignore the next (single) rule, so no second part that ends the hack is needed.
    The second is that if the \ is immediately followed by the closing */, you can comment out a whole block of rules for MacIE5 and in this case you need a second part that ends the hack.

    The proposed regexp works for the first flavor as it did the original from core. I just rewrite it to be stable and efficient.

    Tri's regex as standalone does not completely remove the comment. It needs the part that says "remove whitespace".

    The proposed patches are leaving the "remove whitespace" part of the regexp intact, only the "remove comments that are not CSS hacks" is changed, I don't know why it exists in your test as "case 4".

    In my tests both regexp from #41 and #39, are striping out every comment regardless of hacks or not.

    donquixote’s picture

    Case 4 exists more out of curiosity - I like to understand things.
    But I would say yes, the patch does the job in my test cases, in that it behaves the same way as the old version.

    Tri’s picture

    Status: Needs work » Needs review
    FileSize
    1.46 KB
    1.32 KB
    1.32 KB

    I have worked out the patch some more and this version respects both flavors of ie-mac hack, multiline or single line.

    The trick is to add a backslash also at the end hack block in a first pass (it doesn't hurt ie-mac as my tests have shown) so the second pass which strips the comments doesn't touch it.

    I am attaching patches for d6 and head along with a test script that checks for 60K comment blocks, both versions of the ie-mac hack and multiline, multistar comment blocks. It also displays the elapsed time to complete the tests.

    Please test it and report back.

    Tri’s picture

    The patches/files as #44 with a small fix. #44 was adding an extra \ also at the begging ie-mac hack comment block.

    Now it's not.

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    Tri’s picture

    Assigned: Tri » Unassigned
    FileSize
    1.35 KB
    1.22 KB

    There was an other css agreegation related bug here drupal_load_stylesheet_content() removes whitespace improperly, creating invalid CSS which ended up commiting a patch that removed the "# Remove line breaks" part of the regexp discussed in this issue.

    The regexp is still segfaulting on large enough comments so I am rerolling my patch against HEAD. Unfortunately, I am too busy at the moment to educate myself about writing simpletests, as the guys over that issue did. I understand that this is the correct way to go, but I can't for now. In the attached zip, you will find a css file that can be used to prove the bug, so someone fluent in writing simpletests is welcomed to use it.

    Just to sum up what has been done in this thread, in order to spare new comers here from reading the whole thing, besides the original segfaulting on large comments bug, an other bug was found were ending ie-mac hack comment blocks were erroneously getting stripped out. The proposed patch fixes them both.

    Tri’s picture

    Status: Needs work » Needs review
    JohnAlbin’s picture

    subscribe

    rfay’s picture

    We're battling an unwinnable battle here with the wrong tools: You can't successfully parse CSS with regular expressions. Several minor improvements have been committed in D7, but for D8 we definitely need to parse CSS: #494498: Add CSS parsing capability to Drupal.

    That said, we still need to address this for D7.

    kentr’s picture

    @Tri,

    Thanks so much for this work. This bug was driving me crazy.

    Manually applied #47 to 6.14 and it fixed the crashed site problem with no other apparent side-effects.

    Since in my ignorance I'm guessing that the testing system uses the version number in the issue data, I'm going to post the D6 patch here #331915: Performance: When enable optimization for css and javascript ... site goes down instant!.

    mikeytown2’s picture

    Status: Needs review » Reviewed & tested by the community

    fix for bug works for me as well

    donquixote’s picture

    @rfay (#50):

    You can't successfully parse CSS with regular expressions.

    Is this a documented fact from computational complexity theory? Or is it just "difficult" ? Remember, we are not completely parsing CSS here, we only want to get rid of comments. That's a smaller problem, I would say (but again, don't know if that's a proven fact).

    Tri’s picture

    @kentr
    I am glad that I helped :). The patch for D6 is actually at #45. The testbed says "failed" for the D7 patch, it doesn't check for D6. I am using it in all my D6 sites and it works flawlessly.

    I am also glad that there is life again in this thread. Since status is now RTBC, I believe it might be time for the patch to make it into core. Objections anyone?

    rfay’s picture

    I should mention that #472820: drupal_load_stylesheet_content() removes whitespace improperly, creating invalid CSS (mentioned in #47 on its commit to D7) also got committed to D6 the other day, which will (I believe) cause the D6 patch from #45 to need a re-roll.

    Tri’s picture

    FileSize
    1.26 KB

    @rfay

    You are right. I missed the D6 commit of your patch. So here is a re-roll against D6.14.

    marcvangend’s picture

    Subscribe, I'm seeing this problem on my Windows machine too.

    mikeytown2’s picture

    @marcvangend

    Does the patch fix the issue for you? If so please say so.

    JohnAlbin’s picture

    Status: Reviewed & tested by the community » Needs review
    FileSize
    565 bytes

    I think we should remove support for the IE5mac hack in D7. It was released in 2000, hasn't been updated since 2003, is unavailable for download since 2006. And most importantly, NO ONE USES IT.

    Its nonsensical to jump through hoops to make sure the hack continues to work in Drupal 7. Support for it is already in D6, so I guess we'll have to leave it. But if we can fix the segfault problem and simplify the regex in D7, we should do it.

    Is that regex supporting any other comment-related hacks besides the IE5mac one? It doesn't look like it. Here's a detailed explanation of the /\*([^*\\\\]|\*(?!/))+\*/ # Remove comments that are not CSS hacks. regex that is currently in D7:

    /\*       # match "/*"
    (         # start of matching sub-pattern
    [^*\\\\]  # match a single character that is not "*" or "\"
    |         # OR
    \*(?!/)   # match "*" that is not followd by "/" (memory-exploding negative look-ahead)
    )         # end of matching sub-pattern
    +         # match one or more of the sub-pattern above
    \*/       # match "*/"
    

    In the attached patch, I've removed the check for a backslash in a comment and modified Tri's regex very slightly. BTW, Tri, your regex is awesome; I'd have never thought of it. Here's a break down of the regex in my patch:

    /\*       # match "/*"
    [^*]*     # 0 or more characters that are not "*"
    \*+       # 1 or more "*" characters
    (         # start of matching sub-pattern
    [^/]      # single character that is not "/"
    [^*]*     # zero or more characters that are not "*"
    \*+       # one or more "*"
    )         # end of matching sub-pattern
    *         # match zero or more of the sub-pattern above
    /         # match "/"
    

    Still needs simpletests.

    marcvangend’s picture

    @mikeytown2: I had this problem in two D6 sites I copied to localhost for upgrades and new features. On one site the patch from #56 solved the problem, the other site I haven't tested yet.

    By the way: many thanks to all of you who are working on this. Personally, I suck at regex, so I'm glad I don't have to fix this :-)

    Tri’s picture

    @JohnAlbin

    I think we should remove support for the IE5mac hack in D7

    I believe you are mistaking css parsing with css authoring. This issue has to do with css parsing - any css that someone might wish to include in his site, has to stay intact - functionally speaking. I agree with you that IE-Mac is outdated but this is a matter of css authoring. This thought came up again in this very thread (#30, #31 second part, #36 & #41).

    ...and simplify the regex...

    Regexp's were never meant to be read by humans. They are crafted with the machine in mind. In the regexp world, the easier for the eye - the harder for the machine and vise-versa, holds true.

    ...jump through hoops to make sure the hack continues to work in Drupal 7.

    Actually I don't see any serious hoops. The proposed patch is in fact two lines of code and it is not even PHP. A few character classes and a few greedy repeats is all that is. From this we get a) stability (from what other people have verified in this thread b) speed (see #26) and c) IE-Mac support for whom might want it (improved! (#44)).

    About the simpletests, I understand that it is the defacto way to test patches in Drupal, though, since the patch is plain regexp stuff - no PHP or Drupal constructs used, I believe it can be tested standalone... Since I am not familiar with simpletest works, I can't write one myself, so if anyone can help on that it would be nice.

    I am re-attaching (not re-rolling - so people that have tested it so far don't have to bother reapplying) my patches for D7 & D6 from #47 & #56 so people coming here searching, can find the one they need without jumping from post to post. Also in the zip there is a .css for tests and .php to test standalone.

    mikeytown2’s picture

    I would call #61 RTBC

    JohnAlbin’s picture

    I believe that talking about css techniques and support for IE 5 Mac hacks is out of this thread's scope also. A fundamental concept of the Drupal architecture is never alter a user’s original data, so silently striping code that someone has chosen to include in his css through a core function is out of the question.

    And if you don't enable CSS optimization, your IE5mac hack will remain. But if you want Drupal to alter your CSS so that it is optimized, I don't see the problem with stripping comments. We should not add extra code to support IE5mac in D7's CSS optimizer. I agree with Damien on this point.

    Regexp's were never meant to be read by humans. They are crafted with the machine in mind. In the regexp world, the easier for the eye - the harder for the machine and vise-versa, holds true.

    Readability is not what I meant when I said "simplify". I meant less code == less time to compile the regex == faster execution. Your patch does simplify the existing regex (can't say enough good things about that part of your patch), but it also adds an additional call to preg_replace() per stylesheet. On a site with 35 stylesheets, that's an additional 35 calls to preg_replace just to support IE5mac. That's what I'm objecting to.

    Also, your speed tests are against a version of the patch before you added the extra preg_replace().

    Finally, your /\*[^*\\\\]*\*+([^/*][^*]*\*+)*/ contains an unneeded * in it. Temporarily ignoring the fact that the backslash checking should be removed, it should (at the very least) be: /\*[^*\\\\]*\*+([^/][^*]*\*+)*/ because the regex part just before the [^/] is a greedy \*+ so [^/*] would never match a *.

    kentr’s picture

    @JohnAlbin:

    Thanks for looking at optimizing the regex. Would you mind testing your regex and rolling a patch?

    Either way, IMHO it's important to get *something* committed so that CSS aggregation won't break sites - to help people who are using D6.

    Tri’s picture

    John, thanks for the kudos and for revising my regexp. Though,

    the regex part just before the [^/] is a greedy \*+ so [^/*] would never match a *

    is not true. Greedy repeats are "non-capturing", which means that they can "lend" a character if the following pattern needs it to match. So *+([^a])foo will match "****foo" while *+([^a*])foo will not.

    About IE-Mac, I see your point. All this regexp talking is scaring people away and they don't get involved in something that is IMHO a matter of decision taking about how to strike a balance between efficiency and "politic correctness". I am going to sum up the possible choices in one post so the community can make up their mind.

    The ie5 mac hack comes in two flavors.
    The first is that if you insert a comment containing a backslash, MacIE5 would ignore the next (single) rule, (lets call it hack v.1).
    The second is that if the \ is immediately followed by the closing */, you can comment out a whole block of rules for MacIE5 and in this case you need a second comment part after the block of rules that ends the hack, (lets call it hack v.2).

    The original implementation from core supports hack v.1 only (when it doesn't segfault on large comment blocks). So the choices we have are:

    1) Drop the IE-Mac hack all together.
    This will be the fastest of all. Scores high on efficiency but alters the functionality of the original css. If we follow this path, we have to place a warning at the "Performance page" bellow the "Enable CSS optimization" about hacks not supported.

    2) Support hack v.1 only as the original did.
    This is the patch from #22 (needs re-roll for current HEAD). This is something like 20-60 times faster than core (#26). Still alters the functionality of the original css and will need a warning about multi line hacks not supported.

    3) Support both hack v.1 & hack v.2
    This is the patch from #61. Some rough testing shows that it is 3-4 times faster than core.

    The speed benefits applies only when drupal_load_stylesheet_content() function runs, so only the first visitor of a page will notice.

    Please everybody say what you believe is the correct way to go. After the path will be shown, we can use the correct regexp to take us there...

    marcvangend’s picture

    I vote for option 1. I haven't had to support IE5 in ages and the gain in performance is much more important to me.

    IE5 for Mac was replaced as the default browser by Safari in 2003, with the release of OSX 10.3. Do you remember? That's when iMacs looked like this. Drupal doesn't even try to be compatible with it's previous major release, so I don't see why we should sacrifice performance IE5 for Mac.

    If the majority here decides that full support of IE5 hacks is important, I want to propose to offer three choices on /admin/settings/performance:

    Optimize CSS files:
    O  Disabled
    O  Performance mode (faster, can remove CSS hacks for older browsers)
    O  Compatibility mode (slower, respects all CSS hacks)
    
    rfay’s picture

    I should point out that unless I'm off track here, the performance issues *only* come into play when the aggregated CSS has to be rebuilt, as when CSS Aggregation is turned on or when a file is changed, making it far less of a real performance issue - it does not occur on every page load, but rather more when administrative events happen.

    donquixote’s picture

    Does Safari work on older Macs too?

    kentr’s picture

    I should point out that unless I'm off track here, the performance issues *only* come into play when the aggregated CSS has to be rebuilt, as when CSS Aggregation is turned on or when a file is changed, making it far less of a real performance issue - it does not occur on every page load, but rather more when administrative events happen.

    Definitely not on every page load. What about rebuilding the cache? But, optimization in general is a good thing, so this would be a good time (while it's in our face) to implement an acceptable optimization lest it get lost in the shuffle...

    Did the code that attempted to remove comments while preserving the Mac hack ever work reliably? If not, take it out, because it wasn't ever something people could depend on anyway.

    But if it did work, then it was a core feature of Drupal, so removing it doesn't seem to be our call. In this case, i vote Tri's option #2. Adding support for hack v.2 if it was never in the first place is a step backwards, IMO.

    I like marcvangend's solution for providing the option to the user - at least for D7. If it's realized and easily backported to D6, nice but not a big deal.

    rfay’s picture

    @kentr, yes, nearly the only time this performance would be an issue is at cache rebuild time. In my world-view of performance optimization, that makes it irrelevant to even think about because that's a very unusual event.

    marcvangend’s picture

    @tri, @rfay: you're right, the code isn't executed on every page load. I guess I forgot about that because I've seen the error occur again and again and again...

    If performance isn't really an issue here, I definitely think we should choose option 3. Not because I suddenly care about MacIE5, but because it's better for usability/UX to avoid warnings whenever possible. I can imagine that many site administrators don't even know if their theme uses MacIE5 hacks. It would be a shame if they don't dare to turn on CSS optimization, just because they cannot judge if the warning applies to their situation.

    mikeytown2’s picture

    Agree as well, do it right the first time. Allow this to not fail on CSS hacks.

    donquixote’s picture

    Admin performance (with "rebuild theme registry on every page") is my time and my money, so...
    well, but in this particular case, it's not such a big difference.

    I have never bothered to implement any IE Mac support in my projects, and probably none of those posting in this thread have, but that's not really the point is it?

    I did a quick search for "ie mac site:drupal.org"
    http://www.google.de/search?hl=de&client=opera&rls=de&q=site%3Adrupal.or...
    Some of these issues contain posts from 2007, which is not that long ago.

    And one quote I found in a mailing list:
    http://lists.drupal.org/pipermail/themes/2005-December/000088.html

    > No-longer-supprted does not mean no-longer used (unfortunately).
    >
    > And don't forget that a lot of Mac(hardware) cannot run anything else then
    > ie5! These people have no choice but to continue on that broken browser.

    Exactly what I said - still used on older machines :(
    Some stats would be interesting! And certainly more relevant than some personal opinions posted in this thread.

    EDIT:
    Ok let's get 3) in. It's not a bottleneck even for admin pages, so why care?

    mikeytown2’s picture

    Status: Needs review » Reviewed & tested by the community

    Everyone is in agreement here that we leave in the macIE css hacks correct?

    #61 is RTBC

    crutch’s picture

    Subscribe (ref D6 issue: http://drupal.org/node/331915)

    Dries’s picture

    The code comments in #61 are missing a space after the pound character. Needs quick reroll.

    Dries’s picture

    Status: Reviewed & tested by the community » Fixed

    Rerolled it myself and committed it to CVS HEAD. Thanks.

    marcvangend’s picture

    Thanks Dries.
    If I understand correctly, 'CVS HEAD' means that is fixed in D7 - or will it be rolled into D6 as well?

    mikeytown2’s picture

    Version: 7.x-dev » 6.x-dev
    Status: Fixed » Reviewed & tested by the community

    We need it for 6.x now

    #61 is RTBC

    Tri’s picture

    FileSize
    1.26 KB

    I would like to thank everybody in this thread for their help in reviewing, testing and voting for the various options - especially, Donquixote for discovering the hack v.2 bug and JohnAlbin for revising and thought provoking objections.

    See you everybody at the next issue.

    I am attaching patch from #61 for D6 with the missing spaces.

    andypost’s picture

    +1 RTBC, Tested patch from #80 under

    Apache/2.2.9 (FreeBSD) DAV/2 PHP/5.2.6 mod_ssl/2.2.9 OpenSSL/0.9.8e
    FreeBSD h2.itl.ua 7.1-PRERELEASE FreeBSD 7.1-PRERELEASE #0: Wed Oct 29 09:48:51 UTC 2008 axl@h2.itl.ua:/usr/obj/usr/src/sys/H2 i386 
    

    There's little difference within 6.14 and 6-dev caused by #472820: drupal_load_stylesheet_content() removes whitespace improperly, creating invalid CSS

    ivanstegic’s picture

    Tested the patch from #80 under

    Apache/2.2.14, PHP/5.2.11, FreeBSD delsi.pair.com 7.2-STABLE FreeBSD 7.2-STABLE #0: Wed Oct 21 12:38:36 EDT 2009 erik5@kyack.pair.com:/usr/obj/usr/src/sys/7PAIRe i386

    and it works great!

    casey’s picture

    Damien Tournoud’s picture

    Version: 6.x-dev » 7.x-dev
    Status: Reviewed & tested by the community » Active

    This apparently broke the very thing it was supposed to support.

    From #642974: CSS aggregation removes multi-line ie-mac hacks when the hack rules contain a star:

    CSS aggregation removes ie-mac hack which is being used in defaults.css.

    // from defaults.css
    $contents = <<<EOS
    .clearfix {
      display: inline-block;
    }
    
    /* Hides from IE-mac \*/
    * html .clearfix {
      height: 1%;
    }
    .clearfix {
      display: block;
    }
    /* End hide from IE-mac */
    

    Result:

    .clearfix{display:inline-block;}/* Hides from IE-mac \*/ * html .clearfix{height:1%;}.clearfix{display:block;} 
    

    So let's just cut the crap and remove support for this IE-mac hack completely.

    And: why in hell was that committed without the corresponding tests? Frankly, we should not accept any regexp-that-nobody-understands without a comprehensive test suite.

    casey’s picture

    I am working on the CSS aggregator: #641472: Fix CSS Aggregator.

    Latest patch still contains that bug, but i am working on it.

    Tri’s picture

    This apparently broke the very thing it was supposed to support.

    This fixes the original segfault bug and supports v.1 mac hacks exactly as the original from core did. The #84 has to do with v.2 (multiline) mac hacks. Since v.2 mack hacks are a kind of a "new" feature, I believe it has to be discussed in its own thread an not here.

    Let's do it here #642974: CSS aggregation removes multi-line ie-mac hacks when the hack rules contain a star

    donquixote’s picture

    Version: 7.x-dev » 6.x-dev
    Status: Active » Reviewed & tested by the community

    @Tri:
    I don't get it. I thought we had option 3 ? If 3) removes the hack, it does not do what it is intended to do.

    @Damien Tournoud

    > So let's just cut the crap and remove support for this IE-mac hack completely.

    Then why does some core CSS still use this hack? If this was discovered in even core's common.css, then I would not be surprised if a bunch of modules and themes do also have css hacks.

    Tri’s picture

    @donquixote
    We do have v.2 mac-hacks as stated in 3). But Casey at #83, discovered a bug: If the block of rules of the v.2 hack contain a star, the first preg_replace fails to detect the second comment part that ends the hack in order to add a slash to it, so the second preg_replace just strips it off.

    Since this thread has come a long way and started as a critical segfault bug, I felt is better to address this bug in a thread of it's own rather than reopening this one.

    Gábor Hojtsy’s picture

    Version: 6.x-dev » 7.x-dev
    Status: Reviewed & tested by the community » Needs work

    Let's not move fixing bugs introduced by this patch to a different issue, or we will have a hard time figuring out when/how is this ready for D6 (which it does not yet seem so). Also, Damien seemed to have other outstanding issues as well, like adding tests for this.

    Tri’s picture

    Status: Needs work » Needs review
    FileSize
    124.36 KB

    Ok, so the action comes back here again from #642974: CSS aggregation removes multi-line ie-mac hacks when the hack rules contain a star.

    I am attaching a new patch that uses a different approach to the conditional comment stripping, since trying to identify start & end v.2 ie-mac hack comment blocks through regexps is getting too complex. I use a regexp to match all comments and comment strip logic is implemented in a function.

    In case you are worried about performance: In some rough benchmarks I have done, I found that it takes ~0.040sec to proccess 25 css files coming from a D6 setup with zen, views, admin_menu, date_popup, filefield & panels on my rather slow dev box.

    I added a css unit test to check for mac hacks v.1 & v.2. It also has a huge 60Kb comment block to test for segfaults, so don't get scared by the size of the patch, it's just the test css files.

    Please test it and report back.

    flaviovs’s picture

    Patch #90 worked for me on my 6.15 install (had to tweak the patch a little to make it apply). Now I can re-enable CSS aggregation on my sites.

    BTW I think this is the direction to go. Trying to match hacks in multiline comments using a single (multi branch) regexp may be problematic, IMHO.

    @Tri, if I understand your patch correctly, you moved the comment stripping logic to another preg_replace_callback() call, but kept the branch separator in the original regexp, however there's no need for it anymore. You can chop off the "|" separator from the regexp.

    ridgerunner’s picture

    Status: Needs work » Needs review

    Tri, your regexes from post 61 and 90 can be improved upon.

    Point #1:
    The second regex from post 61 and the first regex from post 90 both share the same efficiency problem. Each of these has an extraneous '|' at the very end which effectively becomes a NULL alternative that allows the regex to *always* match the nothingness between each and every character in the string. The preg_replace function will happily match, (and time-consumingly replace with the empty string captured in /1), all those nothingnesses that were not already matched as whitespace surrounding separators and not CSS hack comments. Removing this unnecessary '|' should significantly improve their performance.

    Point #2;
    If I'm not mistaken, (and please correct me if I'm wrong), these regexes are *way* more complicated than they need to be. With regard to the task of stripping out non-mac-hack (type 1 and 2) comments , here is an improved version of your 2-regex solution that is smaller, faster, simpler and uses less memory: (and these regexes completely avoid any of the segfault memory overflow pitfalls)

    // Step 1: Convert the first comment following each type 2
    // mac-hack comment, into a type 1 mac-hack comment
    $contents = preg_replace('%(\\\\\*/.*?)/\*%s', '$1/*\\', $contents);

    // Step 2: Remove all regular, non-mac-hack comments
    $contents = preg_replace('%/\*[^\\\\]*?\*/%', '', $contents);

    Please let me know if my logic is flawed or if there are any additional cases where these regexes fails to work. (I am aware of a few corner cases, but I'll save them for a later post.)

    I hope this helps! =)

    Edit 2010-01-09: I see that flaviovs in the previous post 91 also noticed the extraneous '|'.

    Edit 2010-02-18: Although the two regexes presented here are simpler to read, and do function correctly, they can be improved upon with regard to speed and backtracking efficiency. A much improved solution is in the works. Standby...

    Issue tags: -Quick fix, -CSS aggregation

    Re-test of common.inc-90.patch from comment #90 was requested by ridgerunner.

    Status: Needs review » Needs work
    Issue tags: +Quick fix, +CSS aggregation

    The last submitted patch, common.inc-90.patch, failed testing.

    redearthbluesky’s picture

    Hi,
    I thought I would throw my two pents worth into this, as the whole thread fizzled out with a whimper rather than a satifying thunk - leaving some doubt in my mind in the end if there was actually a fix for 6.15.

    I have a zen subtheme using drupal 6.15, and I used the fix in #45 to manually update my includes/common.inc file at around line 1967, and this worked great guns when I enabled the optimisation of css in performance. I have now declared an ie7 css file in my .inc file, copied the ie.css from the zen folder into my theme folder and and renamed it ie7.css, and have just finished modding my ie7 site so it is now identical to my ffox et al one.

    cheers all and thanks for the great fix - if a rather obtuse thread to present it,

    guygg’s picture

    Like #95 above, I was able to manually make the changes in the #45 patch file to the 6.15 version of common.inc, and that fixed the problem for me as well. Hopefully the fix for this problem will get rolled into the 6.16 release.

    Thanks, redearthbluesky...

    kentr’s picture

    #90 manually-applied to 6.15 fixed the bug on my site, no apparent new problems.

    dicreat’s picture

    FileSize
    1.75 KB

    #90 solve my problem after manually-applied on Drupal 6.15.
    Patch with fix in attach.

    ridgerunner’s picture

    Status: Needs review » Needs work

    There is a problem with all the solutions presented so far - they all ignore the fact that the CSS :before and :after pseudo-elements can utilize the content property, and this property is allowed to have arbitrary content contained within a string that should not be modified. Let me play the devil's advocate and present the following (admittedly contrived yet completely valid) CSS 2 markup:

    .comment:before {
        content: "/* ";
    }
    .this_rule_gets_deleted {
        color: #F00;
        background-color: #FFF;
    }
    .comment:after {
        content: " */";
    }

    The drupal_load_stylesheet_content() Drupal function will dutifully (and erroneously) delete this valid CSS content! - i.e. everything between the "/* " and " */" strings in the example markup above. The problem is that none of the PHP code presented thus far properly handles strings. (In fact, they are not considered at all.) A correct solution should not modify anything contained within single or double quoted strings. Additionally a correct solution should properly handle any escaped quotes that occur within a string (e.g. "String with \"escaped\" quotes"). Note that this problem is independent of the MacIE5 comment hacks and should be addressed even if support for MacIE5 is dropped.

    Inspired by Tri's solution presented in post #90 - which utilizes a finely crafted comment matching regex that was taken directly from chapter 6 of Jeffrey Friedl's excellent work: Mastering Regular Expressions - 3rd edition, - I've been working on an improved solution that correctly handles all these issues. I will be presenting a patch with this solution soon.

    Note that this problem may warrant an entirely new issue, but I'm new to this community and am not sure of the proper protocol, so I am posting it here.

    marcvangend’s picture

    @ridgerunner: that certainly is an edge case, but very clever nonetheless. In the art of regex, I'm not even an apprentice, so I welcome everyone who can help fix this bug. Still I would say that your improvement should be a separate issue. IMHO, this bug has been around for way too long now and I would like to see it fixed in Drupal 5, 6 and 7 as soon as possible. I think we should just choose the best fix with equivalent functionality and get it committed. New features can be discussed afterwards in a new issue, without the risk of derailing this one.

    Tri’s picture

    FileSize
    128.74 KB

    Hello everybody.
    I have just recovered from a terrible bike crash that almost killed me and kept me away from computers for quite a while...
    Now,I am glad to see action in this thread.

    @flaviors-ridgerunner
    You are right about the extraneous '|', it has to go.

    @ridgerunner
    You are right that the quoted /* & */ are breaking the wanted functionality.
    Thanks for pointing outMastering Regular Expressions. I managed to get a copy of the 2nd edition from the library and I have to say that I am amazed by the quality of this book. I wasn't aware of it but I have to say that everybody who is interested in regexps definitely needs a copy.

    Anyway the writer has a thorough analysis on the comment striping problem and he is also mentioning the quoted comments case. He is using a technique to assemble complex regexps, utilizing substrings which I find very clever. So I am attaching a new version of my patch which uses this technique along with the solution he proposes for the quoted comments. I have added tests for double & single quoted strings. I have also made some small whitespace modifications to comment_hacks.css.optimized.css in order pass the tests, since by the way drupal_load_stylesheet is working now, is striping some LFs more.

    Please everybody test it and report your findings.

    For everyone who wants to compare this with the solution proposed by Jeffrey Friedl: While he is going to great depth in optimizing his final regexp, those optimizations don't apply in this case since there is the preg_replace_callback(). Using his original solution makes the callback to be called very often, which beats the optimization. In some tests I have done with both solutions it is almost the same, so I decided to use this one which is simpler.

    Tri’s picture

    Status: Needs work » Needs review
    marcvangend’s picture

    @Tri: sorry to hear about your accident, it's good to see you back here.

    ridgerunner’s picture

    You are on the right track with the "<$double_quot|$single_quot|$comment>" callback technique, but there are still some deficiencies with and improvements to be made to your attached patch. I'll be providing feedback in the form of a similar but much more comprehensive solution that I am currently putting the finishing touches on and will be posting here later today.

    I'm glad you discovered the MRE book! I can honestly say that this is the most useful book I have ever read in my life. (I've read it three times so far, but working on this Drupal issue forced me to go back and read chapter 6 yet again.) In my view there are two kinds of people in the world: Those who have read and comprehended Mastering Regular Expressions, and those who haven't. But you *do* need to get your hands on a copy of the third edition where he added extended and thorough coverage of PHP and the PCRE engine which drives it.

    Welcome back and get well soon!

    ridgerunner’s picture

    Priority: Critical » Normal
    FileSize
    4.43 KB
    5.1 KB

    Attached is my first patch submitted for review. It purports to fix the issue that is the topic of this thread. It also addresses the issue about string handling I raised in post 99 above. This patch provides the following benefits:

    • Strings are properly handled and their contents are not modified.
    • Both types of MacIE5 comment hacks are properly handled. The text within the MacIE comments are also compressed and a short ID tag is added. e.g. "/*\T1*/" for type1 and "/*T2\*/ ... /*T2E*/" for type 2.
    • The CSS code is aggregated, but a little bit of formatting is applied so that it is still user readable. i.e. A line break is added after every rule following the closing curly bracket. A newline is added to the end of the file to guarantee no problems arise if CSS files are appended together.
    • It addresses the Some CSS rules are broken once CSS aggregation is enabled issue, by NOT clearing whitespace in from of colons. The handling of whitespace surrounding punctuation in this patch was split from one regex to two to handle pre and post whitespace separately.
    • The layout of the regular expressions are in a logical manner that is easy to grasp and maintain. Like Tri's latest solutions, it utilizes a preg_replace_callback function. There is simply no other way to easily handle the complex logic required to properly deal with the MacIE5 hacks.
    • The regular expressions used are highly optimized for speed and require very little backtracking. There should be no segmentation fault problems with this solution with reasonably sized CSS files. Very large CSS files may still cause memory issues. This needs to be tested.

    To a regex novice, the regular expressions presented here may appear to be large and complex. But rest assured, they are carefully crafted to achieve speed and correctness in accordance with the best practices presented in Mastering Regular Expressions - 3rd Edition. Most of these regexes implement the very effective and efficient "unrolling-the-loop" technique presented in chapter 6 which minimizes backtracking resulting in both reduced memory requirements and faster execution speed. The main behemoth regex - (the: One Regex to Rule Them All!), attempts to faithfully follow the "Well-Guided Regex is a Fast Regex" technique which is the culmination of Friedl's chapter on efficiency.

    In addition to the patch, I've also attached a PHP script that implements the exact same code. You can run it from the command line to minify a CSS file specified. The script writes the minified output to a new file with '_min" appended to the file name.

    I have not yet tested this patch out using the Drupal automated simpletest suite but I have manually tested it on some worst case test files here on my local system. As I said, this is my first contribution to the Drupal project, so I'm more than just a little bit ignorant of the proper protocol and procedures, but I think the patch should apply ok. Any help you could provide me would be appreciated. I would like to specifically thank Tri for his efforts posting solutions to this thread which sucked me in here and got me hooked on attempting to solve this non-trivial problem!

    Status: Needs review » Needs work

    The last submitted patch, rr_common.inc-105.patch, failed testing.

    ridgerunner’s picture

    Status: Needs work » Needs review
    FileSize
    5.01 KB

    It appears that CVS patches must have unix \n line endings. Lets try this again...

    marcvangend’s picture

    Priority: Normal » Critical
    Status: Needs review » Needs work

    Setting back to critical, assuming ridgerunner changed the priority by accident.

    marcvangend’s picture

    Status: Needs work » Needs review

    Oops, guess we were writing comments simultaneously.

    ridgerunner’s picture

    You are correct. I did. Sorry about that!

    Status: Needs review » Needs work

    The last submitted patch, rr_common.inc-107.patch, failed testing.

    Tri’s picture

    Status: Needs work » Needs review
    FileSize
    2.7 KB
    2.68 KB
    128.87 KB

    Here is another roll of my patch. Functionality is the same with #101 with some aesthetic fixes to comply with the Drupal coding standards. I also added the S "Study" modifier at the regexps which seems to speed up execution by 250%(!). In order to perform some benchmarking, I took the css files from a vanilla D6 install with admin_menu, date, devel, imagecache, imagefield, views, wysiwyg and zen along the comment_hacks.css from the patch and I concatenated them in one file that many times so the final css was 25Mib big. Using this file as source I tested the patched drupal_load_stylesheet_content() and it took ≅2.7 sec on my rather slow devbox. For comparison, patch #101 (without the "S") took ≅7.9 while patch #107 took ≅3.9.

    Summing up (once more) the works in this thread - which has come a long way, I agree with marcvangend #100 -, this latest patch:

    • Doesn't segfaults on large comment blocks.
    • It respects IE-Mac hacks both v.1 and v.2.
    • It doesn't break on single or double quoted comment delimiters. Escaped string delimiters are supported also.
    • It has tests for all these.
    • It's future proof, since any css comment "trickery" might come up in the future (I hope not:)), can be implemented by modifying the callback without messing with regexps.
    • It's quite fast.

    Sounds like a good deal to me.

    I am attaching patches against D6-dev and D5-dev also, because I see users coming here seeking for solution and applying the patch manually, so these people can test this on their installations. It's practically the same for all versions. Please everybody take the pain to apply it and try to temporarily add to your css some IE-Mac hacks and some quoted comment delimiters - even if you don't need them, so we can properly test this. If it doesn't break, maybe we can get it committed and end this css aggregation quest (for now).

    @ridgerunner
    Nice job on these regexps. Though, I believe there is some unneeded complexity in the way you approach the problem. There is no need to have separate regexp chunks to catch v1,v.2 ie-mac hacks and normal comments since all these are just comments, so they can be caught all together and get processed easier and faster in the callback. Even if the regexps are well written, still there is overhead to execute them, so it makes sense to keep them as minimal as possible. Also there is no need to take steps to make the output user readable, since it is not supposed to be read by humans. It has to be as small as possible, while preserving the original functionality. About the #460448: Some CSS rules are broken once CSS aggregation is enabled issue, there is a tendency in the community to handle things "one patch at a time". This makes testing and committing easier. I don't know if you agree, but I think it's better to join forces in order to get the best possible solution in terms of speed, stability and elegance of our code than working individually on separate solutions...

    ridgerunner’s picture

    Hey Tri, I took a closer look at your patch and have some feedback about it (and the aggregation function in general). Here are some comments in no particular order:

    1. With the type 2 hack, CSS is ignored between the opening comment: '/* \*/' and the closing comment: '/* */'. Unlike all other browsers, MacIE5 respects the escaping of characters within a comment and the hack works because the asterisk of the closing comment sequence is escaped like so: '\*/'. Thus the comment is effectively extended to the end of the next "regular" comment that has a proper, (unescaped) closing sequence '*/'. However, if two type 2 hack comments are placed in a row (unlikely, yes, but possible), the MacIE5 browser will skip both of these and continue the comment until a non-escaped closing sequence is encountered. The code logic of this patch, however, does not replicate this behavior and erroneously ends the comment on the second type 2 comment. (To work correctly, it should go on and preserve the third one which actually has the closing '*/' sequence.) Here is some code that demonstrates this deficiency:
      /* begin MacIE5 type2 hack \*/
      .this_rule_ignored_by_macie5 { color: #FFF; }
      /* another (unlikely) type 2 opening comment skipped by MacIE5 \*/
      .this_rule_also_ignored_by_macie5 { color: #FFF; }
      /* the "real" closing comment here */

      The patch preserves the second type 2 comment, then goes on and deletes the third comment that contains the actual closing comment sequence. The MacIE5 browser extends the comment further into the document, stopping who knows where. Admittedly, this is an unlikely scenario, but it is possible. (About as likely as anyone is still using MacIE5 I might add! ;^) Disclaimer: I have no way of knowing whether or not the preceding argument is actually true, since I have no access to an old mac running IE5. But it seems logical and very likely to me.

    2. The patch fails to address the issue raised in the: Some CSS rules are broken once CSS aggregation is enabled thread. i.e. Whitespace should NOT be removed ahead of the ':' colon character.
    3. This patch could more aggressively remove multiple blank lines and consecutive spaces. e.g. Multiple linefeeds following a type2 hack are not consolidated but could be.
    4. Both type 1 and type 2 comments are not themselves being cleaned of unnecessary text. These could be consolidated to their minimal forms. i.e. '/*\ */' and '/* \*/'.
    5. Although string contents are preserved with regard to opening and closing comment sequences, they are not preserved when it comes to the cleanup done around punctuation characters. The following illustrates this problem.
      Before aggregation:
      .add_hello_world:after {
        white-space: pre;
        font-family: monospace;
        content: '\A#include <stdio.h>\A\
      int main()\A\
      {\A\
        printf("Hello, world!" /* this comment is ok, but... */\A\
          "The { formatting } , of this :after string is modified!"; );\A\
        return 0;\A\
      }';
      }

      After aggregation:

      .add_hello_world:after{white-space:pre;font-family:monospace;content:'\A#include <stdio.h>\A\
      int main()\A\{\A\
        printf("Hello,world!" \A\
          "The{formatting},of this:after string is modified!";);\A\
        return 0;\A\}';}

      Note that the patch I submitted is guilty of a similar offense (it strips leading whitespace). I'll be fixing this shortly.

    6. This patch and previous versions do not remove whitespace around the '+' adjacent sibling combinator or the '>' child combinator. It is perfectly safe and valid to eliminate whitespace around these two punctuation characters.
    7. Another optimization that can be done is to remove the very last semi-colon before the closing curly bracket that closes each rule. This was suggested in the further optimize CSS file (need help with reg exp). This is a good idea and quite easy to do.
    8. The regex that removes space around punctuation uses alternation to differentially handle whitespace around parentheses. I think it would be faster to separate this into two regexes. this would also allow proper handling of the ':' colon, where leading whitespace should not be stripped.
    9. Not a comment, but rather a question: The aggregation code is deliberately preserving whitespace on the out-sides of parentheses '\s(...)\s'. Does anyone know why this is necessary? I did a search but could not find any reason for this.

    Otherwise the patch looks pretty good as far as I can tell. (I'm still trying to figure out how to get the SimpleTest feature to work and successfully test the patch I put together. I am somewhat of an expert when it come to regular expressions and pretty good with PHP, but I am a newb when it comes to Drupal development!)

    Edit: I see that you have modified your patch and responded to some of my concerns. This post was written as a reply to your post 101. Cheers! :)

    mikeytown2’s picture

    @ridgerunner
    Could you give you analysis on the latest patch (#112) since you stated your last reply (#113) was directed at comment #101.

    ridgerunner’s picture

    Yes, I'm in the process of looking at it right now. I was trying to figure out why the 101 version was erroneously deleting the comment embedded within the single quoted string in the example code I provided in item #5 of post 113 above. In theory, comments within strings should not be modified - but they are.

    I found the problem: The 's' "dot-matches-all" modifier needs to be added to the main callback regex. Otherwise the string matching regexs fail to match strings that cross line boundaries. This applies to both patches 101 and 112.

    So far, all the comments I made about the 101 patch appear to apply to patch 112 as well.

    Tri’s picture

    Wow ridgerunner, we were typing simultaneously! Lots of karmic stuff is going on in this thread lately! :)

    Thanks for the review.

    1. I think it doesn't make sense to use a v.1 hack inside a v.2 hack, since if you are already in a v.2 hack you don't need a v.1 hack to hide from IE-Mac. Out of curiosity though, I will test this on an old Quicksilver I have and I will get back.

    About optimizations, D7 is in a "code freeze" state, which means only bug fixes can go in.

    About bugs related with whitespace cleanup it is better to address them over there, or start a new thread specifically on this. "One at a time"...

    For the Simpletest thing, I suggest to get a copy of head running locally and enabling the "Testing" module that ships with it. It will help you test a patch before submitting it and by watching the internals you can get into it.

    Tri’s picture

    Here is another roll of the patch with the "s" switch in. I never thought that someone might want to include a code comment in his html through the content css property, but ridgerunner, you are right it has to work even then.

    ridgerunner’s picture

    After my patch failed the automated test last night, I did start digging in to SimpleTest and got the HEAD version up and running (the latest version as of this morning). I enabled the testing module, selected *all* of the tests, then started it running. After a couple hours or so it was still maybe only half way through with lots of reported exceptions. Is this normal? (Note that I did NOT apply any patches - just the straight HEAD checkout). I'm running WinXP SP2 with Apache-2.2.11 (Win32) / PHP-5.2.9-2 / MySQL-5.0.51a local web server on a single core AMD64 2.5GHz with 2GB RAM so its no slouch. But it does seem that these tests should run much faster than this yes? no? Should all the tests pass normally or is the HEAD bleeding edge with lots of warnings/errors? Which specific tests apply to the common.inc.drupal_load_stylesheet_content() function?

    Anyhoo... yes, I definitely want to play by the rules regarding proper procedure for contributing to Drupal. I can understand that it is a team effort, and that "one step at a time" is the standard protocol. I certainly don't want to be stepping on anyone's toes! (I apologize if that's how my tone has come across.) Its just that the changes I had in mind (i.e. simultaneously address all the issues I outlined above - and particularly the goal of condensing the text inside the mac-hack comments (now that one was a real mind-bender!)), required one bigger hammer to solve correctly. And the solution to all these issues involves the same bit of code. Thus, the resulting solution I present here is pretty darn complex (and may veer away from the status-quo), but it does do a better job regarding compression of the CSS (about 20% smaller with my current 60KB test file). Although this extra functionality does come at a cost - my version runs about 3 times slower than yours (but at 1200 microsec, (vs your 400), its still pretty darn fast!)

    Update: Before starting this post I updated my Drupal CVS checkout to the latest and greatest and restarted the entire test suite. After approximately one and a half hours, here is its progress status:

    Processed test 112 of 316 - Unmanaged file copying. 37% done.
    Overall results: 6463 passes, 20 fails, 169 exceptions, and 1288 debug messages

    Does this sound about right? Or is there something terribly wrong with my setup?

    andypost’s picture

    @ridgerunner something wrong with your environment, simpletest highly dependable on environment! I recommend you to use LAMP stack inside some virtual machine (virtualbox, vmware player)

    ridgerunner’s picture

    FileSize
    950 bytes

    Tri,

    Looking at your code I see a way to easily improve the data compression (for css files that have macie5 hack comments), by changing just two lines of code in _process_comment(). Instead of returning the whole matched type 2 comment (in all its $matches[0] bloated glory), simply return '/* \*/' instead. And for the type 1 comment matches, return '/*\ */'. But don't try messing with the closing comment for type 2 hacks - you will break things - (solving that one was very tricky and forced me to go with the complex solution I came up with - you can't simply return '/* */', its not that simple.) See attached file with the modified function.

    Easy! =)

    Tri’s picture

    Ridgerunner, if you try to run all tests it will take very long on a normal devbox. The automated test system needs 15-20min to run them all and these machines are dedicated on this purpose and are way above a normal box in terms of hardware. The tests you want are under System > CSS Unit Tests. To run just these is a matter of seconds.

    Considering the errors you get, it not normal, head code always passes all tests, so probably there is something wrong with your setup. I can't help you further cause I am on Linux.

    I really appreciate your efforts and feedback and I am glad that you are on the thread, there is no need to apologize. I write regexps myself and I can understand well the amount of brain circles put in your patch :) . Just keep in mind that Mac-IE hacks are actually a very small portion of an installation's css and well respected Drupal people in this thread have argued that support for those has to be dumped completely. So don't put too much thought in this. Besides that this whole thread revolves around the proper comment stripping part of the css aggregation procedure rather than the whitespace condensing one...

    Edit: The concurrent typing is becoming the norm here I guess! Lol.

    ridgerunner’s picture

    Thanks for the feedback.
    Cheers!

    Shai’s picture

    Great work folks... I don't mean to interrupt the great work you are doing getting a patch in D7 to solve this problem.

    Can someone recommend what someone with this problem using D6 could do to implement a fix right now? Should I try to apply the patch in #117.

    Should I just try to look for the offending long comment?

    Any suggestions. Please let me know if there is a better place to post this request.

    Thanks again for the great work.

    Shai

    Tri’s picture

    @Shai
    Patch #117 common.inc-117-d6.patch is against 6.x-dev so it will fix it for you. Apply it and report back if it fixed it indeed. If it isn't a big deal to you, temporarily add to your css some IE-Mac hacks and some quoted comment delimiters - even if you don't need them, so we can properly test this.

    Shai’s picture

    @Tri,

    I can't believe I didn't see that Drupal 6 patch there. It was right in front of my eyes! Thanks.

    I applied it, it applied cleanly. But it didn't solve my problem; most styling doesn't get applied, even though the file is being written and I can see some rules (via Firebug) that are being applied. But most are not. Shrug, so I'm pursuing other possible causes for the problem.

    Thanks much for responding.

    Shai

    Garrett Albright’s picture

    Success with the D6 patch for #117 for me. Mad props, Tri! Let's get this committed all across the board.

    sun’s picture

    Status: Needs review » Needs work
    +++ includes/common.inc	21 Feb 2010 02:03:11 -0000
    @@ -2951,26 +2951,61 @@ function drupal_load_stylesheet($file, $
    +    $comment     = '/\*[^*]*\*+(?:[^/*][^*]*\*+)*/'; // Regexp to match comment blocks.
    +    $double_quot = '"[^"\\\\]*(?:\\\\.[^"\\\\]*)*"'; // Regexp to match double quoted strings.
    +    $single_quot = "'[^'\\\\]*(?:\\\\.[^'\\\\]*)*'"; // Regexp to match single quoted strings.
    

    We can move (only) these comments above the remarked code lines.

    +++ includes/common.inc	21 Feb 2010 02:03:11 -0000
    @@ -2951,26 +2951,61 @@ function drupal_load_stylesheet($file, $
    + * Implements comment strip logic.
    

    The function summary should summarize the function's purpose. "Implements" generally refers to hook implementations.

    +++ includes/common.inc	21 Feb 2010 02:03:11 -0000
    @@ -2951,26 +2951,61 @@ function drupal_load_stylesheet($file, $
    + * drupal_load_stylesheet_content(). Support for comment hacks is implemented here.
    

    Exceeds 80 chars.

    +++ includes/common.inc	21 Feb 2010 02:03:11 -0000
    @@ -2951,26 +2951,61 @@ function drupal_load_stylesheet($file, $
    +  static $keep_nextone = FALSE;
    

    This probably needs to be a drupal_static() for 100% testing framework compliance.

    +++ includes/common.inc	21 Feb 2010 02:03:11 -0000
    @@ -2951,26 +2951,61 @@ function drupal_load_stylesheet($file, $
    +    switch (strrpos($matches[0], '\\')) {
    +      // No backslash, strip it.
    +      case FALSE :
    +        return '';
    +        break;
    +      // Ends with \*/ so is a multi line IE-Mac hack, keep this and the next one.
    +      case drupal_strlen($matches[0])-3 :
    +        $keep_nextone = TRUE;
    +        // Fall through to default case.
    +      // Single line IE-Mac hack, keep this.
    +      default :
    +        return $matches[0];
    +    }
    

    There should be blank lines between cases, except when falling through.

    +++ includes/common.inc	21 Feb 2010 02:03:11 -0000
    @@ -2951,26 +2951,61 @@ function drupal_load_stylesheet($file, $
    +      // No backslash, strip it.
    ...
    +      // Ends with \*/ so is a multi line IE-Mac hack, keep this and the next one.
    ...
    +      // Single line IE-Mac hack, keep this.
    

    Please move the inline comments into the cases.

    +++ includes/common.inc	21 Feb 2010 02:03:11 -0000
    @@ -2951,26 +2951,61 @@ function drupal_load_stylesheet($file, $
    +        return '';
    +        break;
    

    Superfluous break.

    +++ modules/simpletest/files/css_test_files/comment_hacks.css	21 Feb 2010 02:03:12 -0000
    @@ -0,0 +1,83 @@
    +/*
    +* A sample css file, designed to test the effectiveness and stability ¶
    +* of function drupal_load_stylesheet_content( ).
    +*
    +*/
    

    Wrong indentation, weird space in function name, superfluous blank comment line.

    +++ modules/simpletest/files/css_test_files/comment_hacks.css	21 Feb 2010 02:03:12 -0000
    @@ -0,0 +1,83 @@
    +* A sample css file, designed to test the effectiveness and stability ¶
    ...
    +/* ¶
    
    +++ modules/simpletest/files/css_test_files/comment_hacks.css.optimized.css	21 Feb 2010 02:03:12 -0000
    @@ -0,0 +1,8 @@
    \ No newline at end of file
    

    There's plenty of trailing white-space and missing newlines in this patch. At least that needs to be fixed.

    +++ modules/simpletest/files/css_test_files/comment_hacks.css	21 Feb 2010 02:03:12 -0000
    @@ -0,0 +1,83 @@
    +.this_rule_must_stay {
    +    color: #F00;
    +    background-color: #FFF;
    

    (and elsewhere) I'm glad that these are not really tabs, but they look like tabs - can we decrease to usual indentation of 2 spaces everywhere, please?

    142 critical left. Go review some!

    ridgerunner’s picture

    Tri,

    Before you roll up your next patch, please take another look at my post 120 (I think you might have missed it). This trivial 2-line change improves both execution speed and CSS file compression.

    Note: all the critiques I made in post 113 still apply.

    fietserwin’s picture

    To further complicate matters, this (sought) example also doesn't work with the "optimizer" (I did't test with the current patch, but the part to optimize white space looks unchanged to me):

    pre:after
    {
    content: "{ padding-right : 10px; }";
    }

    this will be "optimized" to

    pre:after{content:"{padding-right:10px;}";}

    where it should keep its hands off the string contents.

    I think that optimizing css code using regular expressions is impossible. Face it:
    - a regular expression is not a true parser and never will be
    - regexps have performance and memory problems on long strings popping up irregularly, depending on OS, etc.

    So a real solution will have to parse the files, or be content with conservative suboptimal solutions. This won't be a performance problem as it is rarely executed and parsers are very fast as well. Problem will be to write the code or find a library that already does this.

    Damien Tournoud’s picture

    I see a lot of energy in this to properly support (two types of) an very outdated hack. What I said a few times already still holds: let's remove support of Mac IE 5.5 hacks.

    RedTop’s picture

    Patch in #117 worked beautifully on Apache 2.2, PHP 5.2.11 WAMP server running Drupal 6.16. Thank you very much!

    (I am a bit worried about the number (well, two... but every patch is one to many) of patches I have in common.inc though. I hope this gets committed soon!)

    charlesuchu’s picture

    Issue tags: +Quick fix

    Clipped original message after reading more of this thread and seeing more clearly that it was not a php pcre bug, but the regex itself that was problematic.

    marcvangend’s picture

    Issue tags: -Quick fix

    With RedTop: hope this gets committed soon.

    I wish I could contribute code to this issue, but I suck at regex. I really appreciate all the people who have donated their regex skills and time to fix this bug improve the code. Still, it feels as if this issue keeps getting kidnapped by clever yet far-fetched code snippets that could break the regex. I'd say: Let's include the remarks in #127 and get this over with.

    PS. The tag "Quick Fix" no longer applies, does it? ;-)

    jaochoo’s picture

    I am obviously having the same problem: JS aggregation works just fine, but CSS aggregation will produce a white screen without any output. RootCandy admin theme still works fine, but my actual site (using a Zen subtheme) does not work anymore. The quickfix in this thread: http://drupal.org/node/331915#comment-1357688 (turning off the comment removal from CSS) works, but actually I would like to have all comments removed (in particular using Zen with all its comments).

    Is the patch in this thread here recommend and a better approach than the one in http://drupal.org/node/331915#comment-1357688? How can I apply the patch? I never did that, sorry..

    RedTop’s picture

    If you're using * nix you can use the patch file for automated patching. There's some documentation here: http://drupal.org/node/60108

    I use Windows (Notepad++) and haven't found a way to do this automated (If anyone knows, let me know please!). However, doing it manually is quite easy.

    - open the patch file in a texteditor or your browser
    - The first line tells you which file to patch:

    RCS file: /cvs/drupal/drupal/includes/common.inc,v
    

    ... which in this case is common.inc.
    - Next look for strings like this one:

    @@ -2007,10 +2007,16 @@
    

    ... they tell you which around which line number you have to start patching at (in this case 2007).

    - usually what follows are a couple of lines of code to help you recognize the exact location of where to patch. These lines do not start with a '+' or '-'.
    - The lines which do start with a '-' have to be deleted.
    - The lines which do start with a '+' have to be added.

    So for example:

    -      $contents = preg_replace('<
    -        \s*([@{}:;,]|\)\s|\s\()\s* |  # Remove whitespace around separators, but keep space around parentheses.
    -        /\*([^*\\\\]|\*(?!/))+\*/     # Remove comments that are not CSS hacks.
    -        >x', '\1', $contents);
    +      $comment     = '/\*[^*]*\*+(?:[^/*][^*]*\*+)*/'; // Regexp to match comment blocks.
    +      $double_quot = '"[^"\\\\]*(?:\\\\.[^"\\\\]*)*"'; // Regexp to match double quoted strings.
    +      $single_quot = "'[^'\\\\]*(?:\\\\.[^'\\\\]*)*'"; // Regexp to match single quoted strings.
    +      $contents = preg_replace_callback(
    

    The first four lines will be deleted, after which the last four lines will be added. Basically it says to substitute the first four lines with the last four lines (as you'll notice the code is similar but updated).

    Hope this is clear enough. :)

    marcvangend’s picture

    Apply patches on Windows: http://drupal.org/node/60179
    But lets keep this issue on topic please.

    jaochoo’s picture

    Thanks both for the help! Sorry to ask another question about patching, but in #117 there are 3 patch files: common.inc-117.patch, common.inc-117-d6.patch, common.inc-117-d5.patch. I obviously don't need the last one (Drupal 5?), but which one of the first two files do I need?

    ridgerunner’s picture

    Which version of Drupal are you using? 6.15, 6.16?

    jaochoo’s picture

    I am using Drupal 6.16

    kentr’s picture

    @jaochoo:

    Use the patch with "d6" in the title. The others are for Drupal 7 and Drupal 5.

    You can tell that the one without "d<anything>" in the title is Drupal 7 because of the simpletest (simpletest only works on D7 patches currently), and the issue is tagged as a Drupal 7 issue (so, by default, patches in this issue are for D7).

    catch’s picture

    Agreed with Damien in #130 - we should explicitly remove support for Mac IE 5.5, and associated hacks. However it looks like some are in jQuery UI, so that might not be too straightforward.

    donquixote’s picture

    I'm happy to remove the mac hacks for D7, but will that be ok for D6 as well?

    marcvangend’s picture

    @#142: I don't think so, we don't want to break existing sites that already use the hacks.

    donquixote’s picture

    So we will need to find a solution that supports the hacks anyway, to fix the bug for D6. Removing the hack for D7 can speed up the D7 fix, but won't save us any work.

    mikeytown2’s picture

    Issue tags: -Quick fix

    What if we get this out the door and open up a separate issue for the IE 5.5 Mac Hacks?

    marcvangend’s picture

    Mikeytown2, I completely agree. In fact, #61 was already RTBC. Dries committed it to HEAD, so the fix is already in D7 (see http://drupal.org/cvs?commit=288374). That was almost 5 months ago. As far as I'm concerned, we can commit the D6 patch in #61 now and start a new topic about optimizing regexes and IE5.5 hacks.

    By the way, the statistics you're linking to are extracted from W3School's server logs. The audience of that site is not a representative selection of all web users.

    Tri’s picture

    Well, actually there is no need to open a separate issue for IE-Mac hacks, because there is nothing left to be discussed. The last patch #117 works fine as far as css hacks are concerned. Some problems mentioned at #129 and #113 have to do with the white space removal part of drupal_load_stylesheet_content, not with the css hacks one.

    There was a mini community pole at #65 - #73 where we decided that we want to keep css hacks support, this is why we kept working on this, and now we already have the code that does the job. If for any reason we don't want it any more, we have to roll another patch that will remove the hacks but will address bug #99 that doesn't have to do with hacks.

    So, IMHO, if we want to end this long thread we can commit #117 with sun's nitpicks (#127) and maybe ridgerunner's trivial 2-line change (#128) and start a new issue about css whitespace removal, where we can fix #129, #113 and several other related issues found in the forums.

    I will roll a new patch as soon as I familiarize myself with drupal_static().

    JStanton617’s picture

    Given that this is fixed in HEAD, can we change the version from 7.x-dev to 6.x-dev to remove this from the critical D7 bugs list?

    JohnAlbin’s picture

    Priority: Critical » Normal

    There was a mini community pole at #65 - #73 where we decided that we want to keep css hacks support

    No. Myself (#59), DamZ (#36), marcvangend (#66) all voted to remove support for any IE5/mac hack.

    (And, yes, I see Marc changed his mind later, but I don't understand his comment where he flipped his vote and said "Not because I suddenly care about MacIE5, but because it's better for usability/UX to avoid warnings whenever possible". Sorry, what warnings? We don't present the user with any warnings because of possible IE5/mac hacks in the CSS. I'm honestly confused how this improves usability.)

    Fortunately, there is an even older issue to remove IE5/mac hack. So we can split that discussion back over to the original thread: #139117: Remove outdated IE5-mac hacks for build_css_cache regex. Those who are interested in that work should head over there; hopefully making it easier for this issue to proceed.

    As for the remaining items in this issue: missing tests, a broken form of of the IE5/mac hack… I don't see any "critical" bugs in that, so I'm setting this back to "normal". Let me know if I missed something that is actually critical. I do recognize that its critical when it goes back to D6, but… :-\

    Lastly, yes, performance does matter. No, its not something that only happens for admins. CSS aggregation is rebuilt ANY TIME that a module or theme changes what CSS files are included on a given page. If a Views dynamically adds a CSS file based on a view that is on a page, the ENTIRE aggregation is re-done. If a block adds a CSS file just for its own display and the block has visibility rules, the ENTIRE aggregation is re-done. Obviously, that's a flaw in our current aggregation approach. But its one that we will have to tackle in D8. I only point it out to illustrate that performance cannot be ignored in this issue. A simpler/faster regex is important.

    [Edit: Was the bug in #99 newly introduced by the committed patch above? If it pre-existed, then its a separate issue.]

    marcvangend’s picture

    @JohnAlbin: "avoid warnings" was referring to Tri's options 1) and 2) from comment #65:

    1) Drop the IE-Mac hack all together.
    This will be the fastest of all. Scores high on efficiency but alters the functionality of the original css. If we follow this path, we have to place a warning at the "Performance page" bellow the "Enable CSS optimization" about hacks not supported.

    2) Support hack v.1 only as the original did.
    This is the patch from #22 (needs re-roll for current HEAD). This is something like 20-60 times faster than core (#26). Still alters the functionality of the original css and will need a warning about multi line hacks not supported.

    wgrunberg’s picture

    I apologize for the cross post but I would like to bring the following to your attention.

    I found out that, the CSS optimization crash bug on our Drupal 6.16 site is specific to what LAMP/WAMP setup we are using.

    It does not crash on the following Linux setup provided by site5.com:
    Apache/2.2.11 (Unix) mod_ssl/2.2.11 OpenSSL/0.9.7a Phusion_Passenger/2.2.11 mod_auth_passthrough/2.1 mod_bwlimited/1.4 FrontPage/5.0.2.2635

    It does crash on the following Windows 2008 Server plus XAMPP setup:
    Apache/2.2.11 (Win32) DAV/2 mod_ssl/2.2.11 OpenSSL/0.9.8i PHP/5.2.9

    (Cross-post from #331915: Performance: When enable optimization for css and javascript ... site goes down instant!)

    st0nerhat’s picture

    #117 w/ #120 works for me! Thanks!

    davidwhthomas’s picture

    Status: Needs work » Reviewed & tested by the community

    I had the exact same problem CSS aggregation caused a "page reset" connection error in the browser with a zen sub theme.

    The patch in #117 solved the issue for me too, thanks!

    DT

    pendashteh’s picture

    why Drupal guys do not apply the patch #117 in next Drupal 6 release?

    okday’s picture

    subscribe

    catch’s picture

    Status: Reviewed & tested by the community » Needs work

    See #127, and #149, this is not RTBC.

    NoDice’s picture

    I would also like to confirm that I was having the same issue too with Drupal 6.16 running on XAMPP 1.7.1. My theme is based off Zen but not technically a Zen sub-theme. Immediately after enabling "CSS Optimization", my site crashed and was completely inaccessible with consistent Apache crashing.

    After applying the patch in #117, everything works fine.

    NOTE: Like #151, I can also confirm that this is an issue with Windows XAMPP, WAMP environments. I have the same Drupal 1.16 site and configuration running just fine on a Linux box without applying the patch.

    NoDice’s picture

    I would also like to confirm that I was having the same issue too with Drupal 6.16 running on XAMPP 1.7.1. My theme is based off Zen but not technically a Zen sub-theme. Immediately after enabling "CSS Optimization", my site crashed and was completely inaccessible with consistent Apache crashing.

    After applying the patch in #117, everything works fine.

    NOTE: Like #151, I can also confirm that this is an issue with Windows XAMPP, WAMP environments. I have the same Drupal 1.16 site and configuration running just fine on a Linux box without applying the patch.

    Tri’s picture

    Status: Needs review » Needs work
    FileSize
    2.8 KB
    2.88 KB
    129.68 KB
    131.46 KB

    Here is an other roll of the patch, which incorporates Sun's (#127) suggestions (nitpicks and drupal_static() use) along with ridgerunner's(#128) improvement (I just replaced spaces with underscores in the replaced comments). I added a newline at the end of the output because the whitespace removal part is stripping it. I am attaching versions for D6 & D5 also.

    No hacks version
    Based on #71, I thought that the mini-poll's outcome was to keep support for macIE css hacks. Anyway I am attaching a new patch also, that removes comment hacks completely, while it doesn't break on single/double quoted strings.It's the common.inc-nh one.

    Speed comparison
    To assemble a typical css sample I took the css files from a vanilla D6 install with admin_menu, date, devel, imagecache, imagefield, views, wysiwyg and zen along the comment_hacks.css from the patch and I concatenated them in one file as many times as needed to get the desired file size. Times are the average of 100 repetitions.

    Css file size common.inc.patch common.inc-nh.patch
    100Kib 0.009sec 0.001sec
    1Mib 0.099sec 0.011sec
    24Mib 2.107sec 0.233sec

    Thoughts
    I hate to play the "dead browser's advocate", I feel about it the same way you do. The reason I am arguing for is conceptual. Think network neutrality: As all internet traffic should be treated equally by the network, so all css rules must be treated equally by the aggregation function, even the outdated ones. Discussion about hacks support at the aggregation level would be reasonable if none was using such hacks, but unfortunately they are still around... Anyway this is just my opinion. If we decide that we don't want it even if it's ready, I am all with you. Considering performance , even if the "with hacks" version is 9 times slower, is still very fast if you take into account the usual css file sizes. So I don't think that speed is an issue, even if it gets called more often than administrative tasks.

    @JohnAlbin
    I think bug in #99 preexisted the committed patch, but since it has to do with the exact same -comment stripping- lines of code and since we got thus far with it, I left it in. If it causes trouble in committing/testing let know to start a new thread on that.

    The rest of the issues mentioned, have to do with the whitespace removal part which for sure needs some refactoring but this definitely has to go in an other thread.

    Tri’s picture

    Status: Needs work » Needs review
    ridgerunner’s picture

    Status: Needs work » Needs review

    I reviewed and tested the d6 and d7 patches presented in post 159 and have the following comments.

    • They effectively solve the seg-fault issue.
    • They are quite fast.

    They are not perfect but are much better than the code currently in 7-dev and 6.16. IMHO these patches should be committed to both branches ASAP. Regarding which version to choose, I would vote to keep the hack support for version 6 but drop it for version 7. I would change the status to RTBC but I don't (yet) have enough street cred around here to do so.

    Notes to Tri:
    I see that you have added the "S" study modifier to the @import regex. However, this modifier only speeds up certain types of regexes and will not help this particular one. The "S" study modifier performs its (one and only) optimization on regexes which begin with a limited number of possible starting characters (such as a group of alternatives - which is what we have in the main regex). It can speed up a regex by limiting the number of starting positions within the target string from which to start its matching attempts. It will *not* speed up a regex which has known, fixed exposed literal text at its beginning such as this one which begins with '@import'. (Note that this, and other PHP specific regex optimization topics are covered in detail in the *3rd* edition of MRE - highly recommended).

    Also, did you know that you can access characters in a string directly? A string can be treated as an array of characters and there is no need to resort to the substr() function. See: PHP Manual: String access and modification by character. So this...

      $start_char = substr($matches[0], 0, 1);
      if ($start_char == "'" || $start_char == '"') {
        return $matches[0];
      }

    can be written like so...

      if ($matches[0][0] == "'" || $matches[0][0] == '"') {
        return $matches[0];
      }

    (Note that you do need to be careful when dealing with strings containing unicode characters but in this case it doesn't matter.) According to my measurements, this one change results in better than a 10% speed gain for load_drupal_stylesheet_content().

    I've spent quite a bit of time lately looking into this whole CSS aggregation/minification issue (there are some serious issues with how Drupal (mis-)handles the @import directive) and am writing a detailed series of articles on the subject. Coming soon...

    Bottom line: These patches are good and should be committed.

    Tri’s picture

    @ridgerunner
    I noticed that the study modifier didn't add any gains at the @import regexp during my speed tests and I removed it from the "no-hacks" version, but forgot to do so in the "with hacks version"... Though I will manage to get a copy of Jeffrey Friedl's 3rd edition masterpiece.

    Also, nice tip about the square brackets access of chars in a string. More elegant and succinct. I measured a 7% gain here. As a matter of code brevity, I also removed the else from _process_comment() since it was superfluous.

    I am rerolling with these changes in place. The "no-hacks" version don't need reroll, it's good as is in #159.

    andypost’s picture

    Fixed in D6 #133188: Line break converter can result in empty node display - PCRE limits
    So maybe some code changes are required

    mikeytown2’s picture

    andypost’s picture

    Issue tags: +Performance

    @mikeytown2 Related but different.
    Also #162 is a greate performance improvement on a large texts!

    andrew.lansdowne’s picture

    subscribe

    snorkers’s picture

    Please, please, please can these patches be committed to core.

    Every time a new release is rolled out (D6 at least), I have to re-roll against these patches to get my main site to work - and I've been doing this for over a year now. The work you have all done on this issue is fantastic and way above my current skill set - but I really don't understand why we are still finessing what seems to be a working solution.

    For info, #162 works against 6.17, although there was a small offset:

    Hunk #1 succeeded at 2020 (offset 3 lines).
    Hunk #2 succeeded at 2046 (offset 3 lines).
    
    guygg’s picture

    Yeah, I was just looking at putting 6.17 on the 3 sites I admin, but I think I'll just skip 6.17 as I'm getting a little tired of re-rolling this issue over and over again with every core release. Is there something at this point that's holding up getting this rolled into the releases? I'm amazed more people don't suffer from the problem in this issue. It's a problem with all 3 of the sites I deal with, and they run on different OS/platforms from each other.

    As snorkers says, I really appreciate all the work that went into squishing this bug, but I'm also wondering what's holding up what for me and seemingly everyone else is a working solution. Guess I'll hold out hope that this makes it into 6.18. Fingers crossed.

    Thanks to all those who have put in the grunt work on this fix...

    wgrunberg’s picture

    This bug persists in Drupal 6.17 in a XAMPP on Windows 2008 Server environment. I can't reproduce the crash on LAMP setups or on Windows XP or Windows 7. Disabling the removal of CSS comments in the CSS optimization function alleviate the symptoms: http://drupal.org/node/331915#comment-1357688

    casey’s picture

    Priority: Normal » Critical

    Reroll of #162 + replaced "_process_comment" with "_process_stylesheet_comment".

    If this is crashing servers, I'd say this is critical or at least major.

    casey’s picture

    FileSize
    130.55 KB

    Forgot patch

    casey’s picture

    But then again I rather use patch of #159 combined with #139117: Remove outdated IE5-mac hacks for build_css_cache regex. We don't support IE-mac.

    jonne_jvl’s picture

    Am I the only one to think that the proper "fix" is to comment out this regex?
    IMHO, the "optimize" part of "optimize css" is to aggregate the many files into one, significantly cutting out
    http requests AND preventing "too many css files" errors in certain browsers.

    Pre-compressing the resulting file, and in a severely buggy way at that, seems totally useless.
    This job is better sent to apache, varnish, minifycss or just about any way "but" a buggy regex.

    Why is it still there? Cant it at least be dropped from d6? What performance impact or functionality loss could that possibly have?

    Sorry if I sound annoyed, I really appreciate everyones efforts so solve it, but seriously? 1 year?

    ellis-’s picture

    subscribe

    jaochoo’s picture

    Pre-compressing the resulting file, and in a severely buggy way at that, seems totally useless.
    This job is better sent to apache, varnish, minifycss or just about any way "but" a buggy regex.

    People using a Zen sub-theme (which heavily uses comments for documentation purpose) probably would appreciate if there could be any comment-stripping. I've tried the CSSTidy module to minify the CSS the other way round but had to report several bugs (CSSGzip not working anymore, CSS broken due to wrong definitions).

    jonne_jvl’s picture


    I've tried the CSSTidy module to minify the CSS the other way round but had to report several bugs (CSSGzip not working anymore, CSS broken due to wrong definitions).

    So, kind of like drupal6 core then, without the occasional segfaults?

    Just for reference, the zen css files together with the drupal6 reference is 76k
    after stripping 39k
    after mod_deflate 17k
    after both 8k

    I dont see how this can compare with the severity of segfaulting certain configs

    Wouldnt

    -    if ($_optimize) {
    +   if ($_optimize && !variable_get('css_disable_regexcompression', false)) {
    

    Move this to a non-critical status? Then this issue is free to live long and well or split into smaller issues.

    scoobie’s picture

    When I upgraded to 6.17, this issue surfaced it's ugly head again. So I applied the common.inc-159-nh0159.patch.
    Worked like a charm!

    Thanks!

    I used the -nh hack simply because there is no reason to support a dead browser anymore.

    Damien Tournoud’s picture

    This is a reroll of the non-hack version of #159.

    Looks good to get in for me.

    casey’s picture

    Status: Needs review » Reviewed & tested by the community

    With special thanks to Tri and ridgerunner for their great efforts.

    webchick’s picture

    Version: 7.x-dev » 6.x-dev
    Status: Reviewed & tested by the community » Needs review

    Thanks a lot for the tests, and for the great and detailed analysis in this issue. The benchmarks in #159 regarding performance of keeping the mac hack vs. dropping it are pretty compelling. I agree with those who've said that supporting a several years' old Mac hack is not something we need to do in D7 (but likely can't drop from D6). It's bad enough we still have to care for IE 6 in D7. *sniff!*

    I found a small typo (usefull) but just fixed that and Committed to HEAD.

    Moving down to 6.x. It looks like the latest 6.x patch is in #162, but it's unclear to me if that version has been reviewed yet (even though others have), so marking it for "needs review" rather than "reviewed & tested by the community".

    Tri’s picture

    Issue tags: -Performance, -CSS aggregation
    FileSize
    2.8 KB

    Reroll of the #162 patch against 6.18-dev. It practically the same patch that many people have tested in this thread, so this can be considered RTBC.
    I am attaching also a reroll against 5.23-dev in case someone needs it.

    Tri’s picture

    Issue tags: +CSS aggregation, +Perfomance
    FileSize
    2.72 KB

    Sorry, I didn't mean to change the tags... I am working from within a boat and my connection is kind of funny. D5 patch attached here.

    marcvangend’s picture

    Thanks everyone. Assuming that quality is directly proportional to the time spent, this must be one of the best bugfixes ever :-)

    andypost’s picture

    Status: Needs review » Needs work

    Both patches have wrong indention

    jonne_jvl’s picture

    FileSize
    2.71 KB

    #181 indention

    asb’s picture

    #265719: CSS aggregator produces invalid code and directory names for @import files Breaks IE, Internet explorer does not style page.

    Does the issue and associated patch over there (#265719) affect this (#444228) issue and patch?

    jaochoo’s picture

    I need to update my Drupal core soon, therefore I would like to know if someone could tell we when the next stable version will be released with this patch included? I might wait until then instead of doing an upgrade now and rolling the patch later.

    jonne_jvl’s picture

    #186
    It deals with a different stage of aggregation and other bugs. So no.

    #187
    We can only hope that all the wonderful work that has been done in this thread will go in *sometime*. The thread seems to go to sleep everytime it misses a release, and its missed a few. I was against keeping core patches so bad that we internally dropped fastcgi setups as "incompatible with drupal".

    mikeytown2’s picture

    Status: Needs work » Needs review
    mikeytown2’s picture

    Issue tags: -Perfomance +Performance

    fix tag

    andypost’s picture

    Status: Needs review » Reviewed & tested by the community

    #185 works fine

    Gábor Hojtsy’s picture

    Status: Reviewed & tested by the community » Fixed

    Thanks for the extensive testing, committed.

    Status: Fixed » Closed (fixed)

    Automatically closed -- issue fixed for 2 weeks with no activity.

    neurovation.kiwi’s picture

    Status: Closed (fixed) » Active

    hi folks,

    just came around that issue myself - never thought that css could crash the server so i searched quite a bit ;)

    problem here was invalid css, namly

    /* start of comment
    
      end of comment
    /*
    

    not sure if this is really an issue that should be covered here, since drupal is no magic unicorn riding on a rainbow than can fix invalid css, but just wanted to mention it here.

    feel free to shut me up and set the fixed tag! (i'm not taking it personally *g*)

    cu
    kiwi

    kentr’s picture

    @neurovation.kiwi:

    A processor shouldn't blindly fix incorrect syntax, because there might be a reason that the dev used invalid syntax. It's no different than with a browser - the browser can only act on correct CSS code, because it can't make assumptions about why the code is incorrect (like assuming that it was a mistake).

    It's the dev's responsibility to use correct syntax that this feature can recognize in order process the CSS code as intended.

    So, I'd say your example is outside the scope of this feature.

    kentr’s picture

    Status: Active » Closed (fixed)

    :-)

    MustangGB’s picture

    Priority: Critical » Major
    Benj’s picture

    bleen’s picture

    Priority: Major » Critical
    Status: Closed (fixed) » Active

    The patch in #185 does not include CSS from subthemes in the aggregated css file!

    I'm still trying to track down exactly what is going on, but my upgrade to d6.19 busted all my stylesheets. When I reversed this patch, everything was fine again. I suspect that it's about sub themes because those styles are all missing from my aggregated css file

    AHHHHHHH

    bleen’s picture

    I have a bit more data ... basically, whats happening is that the style.css from my base theme is being omitted from the aggregated css file. The subtheme's style.css is included correctly. Still lookin

    Heine’s picture

    Priority: Critical » Major
    Status: Active » Closed (fixed)
    Garrett Albright’s picture

    Ping Tri, regarding the patch in #112, in which the "u" modifier was added to the pattern which seems to be causing the problems mentioned in #199 and #881132: CSS Optimization breaks with non-UTF-8 .css files. Do you think you could hop over to that other issue and fill us on as to why you decided to add "u"? And, more specifically, if you can foresee any problems from us just removing it, which corrects the issue?

    rfay’s picture

    Just wanted to mention to all of you who worked so hard on this: If you're not watching the drama at #769226: Optimize JS/CSS aggregation for front-end performance and DX you probably should. It's important to anybody interested in aggregation.

    Benj’s picture

    Has this patch been applied to D6.9? It doesn't look like it to me.

    marcvangend’s picture

    horseatingweeds, this has been applied to D6.19 according to the changelog at http://drupal.org/node/880546.

    eileenmcnaughton’s picture

    Subscribing - I hit this bug on d 6.19 (for which it should have been fixed). The install is a dev install (on windows) so turning off css optimisation is not a problem & I can work around it that way but recording this in case someone else has the problem later.

    Experienced with zen subtheme based on 960. (nb I tried different apache & php versions with same result in 5.3.4 php/2.2.1.7 apache).

    The stuff I am pasting below this point is all to help someone else who hits this problem find this thread as I would have loved to have found it 5 hours ago....

    The keywords I searched on:
    wamp xampp The connection to the server was reset while the page was loading.

    windows application log
    Faulting application name: httpd.exe, version: 2.2.11.0, time stamp: 0x493f5d44
    Faulting module name: php5ts.dll, version: 5.2.11.11, time stamp: 0x4ab130e3
    Exception code: 0xc00000fd
    Fault offset: 0x0015167f
    Faulting process id: 0xccc
    Faulting application start time: 0x01cbb7b5568c8d06
    Faulting application path: C:\wamp\bin\apache\apache2.2.11\bin\httpd.exe
    Faulting module path: C:\wamp\bin\apache\apache2.2.11\bin\php5ts.dll
    Report Id: 9a70bcbd-23a8-11e0-8562-70f39525b971

    Apache error log

    [Wed Jan 19 22:17:46 2011] [notice] Server built: Dec 10 2008 00:10:06
    [Wed Jan 19 22:17:46 2011] [notice] Parent: Created child process 6308
    [Wed Jan 19 22:17:46 2011] [notice] Child 6308: Child process is running
    [Wed Jan 19 22:17:46 2011] [notice] Child 6308: Acquired the start mutex.
    [Wed Jan 19 22:17:46 2011] [notice] Child 6308: Starting 64 worker threads.
    [Wed Jan 19 22:17:46 2011] [notice] Child 6308: Starting thread to listen on port 80.
    [Wed Jan 19 22:20:33 2011] [notice] Parent: child process exited with status 3221225725 -- Restarting.
    [Wed Jan 19 22:20:33 2011] [notice] Apache/2.2.11 (Win32) PHP/5.2.11 configured -- resuming normal operations