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
Comment | File | Size | Author |
---|---|---|---|
#185 | common.inc-185-D6.patch | 2.71 KB | jonne_jvl |
#182 | common.inc-181-d5.patch | 2.72 KB | Tri |
#181 | common.inc-181-d6.patch | 2.8 KB | Tri |
#178 | 444228-kill-the-mac-ie-regexp.patch | 129.01 KB | Damien Tournoud |
#171 | common.inc-170.patch | 130.55 KB | casey |
Comments
Comment #1
asb CreditAttribution: asb commentedIn 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
Comment #2
raulgigea CreditAttribution: raulgigea commentedHi,
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:
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:
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.
Comment #3
raulgigea CreditAttribution: raulgigea commentedSeems 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:
And, I observerd, the segfault doesn't occur if I disable mhash.so extension of php from extensions.ini.
Comment #4
laurentchardin CreditAttribution: laurentchardin commentedI 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.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commented@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?
Comment #6
raulgigea CreditAttribution: raulgigea commentedHi, 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.
Comment #7
raulgigea CreditAttribution: raulgigea commentedOkay, 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 :)
Comment #8
raulgigea CreditAttribution: raulgigea commentedHi,
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
Comment #9
scoorch CreditAttribution: scoorch commentedRaul, great work. I had the 500 Error here too when enabling css compression. Your patch fixed the problem. Thanks so much!
Thomas
Comment #10
asb CreditAttribution: asb commentedIs there any chance to get this patch into core, or will we have to maintain this bloody patch for every Drupal release?
Thanks -asb
Comment #11
poshat CreditAttribution: poshat commentedHi!
I have same problem. I tried turn on optimization CSS and JS and site working ok.
Comment #12
asb CreditAttribution: asb commentedThis 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
Comment #13
donquixote CreditAttribution: donquixote commentedI 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.
Comment #14
mattyoung CreditAttribution: mattyoung commentedIs this related to this #543892: Win XAMPP 1.7.2 Regex PHP Bug - Optimize css & long css comments fails?
Comment #15
donquixote CreditAttribution: donquixote commentedCould be, who knows..
I will link it from there, hope it helps!
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe 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.Comment #17
Tri CreditAttribution: Tri commentedHi.
The solution from #16 is to add this to your settings.php
I have tried with values up to
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.
Comment #18
asb CreditAttribution: asb commented@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
Comment #19
Tri CreditAttribution: Tri commentedHi.
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.
Comment #20
donquixote CreditAttribution: donquixote commentedWonderful!
The modified regex solves the problem for me on Windows XP (for the test script I created). I did not do other tests yet..
Comment #21
stmh CreditAttribution: stmh commentedNice patch -- worked for me too. Thanks!
Comment #22
Tri CreditAttribution: Tri commentedHere 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.
Comment #23
Damien Tournoud CreditAttribution: Damien Tournoud commentedCould you provide a sample input that triggers the segfault?
Comment #24
Tri CreditAttribution: Tri commented@Damien
This code
produces this
http://www.webistas.gr/segfault.php
while this code
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.
Comment #25
snorkers CreditAttribution: snorkers commentedI 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.
Comment #26
Tri CreditAttribution: Tri commented@snorkers
I am sorry but
pcre.backtrack_limit
andpcre.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?
Comment #27
donquixote CreditAttribution: donquixote commented@Tri:
Could you explain what was wrong with the old regex, so we can all learn something? That would be great!
Comment #28
asb CreditAttribution: asb commented@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!
Comment #29
donquixote CreditAttribution: donquixote commentedI 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.
Comment #30
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat 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.
Comment #31
Tri CreditAttribution: Tri commented@donquixote
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
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.
It's not an invalid regexp, because an invalid regexp doesn't even get parsed.
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.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
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.
Comment #33
donquixote CreditAttribution: donquixote commented@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?
Comment #34
Tri CreditAttribution: Tri commentedReroll against head.
Comment #35
Tri CreditAttribution: Tri commentedComment #36
Damien Tournoud CreditAttribution: Damien Tournoud commentedSupporting 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.
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.
Comment #37
mikeytown2 CreditAttribution: mikeytown2 commentedInside
5.x: http://api.drupal.org/api/function/drupal_build_css_cache/5
6.x: http://api.drupal.org/api/function/drupal_load_stylesheet/6
7.x: http://api.drupal.org/api/function/drupal_load_stylesheet_content/7
Comment #38
mikeytown2 CreditAttribution: mikeytown2 commentedComment #39
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe need tests, and I still don't know what you think is wrong with my simple
/\*(.+?)(?<!/)\*/
.Comment #40
donquixote CreditAttribution: donquixote commentedAbsolutely. 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.
Comment #41
donquixote CreditAttribution: donquixote commentedThis 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.
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.
Comment #42
Tri CreditAttribution: Tri commentedThe 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.
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.
Comment #43
donquixote CreditAttribution: donquixote commentedCase 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.
Comment #44
Tri CreditAttribution: Tri commentedI 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.
Comment #45
Tri CreditAttribution: Tri commentedThe 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.
Comment #47
Tri CreditAttribution: Tri commentedThere 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.
Comment #48
Tri CreditAttribution: Tri commentedComment #49
JohnAlbinsubscribe
Comment #50
rfayWe'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.
Comment #51
kentr CreditAttribution: kentr commented@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!.
Comment #52
mikeytown2 CreditAttribution: mikeytown2 commentedfix for bug works for me as well
Comment #53
donquixote CreditAttribution: donquixote commented@rfay (#50):
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).
Comment #54
Tri CreditAttribution: Tri commented@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?
Comment #55
rfayI 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.
Comment #56
Tri CreditAttribution: Tri commented@rfay
You are right. I missed the D6 commit of your patch. So here is a re-roll against D6.14.
Comment #57
marcvangendSubscribe, I'm seeing this problem on my Windows machine too.
Comment #58
mikeytown2 CreditAttribution: mikeytown2 commented@marcvangend
Does the patch fix the issue for you? If so please say so.
Comment #59
JohnAlbinI 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: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:
Still needs simpletests.
Comment #60
marcvangend@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 :-)
Comment #61
Tri CreditAttribution: Tri commented@JohnAlbin
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).
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.
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.
Comment #62
mikeytown2 CreditAttribution: mikeytown2 commentedI would call #61 RTBC
Comment #63
JohnAlbinAnd 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.
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 *.Comment #64
kentr CreditAttribution: kentr commented@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.
Comment #65
Tri CreditAttribution: Tri commentedJohn, thanks for the kudos and for revising my regexp. Though,
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...
Comment #66
marcvangendI 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:
Comment #67
rfayI 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.
Comment #68
donquixote CreditAttribution: donquixote commentedDoes Safari work on older Macs too?
Comment #69
kentr CreditAttribution: kentr commentedDefinitely 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.
Comment #70
rfay@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.
Comment #71
marcvangend@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.
Comment #72
mikeytown2 CreditAttribution: mikeytown2 commentedAgree as well, do it right the first time. Allow this to not fail on CSS hacks.
Comment #73
donquixote CreditAttribution: donquixote commentedAdmin 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
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?
Comment #74
mikeytown2 CreditAttribution: mikeytown2 commentedEveryone is in agreement here that we leave in the macIE css hacks correct?
#61 is RTBC
Comment #75
crutch CreditAttribution: crutch commentedSubscribe (ref D6 issue: http://drupal.org/node/331915)
Comment #76
Dries CreditAttribution: Dries commentedThe code comments in #61 are missing a space after the pound character. Needs quick reroll.
Comment #77
Dries CreditAttribution: Dries commentedRerolled it myself and committed it to CVS HEAD. Thanks.
Comment #78
marcvangendThanks Dries.
If I understand correctly, 'CVS HEAD' means that is fixed in D7 - or will it be rolled into D6 as well?
Comment #79
mikeytown2 CreditAttribution: mikeytown2 commentedWe need it for 6.x now
#61 is RTBC
Comment #80
Tri CreditAttribution: Tri commentedI 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.
Comment #81
andypost+1 RTBC, Tested patch from #80 under
There's little difference within 6.14 and 6-dev caused by #472820: drupal_load_stylesheet_content() removes whitespace improperly, creating invalid CSS
Comment #82
ivanstegic CreditAttribution: ivanstegic commentedTested 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!
Comment #83
casey CreditAttribution: casey commented#642974: CSS aggregation removes multi-line ie-mac hacks when the hack rules contain a star
Comment #84
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis 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:
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.
Comment #85
casey CreditAttribution: casey commentedI am working on the CSS aggregator: #641472: Fix CSS Aggregator.
Latest patch still contains that bug, but i am working on it.
Comment #86
Tri CreditAttribution: Tri commentedThis 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
Comment #87
donquixote CreditAttribution: donquixote commented@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.
Comment #88
Tri CreditAttribution: Tri commented@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 secondpreg_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.
Comment #89
Gábor HojtsyLet'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.
Comment #90
Tri CreditAttribution: Tri commentedOk, 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.
Comment #91
flaviovs CreditAttribution: flaviovs commentedPatch #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.
Comment #92
ridgerunner CreditAttribution: ridgerunner commentedTri, 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...
Comment #95
redearthbluesky CreditAttribution: redearthbluesky commentedHi,
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,
Comment #96
guygg CreditAttribution: guygg commentedLike #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...
Comment #97
kentr CreditAttribution: kentr commented#90 manually-applied to 6.15 fixed the bug on my site, no apparent new problems.
Comment #98
dicreat CreditAttribution: dicreat commented#90 solve my problem after manually-applied on Drupal 6.15.
Patch with fix in attach.
Comment #99
ridgerunner CreditAttribution: ridgerunner commentedThere 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:
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.
Comment #100
marcvangend@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.
Comment #101
Tri CreditAttribution: Tri commentedHello 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 waydrupal_load_stylesheet
is working now, is striping someLF
s 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.Comment #102
Tri CreditAttribution: Tri commentedComment #103
marcvangend@Tri: sorry to hear about your accident, it's good to see you back here.
Comment #104
ridgerunner CreditAttribution: ridgerunner commentedYou 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!
Comment #105
ridgerunner CreditAttribution: ridgerunner commentedAttached 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:
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!
Comment #107
ridgerunner CreditAttribution: ridgerunner commentedIt appears that CVS patches must have unix \n line endings. Lets try this again...
Comment #108
marcvangendSetting back to critical, assuming ridgerunner changed the priority by accident.
Comment #109
marcvangendOops, guess we were writing comments simultaneously.
Comment #110
ridgerunner CreditAttribution: ridgerunner commentedYou are correct. I did. Sorry about that!
Comment #112
Tri CreditAttribution: Tri commentedHere 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:
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...
Comment #113
ridgerunner CreditAttribution: ridgerunner commentedHey 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:
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.
Before aggregation:
After aggregation:
Note that the patch I submitted is guilty of a similar offense (it strips leading whitespace). I'll be fixing this shortly.
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! :)
Comment #114
mikeytown2 CreditAttribution: mikeytown2 commented@ridgerunner
Could you give you analysis on the latest patch (#112) since you stated your last reply (#113) was directed at comment #101.
Comment #115
ridgerunner CreditAttribution: ridgerunner commentedYes, 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.
Comment #116
Tri CreditAttribution: Tri commentedWow 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.
Comment #117
Tri CreditAttribution: Tri commentedHere 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.
Comment #118
ridgerunner CreditAttribution: ridgerunner commentedAfter 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:
Does this sound about right? Or is there something terribly wrong with my setup?
Comment #119
andypost@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)
Comment #120
ridgerunner CreditAttribution: ridgerunner commentedTri,
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! =)
Comment #121
Tri CreditAttribution: Tri commentedRidgerunner, 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.
Comment #122
ridgerunner CreditAttribution: ridgerunner commentedThanks for the feedback.
Cheers!
Comment #123
Shai CreditAttribution: Shai commentedGreat 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
Comment #124
Tri CreditAttribution: Tri commented@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.
Comment #125
Shai CreditAttribution: Shai commented@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
Comment #126
Garrett Albright CreditAttribution: Garrett Albright commentedSuccess with the D6 patch for #117 for me. Mad props, Tri! Let's get this committed all across the board.
Comment #127
sunWe can move (only) these comments above the remarked code lines.
The function summary should summarize the function's purpose. "Implements" generally refers to hook implementations.
Exceeds 80 chars.
This probably needs to be a drupal_static() for 100% testing framework compliance.
There should be blank lines between cases, except when falling through.
Please move the inline comments into the cases.
Superfluous break.
Wrong indentation, weird space in function name, superfluous blank comment line.
There's plenty of trailing white-space and missing newlines in this patch. At least that needs to be fixed.
(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!
Comment #128
ridgerunner CreditAttribution: ridgerunner commentedTri,
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.
Comment #129
fietserwinTo 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.
Comment #130
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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.
Comment #131
RedTop CreditAttribution: RedTop commentedPatch 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!)
Comment #132
charlesuchu CreditAttribution: charlesuchu commentedClipped 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.
Comment #133
marcvangendWith 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? ;-)
Comment #134
jaochoo CreditAttribution: jaochoo commentedI 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..
Comment #135
RedTop CreditAttribution: RedTop commentedIf 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:
... which in this case is common.inc.
- Next look for strings like this one:
... 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:
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. :)
Comment #136
marcvangendApply patches on Windows: http://drupal.org/node/60179
But lets keep this issue on topic please.
Comment #137
jaochoo CreditAttribution: jaochoo commentedThanks 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?
Comment #138
ridgerunner CreditAttribution: ridgerunner commentedWhich version of Drupal are you using? 6.15, 6.16?
Comment #139
jaochoo CreditAttribution: jaochoo commentedI am using Drupal 6.16
Comment #140
kentr CreditAttribution: kentr commented@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).
Comment #141
catchAgreed 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.
Comment #142
donquixote CreditAttribution: donquixote commentedI'm happy to remove the mac hacks for D7, but will that be ok for D6 as well?
Comment #143
marcvangend@#142: I don't think so, we don't want to break existing sites that already use the hacks.
Comment #144
donquixote CreditAttribution: donquixote commentedSo 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.
Comment #145
mikeytown2 CreditAttribution: mikeytown2 commentedWhat if we get this out the door and open up a separate issue for the IE 5.5 Mac Hacks?
Comment #146
marcvangendMikeytown2, 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.
Comment #147
Tri CreditAttribution: Tri commentedWell, 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()
.Comment #148
JStanton617 CreditAttribution: JStanton617 commentedGiven 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?
Comment #149
JohnAlbinNo. 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.]
Comment #150
marcvangend@JohnAlbin: "avoid warnings" was referring to Tri's options 1) and 2) from comment #65:
Comment #151
wgrunberg CreditAttribution: wgrunberg commentedI 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!)
Comment #152
st0nerhat CreditAttribution: st0nerhat commented#117 w/ #120 works for me! Thanks!
Comment #153
davidwhthomas CreditAttribution: davidwhthomas commentedI 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
Comment #154
pendashteh CreditAttribution: pendashteh commentedwhy Drupal guys do not apply the patch #117 in next Drupal 6 release?
Comment #155
okday CreditAttribution: okday commentedsubscribe
Comment #156
catchSee #127, and #149, this is not RTBC.
Comment #157
NoDice CreditAttribution: NoDice commentedI 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.
Comment #158
NoDice CreditAttribution: NoDice commentedI 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.
Comment #159
Tri CreditAttribution: Tri commentedHere 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.
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.
Comment #160
Tri CreditAttribution: Tri commentedComment #161
ridgerunner CreditAttribution: ridgerunner commentedI reviewed and tested the d6 and d7 patches presented in post 159 and have the following comments.
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...
can be written like so...
(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.
Comment #162
Tri CreditAttribution: Tri commented@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.
Comment #163
andypostFixed in D6 #133188: Line break converter can result in empty node display - PCRE limits
So maybe some code changes are required
Comment #164
mikeytown2 CreditAttribution: mikeytown2 commentedIs this an issue with this patch applied?
#352042: drupal_build_css_cache() cannot deal with a url() statement not surrounded by spaces.
Comment #165
andypost@mikeytown2 Related but different.
Also #162 is a greate performance improvement on a large texts!
Comment #166
andrew.lansdowne CreditAttribution: andrew.lansdowne commentedsubscribe
Comment #167
snorkers CreditAttribution: snorkers commentedPlease, 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:
Comment #168
guygg CreditAttribution: guygg commentedYeah, 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...
Comment #169
wgrunberg CreditAttribution: wgrunberg commentedThis 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
Comment #170
casey CreditAttribution: casey commentedReroll of #162 + replaced "_process_comment" with "_process_stylesheet_comment".
If this is crashing servers, I'd say this is critical or at least major.
Comment #171
casey CreditAttribution: casey commentedForgot patch
Comment #172
casey CreditAttribution: casey commentedBut 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.
Comment #173
jonne_jvl CreditAttribution: jonne_jvl commentedAm 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?
Comment #174
ellis- CreditAttribution: ellis- commentedsubscribe
Comment #175
jaochoo CreditAttribution: jaochoo commentedPeople 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).
Comment #176
jonne_jvl CreditAttribution: jonne_jvl commentedI'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
Move this to a non-critical status? Then this issue is free to live long and well or split into smaller issues.
Comment #177
scoobie CreditAttribution: scoobie commentedWhen 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.
Comment #178
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is a reroll of the non-hack version of #159.
Looks good to get in for me.
Comment #179
casey CreditAttribution: casey commentedWith special thanks to Tri and ridgerunner for their great efforts.
Comment #180
webchickThanks 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".
Comment #181
Tri CreditAttribution: Tri commentedReroll 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.
Comment #182
Tri CreditAttribution: Tri commentedSorry, 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.
Comment #183
marcvangendThanks everyone. Assuming that quality is directly proportional to the time spent, this must be one of the best bugfixes ever :-)
Comment #184
andypostBoth patches have wrong indention
Comment #185
jonne_jvl CreditAttribution: jonne_jvl commented#181 indention
Comment #186
asb CreditAttribution: asb commented#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?
Comment #187
jaochoo CreditAttribution: jaochoo commentedI 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.
Comment #188
jonne_jvl CreditAttribution: jonne_jvl commented#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".
Comment #189
mikeytown2 CreditAttribution: mikeytown2 commentedComment #190
mikeytown2 CreditAttribution: mikeytown2 commentedfix tag
Comment #191
andypost#185 works fine
Comment #192
Gábor HojtsyThanks for the extensive testing, committed.
Comment #194
neurovation.kiwi CreditAttribution: neurovation.kiwi commentedhi 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
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
Comment #195
kentr CreditAttribution: kentr commented@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.
Comment #196
kentr CreditAttribution: kentr commented:-)
Comment #197
MustangGB CreditAttribution: MustangGB commentedComment #198
Benj CreditAttribution: Benj commentedComment #199
bleen CreditAttribution: bleen commentedThe 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
Comment #200
bleen CreditAttribution: bleen commentedI 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
Comment #201
Heine CreditAttribution: Heine commented@bleen18, see #881132: CSS Optimization breaks with non-UTF-8 .css files
Comment #202
Garrett Albright CreditAttribution: Garrett Albright commentedPing 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?
Comment #203
rfayJust 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.
Comment #204
Benj CreditAttribution: Benj commentedHas this patch been applied to D6.9? It doesn't look like it to me.
Comment #205
marcvangendhorseatingweeds, this has been applied to D6.19 according to the changelog at http://drupal.org/node/880546.
Comment #206
eileenmcnaughton CreditAttribution: eileenmcnaughton commentedSubscribing - 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