I turned on CSS aggregation for the first time, and all my background images in the theme disappeared!

Specifically,

body {
	background: #fff url(images/headerBg.gif) repeat-x 0 -20px;
}

was "compressed" into this:

body{background:#fff url(/sites/all/themes/images/headerBg.gif) repeat-x 0 -20px;}

On closer inspection, drupal somehow converted the url images/headerBg.gif into /sites/all/themes/zen/ew/images/headerBg.gif and then again into /sites/all/themes/images/headerBg.gif. It's the last transformation that was problematic.

After a bit of digging, the problematic code is includes/common.inc, in function _drupal_build_css_path, reproduced here:

/**
 * Helper function for drupal_build_css_cache().
 *
 * This function will prefix all paths within a CSS file.
 */
function _drupal_build_css_path($matches, $base = NULL) {
  static $_base;
  // Store base path for preg_replace_callback.
  if (isset($base)) {
    $_base = $base;
  }

  // Prefix with base and remove '../' segments where possible.
  $path = $_base . $matches[1];
  $last = '';
  while ($path != $last) {
    $last = $path;
    $path = preg_replace('`(^|/)(?!../)([^/]+)/../`', '$1', $path);
  }
  return 'url('. $path .')';
}

After a bit of experimentation, it turned out the regular expression is at fault. It forgets to escape the two periods with backslashes. Note that (?!../) refers to the look-ahead negative assertion as described here:
http://ca.php.net/manual/en/regexp.reference.php#regexp.reference.assert...

Because the periods aren't escaped, this regular expression replaces "/zen/ew" by "/". The obvious patch:

Index: includes/common.inc
===================================================================
--- includes/common.inc (revision 1348)
+++ includes/common.inc (working copy)
@@ -1810,7 +1810,7 @@
   $last = '';
   while ($path != $last) {
     $last = $path;
-    $path = preg_replace('`(^|/)(?!../)([^/]+)/../`', '$1', $path);
+    $path = preg_replace('`(^|/)(?!\.\./)([^/]+)/\.\./`', '$1', $path);
   }
   return 'url('. $path .')';
 }

The problematic code was introduced in the controversial path for this issue: http://drupal.org/node/113607
A comment by chx even suggested that instead of a regular expression, some php function (perhaps realpath?) should be used.
http://drupal.org/node/113607#comment-500017

CommentFileSizeAuthor
#4 css-compressor-regex.patch590 bytesJamie Holly

Comments

dergachev’s picture

Title: Simple CSS Compression Bug » CSS Aggregation: regexp mangles URL's in CSS
bsdunx’s picture

I tried to reproduce this with my own zen sub-theme but was not able to, it worked just fine.

body {
background: #fff url(images/test.gif) repeat-x 0 -20px;
}

to

body{background:#fff url(/sites/all/themes/zen/bsdunx/images/test.gif) repeat-x 0 -20px;}

damien tournoud’s picture

Version: 6.x-dev » 7.x-dev
Priority: Critical » Normal

The bug only occurs when one directory has only two characters (like "ew" in the example above). The solution outlined by ew looks sane. Could someone please roll a proper patch?

Because the bug is also in 7.x-dev, bumping release. Also I don't believe this is as critical as other issues :)

Jamie Holly’s picture

StatusFileSize
new590 bytes

Patch file attached. I tested it with a two character directory and it converted the URL properly.

kscheirer’s picture

worked for me, applies cleanly with 14 line offset.

kscheirer’s picture

Status: Needs review » Reviewed & tested by the community

#4 and #5 reported success with this patch.

dergachev’s picture

bump

webchick’s picture

Status: Reviewed & tested by the community » Needs work

That regex is terrifying. Can we please have a comment there that indicates what the heck it's checking so we don't mess it up again?

lilou’s picture

hass’s picture

As one of the drivers of the linked @import patch I can say that I've tested this CSS compressing functions in all "thinkable" variants of URL's... So subscribing here to learn.

Jamie Holly’s picture

There's actually a comment about the purpose of the regex a couple of lines above where the patch occurs:

// Prefix with base and remove '../' segments where possible.

All the patch does is simply escape the .'s. It was nothing more than a simple oversight. I'm not really sure how to comment it much better than what's already there (if anyone has a suggestion, I'll gladly make a new patch).

hass’s picture

If someone change the directory mangle regex... don't forget to change $before = preg_replace('`(^|/)(?!../)([^/]+)/../`', '$1', $before); in color.module line 432, please.

dergachev’s picture

Status: Needs work » Closed (duplicate)

Turns out that this was a duplicate of this issue, which was just committed to core.
http://drupal.org/node/258846

However, I'm not sure comment #12 has been addressed.

casey’s picture

Issue tags: +CSS aggregation

tagging

hargobind’s picture

subscribing