The way css is preprocessed at the moment, all @imports in a css file are loaded, optimised.
Note: This issue is actually by JoshuaRogers. It used to be:
#543892: Win XAMPP 1.7.2 Regex PHP Bug - Optimize css & long css comments fails
But that issue got changed to something else. This is separate.
Why is this a problem?
If a.css imports b.css which imports c.css, then c.css will be loaded and optimised. It will then replace the import statement in b.css. The resultant css (including the section that is already optimised) will be optimised, after which it will replace the import statement in a.css. At this point a.css (with the already optimised b.css and c.css) will be optimised before finally saving it. This means that many files may be processed multiple times with no benefit. In addition to this, it causes a large section of text to be sent to the regex (since all @imports are resolved.) This can cause a segfault in libpcre on some installations.
The patch?
Simply reorders the operations. The new order is optimise, fix the @imports. This means that each file is optimised only once, thus it keeps the size of the text sent to the regex from growing too large.
Comment | File | Size | Author |
---|---|---|---|
#5 | 584370-5-css-aggregation.patch | 1.42 KB | Anonymous (not verified) |
543892-1-css-aggregation.patch | 1.3 KB | JacobSingh | |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedA nice little bit of reversal. In the other thread I wrote the patch and Jacob marked it RTBC.
Comment #2
mattyoung CreditAttribution: mattyoung commented.
Comment #3
Dries CreditAttribution: Dries commentedThis seems to make sense to me.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedAnd set the status back...
Comment #7
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #8
rfayThere are so many issues with our CSS aggregation.
#472820: drupal_load_stylesheet_content() removes whitespace improperly, creating invalid CSS, which has been languishing despite many reviews, is now broken by this one, so I'll fix it up. It's actually only the test that's broken.
Comment #9
rfayThe similar issue #472820: drupal_load_stylesheet_content() removes whitespace improperly, creating invalid CSS has been updated so it's not broken by this. If you were interested in this one you should probably be interested in that one. It's already been RTBC'd before, and I don't think anybody has any trouble with it, so please weigh in on it.
Also, there's a structure in that patch that makes it easy to write trivial tests for situations like this issue. I'm a bit surprised this went in without a test.
Comment #10
rfayIf I'm not mistaken this one did not resolve #544568: CSS aggregation attempts to process @import in comment. Agreed?