CSS aggregation fails to parse some url()'s

Follow here : #289293: CSS Aggregation: regexp mangles URL's in CSS

subscribe

Color module is missing in the patch.

I'm not sure how we would test this. I guess there needs to be a dummy simpletest.css file that has urls with leading and trailing whitespace. Then execute drupal_build_css_cache() and check the resulting file for the correct paths?

Or we could store the regular expression as a variable or constant, and write a check against it directly using $this->assertPattern()?

tagging

After this one i'll have a look at

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

-  $contents = preg_replace_callback('/@import\s*(?:url\()?[\'"]?(?![a-z]+:)([^\'"\()]+)[\'"]?\)?;/', '_drupal_load_stylesheet', $contents);
+  $contents = preg_replace_callback('/@import\s*(?:url\()?\s*[\'"]?(?![a-z]+:)([^\'"\()]+)[\'"]?\s*\)?;/', '_drupal_load_stylesheet', $contents);

White-space is also allowed before the trailing semicolon.
A minor nit: I suggest moving the first new \s* into the parenthesis to prevent unnecessary backtracking caused by the two battling \s* when there is no url( prefix, i.e. like this:

-  $contents = preg_replace_callback('/@import\s*(?:url\()?[\'"]? ...
+  $contents = preg_replace_callback('/@import\s*(?:url\(\s)?*[\'"]? ...

-  return preg_replace('/url\(([\'"]?)(?![a-z]+:)([^\'")]+)[\'"]?\)?;/i', 'url(\1' . dirname($filename) . '/', $file);
+  return preg_replace('/url\(\s*([\'"]?)(?![a-z]+:)([^\'")]+)[\'"]?\s*\)?;/i', 'url(\1' . dirname($filename) . '/', $file);

White-space is also allowed here before the trailing semicolon.

Apart from that, it looks good.

Looks like some tests are much needed. There is already one CSS aggregation test in modules/simpletest/tests/common.test.

hmmm patch is still pretty broken.

Reroll with suggestions from c960657.

Subscribing

subscribing

Reroll

Reroll

Duplicates
#567086: Optimise CSS feature does not allow whitespace
#352042: drupal_build_css_cache() cannot deal with a url() statement not surrounded by spaces.

Note that both items in #18 were D6 issues.

subscribe
There's a working patch in #444228: Optimize CSS option causes php cgi to segfault in pcre function "match"

Reroll. Can we get a RTBC here? This patch is so obvious.

@andypost, that patch is about something else.

Last hunk seems removes ability to have space between url and (

That is how it is supposed to be; http://www.w3.org/TR/CSS21/syndata.html#uri

So it's looks fine except no tests. Could you extend a test from modules/simpletest/tests/common.test
within CascadingStylesheetsUnitTest class

What should the test contain? whether an invalid url() in a stylesheet is skipped when being preprocessed? I am not sure this needs a test.

I reviewed this again and see no reason to write a test for this preg so RTBC

Still applies

Committed to CVS HEAD. Thanks.

@casey, Please, re-roll this for D6

All three regexes of the recently committed patch do not work correctly. They all now erroneously match absolute and external URLs if they are unquoted and have leading whitespace. Looking at the first of the three regexes, here is an example of what happens when drupal_build_css_cache() processes susceptible external and absolute URLs:

Given:
$base_path = 'basepath/';
$latest_regex = '/url\(\s*[\'"]?(?![a-z]+:|\/+)([^\'")]+)[\'"]?\s*\)/i';

Before drupal_build_css_cache():
url( http://example.com/path/filename.txt );
url( /path/filename.txt );

After drupal_build_css_cache():
url(basepath/ http://example.com/path/filename.txt );
url(basepath/ /path/filename.txt );

The erroneous match happens because the \s* expression is able to give up one of its spaces to allow the negative lookahead to incorrectly match absolute and external URLs (at a position one space back from where the URL string actually starts). This problem is easily fixed for this particular regex by adding a possessive + to the first \s* expression.

The attached patch possessifies the \s* and all other applicable quantifiers to both fix the above mentioned bug and to improve efficiency (especially for non-matches). This patch also fixes another nasty bug (in the last regex in drupal_load_stylesheet_content()) where @imports having absolute URLs (which should be ignored and left alone) were instead being matched and then effectively deleted. This patch also adds the whitespace fix to the identical URL matching regex in the color module which was overlooked by the previous patch.

It seems reasonable I've not tested previous patch with baseurl.

@ridgerunner How can I help to test this?

seems like we should roll back this commit and then get some tests in place with a new patch.

Suppose a follow-up is enough

Setting back to state of commit at #28. Please open a new issue if there is still a bug.

The problems indicated in #30 are only possible because #21 got committed without additional test coverage. Sad. But still, please open a new issue. It's been sitting here for almost 3 years because it's so hard to find.