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.

CommentFileSizeAuthor
#5 584370-5-css-aggregation.patch1.42 KBAnonymous (not verified)
543892-1-css-aggregation.patch1.3 KBJacobSingh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

A nice little bit of reversal. In the other thread I wrote the patch and Jacob marked it RTBC.

mattyoung’s picture

.

Dries’s picture

This seems to make sense to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.42 KB
Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

And set the status back...

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

rfay’s picture

There 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.

rfay’s picture

The 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.

rfay’s picture

If I'm not mistaken this one did not resolve #544568: CSS aggregation attempts to process @import in comment. Agreed?

Status: Fixed » Closed (fixed)

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