Colorable themes fail when CSS aggregation is enabled due to improper stream wrapper usage

Ouch, yes, totally reproducible...

Steps to reproduce this:

- Use any colorable theme and change the color scheme from default (Garland is good choice because this will really highlight the issue).
- Enable "Aggregate and compress CSS files into one file."
- Surf around the front end - instantly you will see the issue (warning + garland has no colorized images).

Looking at the aggregated CSS I can see the paths to Garlands images look a bit crazy:

 url(/public://color/garland-472c965b/bg-navigation.png)

One thinks this should be:

 url(/sites/all/files/color/garland-472c965b/bg-navigation.png)

Bartik seems to work OK because there are no colorized images., actually no, its borked also, if you change themes while CSS aggregation is on Bartik looses all its color styles.

#859616: drupal_load_stylesheet does not support streams removes the warning but not the problem.

this needs tests .. once we figure out the cause

Adding stream wrapper tag, as the error itself is being caused by chdir() being called on a file uri (public://color/...) rather than a real path. However, changing the chdir command in drupal_load_stylesheet() to use drupal_dirname() and drupal_realpath() still doesn't resolve the missing images in garland.

After a little more digging, the file URIs are being passed into the stylesheet data array here as the $color_path variable, in _color_html_alter():

        if (basename($old_path) == basename($color_path)) {
          // Replace the path to the new css file.
          // This keeps the order of the stylesheets intact.
          $vars['css'][$old_path]['data'] = $color_path;
        }

and then, in drupal_build_css_cache(), these URIs are passed to _drupal_build_css_path() as the $base variable, which is the reason those urls in the aggregated CSS are broken.

I think what's needed here is a method to pull the relative path of a stream uri. For example, given

public://color/garland-4ddc1b47/style.css

the method would return

color/garland-4ddc1b47/style.css

which would go into _color_html_alter() in place of the current $color_path variable.

There's currently a getLocalPath() method, but this returns the realpath, which won't work for _drupal_build_css_path().

Adding regression tag since CSS aggregation works with color module in previous releases.

Subscribing!

Bumping to critical (cringe, sorry) to get some eyes on this (been a month with no movement), really needs to be fixed right away, maintainer can demote if deemed not an RC blocker.

Is this bug report http://drupal.org/node/956228 (Backing up results in: Warning: is_dir(): Unable to find the wrapper "private") related?

I worked around this issue by symlinking "www_root/sites/default/files/" to "www_root/public:". It really does need fixing, though.

I believe file_uri_target() is the API function jhedstrom is describing. Let me see if I can get a working patch put together.

Subscribe.

Ah, seen this then testing the upgrade path but did not pay that much attention to it then I though It prob. was caused of me braking other stuff.

Subscribe

This change in color.module seems to fix the problem. I think it's a rather cludgy (fast) patch however, but it illustrates the problem. Could someone turn this into a real patch please?

--- color.module-old	2010-11-04 17:28:03.000000000 +0100
+++ color.module	2010-11-04 17:27:14.000000000 +0100
@@ -86,6 +86,8 @@ function _color_html_alter(&$vars) {
       foreach ($color_paths as $color_path) {
         // Color module currently requires unique file names to be used,
         // which allows us to compare different file paths.
+        $baselen = strlen(realpath('.').base_path());
+        $color_path = substr(drupal_realpath($color_path),$baselen);
         if (basename($old_path) == basename($color_path)) {
           // Replace the path to the new css file.
           // This keeps the order of the stylesheets intact.

Real patch :)

Thanks for the patch, but as per #20, this isn't the proper fix.

I think the API function ksenzee found in #14 will work. Using realpath won't work for aggregated CSS, which needs a relative path.

This doesn't have anything to do with the cache subsystem, moving to color module.

@jhedstrom file_uri_target() returns the part of an URI after the schema, which isn't the full local path needed. In lack of a proper API function my suggestion #20 uses realpath to create a relative path by creating an absolute path using drupal_realpath() then stripping the absolute bit by using realpath('.'). The solution works but introduces another problem which needs investigating. It really needs a proper API function and needs more testing and documentation.

Rerolling patch

Oops

file_uri_target() indeed does not work, which is why I haven't managed to get a patch posted. But if we're really missing an API function here, we should add it, not work around it. I believe simple API additions are still kosher. I have a couple of hours to spend on this today and will either post a patch or unassign myself.

One can actually build the path relative to the base path by getting the stream wrapper base directory with getDirectoryPath() which together with file_uri_target gives the required path. Of course this only works if we actually have a uri, I added a check with file_valid_uri before translating the path.
I also included a test, that triggers the bug without the patch.
Attached are two different patches, one introduces a new function in file.inc "file_uri_path", which returns the local path relative to the base path for a given uri. The other patch includes this inside of _color_html_alter.

I tested both patches of #31 and they are both working. I'm not sure which one is better - that's up to the cracks to decide. But the errors are gone and the images are showing with both patches. Great work!

I prefer color-aggregate_css-927406-file_uri_path.patch, I see more uses for file_uri_path(). Patch tested and working.

The patch color-aggregate_css-927406-file_uri_path.patch fixes also correctly the issue on my side

Thanks for the test. Seems like we have a good fix as far as color.module is concerned, but I think the patches need more review from people who understand stream wrappers to make sure that API is being used as intended, especially, if we're adding a new API function for it. Therefore, moving to the "file system" component.

I don't have any experience with stream wrappers, but I like the looks of the API function. Could probably be used in other places.

+function file_uri_path($uri) {
+  $stream_wrapper = file_stream_wrapper_get_instance_by_uri($uri);
+  if (!($stream_wrapper instanceof DrupalLocalStreamWrapper)) {
+    return FALSE;
+  }
+  $target = file_uri_target($uri);
+  return $target ? $stream_wrapper->getDirectoryPath() . '/' . $target : FALSE;
+}

This is typically the type of API function that we don't want: it only works with local stream wrappers, and thus spread the confusion between what you can and what you cannot do with stream wrappers.

Instead of re-introducing such a mess (there was an API function like this in the past, that we purposely removed), we need to think about what's the intended behavior here.

It seems clear to me that what actually needs fixing is drupal_build_css_cache() that doesn't know how to properly handle CSS that references URI (or even URLs, for that matter).

I think something like this should be enough to fix the issue. I'm pretty sure our CSS aggregation tests do not cover the use of URLs, but it should be easy to add some.

Thank you, automated testing.

+        // Anchor all paths in the CSS with its base URL, ignoring external and absolute paths.
         $data .= preg_replace_callback('/url\(\s*[\'"]?(?![a-z]+:|\/+)([^\'")]+)[\'"]?\s*\)/i', '_drupal_build_css_path', $contents);
       }

I doubt this part is actually tested.

Yup. That is definitely not tested. This one actually work (but is still untested).

Folding in the test from #31. Even if this particular bug has nothing to do with the color module in the first place, the test still makes sense.

#44: 927406-url-css-files.patch queued for re-testing.

#44: 927406-url-css-files.patch queued for re-testing.

I am a little disturbed that my retest in #46 produced a CSS test fail while the latest retest did not. It feels like we are just rolling the dice to get a pass.

Those fails were unrelated to the patch. It seems that Core was broken for a few hours last night.

#808560: Node comment statistics are only partially exposed in node_load() and are missing last_comment_uid was/is triggering random comment test fails, but I actually saw a CSS test fail when I re-tested in #46. That's why I was concerned, I don't want us to introduce another one of those pesky random fails here.

+  // Load the CSS stylesheet, protect against invalid stylesheets by simply
+  // hiding the possible errors.
+  $contents = @file_get_contents($file);

The comment could use more detail. Invalid stylesheet sounds like one that fails css validation out of context, but we're protecting against a missing stylesheet, so just saying 'missing' or non-existent seems more straightforward here. Also why would we end up with a missing stylesheet in the first place, and want to suppress the error if there is one? I know this is just exchanged for a file_exists() but it's not clear how we'd run into this in the first place.

+  static $current_basepath;

This probably needs a code comment to explain why it's not using drupal_static(). That'll stop someone trying to fix it later.

We're adding a lot of dancing to drupal_build_css_cache(), it's a shame there's no strrtok(). But that also removes this dance from drupal_load_stylesheet():

-    // Change to the current stylesheet's directory.
-    $cwd = getcwd();
-    chdir(dirname($file));

so that feels like a good trade-off.

Two instances of debug in the test which can be removed:

+      $this->verbose('Expected:<pre>' . $expected . '</pre>'
+        . 'Actual:<pre>' . $unoptimized_output . '</pre>'
+      );

catch, carlos8f, Damien Tournoud: looks like the heavy lifting on this issue is done and the patch just needs to include catch's code comments.

I'll try to do that tonight if this isn't resolved by then, but I don't think I'll be able to properly write the comment about why you're using static and not drupal_static. Can one of you write that?

We need to update the docblock for drupal_load_stylesheet() to document $basepath. Also,

  // Load the CSS stylesheet, protect against invalid stylesheets by simply
  // hiding the possible errors.

I'm agreeing with @catch, it's not clear what an "invalid stylesheet" is. If the file in question doesn't exist, it should be triggering a warning so developers can know what to fix. We already wrap processing in if ($contents) in this patch, so we should remove the @ and the corresponding comment about this (unless I'm just misinterpreting and this "@" is essential to the patch).

@cosmicdreams, for the static $current_basepath comment, usually we say something like // Resetting is not necessary, so we don't use drupal_static() here.

Working on cleaning this up.

• Renamed $basepath parameter to $reset_basepath, because that more accurately describes what's happening (we never actually pass a basepath through this parameter). Documented it as internal to the recursive @import resolution functionality. The actual basepath is kept in an internal $basepath static.
• Added comment about the statics: // These statics are not cache variables, so we don't use drupal_static().
• Removed the error suppression operator @ before file_get_contents(), and the "protect against invalid stylesheets" comment. If there is a good reason to suppress errors here, we can re-add that.
• In color.test, replaced regular expression assertions with strpos(), since they are functionally equivalent in this case.
• The new comment "Test Garland with aggregate CSS turned on" was inaccurate because we are also testing Bartik with that code. Changed to "Test with aggregated CSS turned on."
• Removed some unnecessary verbose logging in the CSS unit test.

I also logged the aggregated CSS generated from color.test to a text file, and saw that the resulting color/* image URLs are fixed with this patch. Looks good :)

Ah! Thanks again, automated testing.

The @ has a purpose. I document it as such:

+  // Load the CSS stylesheet. We supress errors because themes may specify
+  // stylesheets in their .info file that don't exist in the theme's path,
+  // but are merely there to disable certain module CSS files.
+  if ($contents = @file_get_contents($file)) {
+    // Return the processed stylesheet.
+    return drupal_load_stylesheet_content($contents, $_optimize);
   }

Misspelled "suppress" in the comment.

This patch solves the error when saving the colorable theme's settings (or clearing the cache), code looks good.

Committed to CVS HEAD. Thanks.