IE @font-face CSS hack URL broken by CDN module's overridden CSS aggregation

Actually, the CDN module explicitly *strips* query strings because they're bad for cacheability! I understand that it's useful in this particular case, but if anything, you should add this to cdn.basic.css.inc, and *only* allow query strings to exist if the result is a superstring of ".eot?#iefix".

There's zero browser-specific code in the CDN module, which is why I'm *very* hesitant to add this.

The article at http://www.fontspring.com/blog/the-new-bulletproof-font-face-syntax appears to suggest that one can also use *two* @font-face declarations to get it to work. Did you try that?

Setting to active as more info has been provided in #4.

I definitely understand where you're coming from, BUT:
- again, I would really like to avoid browser-specific code
- IE >=9 supports WOFF (http://blog.linotype.com/2011/07/webfonts-update-woff-format-now-support...)
- IE <=8 is slow anyway, so a slightly slower loaded font doesn't matter all that much.

Hence I'm heavily inclined to wontfix this, and just ask people who really want this to apply your patch in #2. *If* this is going in, the code path for this can't be part of the default code path, I want it to be explicitly in a separate function, to make it easier to remove it in the future.

Not sure if this is really the same issue, but I guess it's related. I can't find anything in the CDN code that would cause this however. When the CDN module is disabled, we don't have this issue.

We're getting a lot of 404s on this:
.../geo-md-webfont.eot%3F%23iefix
which is found in the aggregated css only and comes from css like this:

@font-face {
    font-family: 'GeogrotesqueMediumRegular';
    src: url('geo-md-webfont.eot');
    src: url('geo-md-webfont.eot?#iefix') format('embedded-opentype'),
         url('geo-md-webfont.woff') format('woff'),
         url('geo-md-webfont.ttf') format('truetype'),
         url('geo-md-webfont.svg#GeogrotesqueMediumRegular') format('svg');
    font-weight: normal;
    font-style: normal;
}

The "?#iefix" is replaced in the aggregated css with "%3F%23iefix" which is wrong.

I'd suggest to add a checkbox "Strip querystrings from files referenced in css" to the admin panel. If there's a "?" in the css file, it means someone put it there for a reason and shouldn't be removed by the CDN module. I also don't think there are many browsers left anymore which don't cache based on a querystring.

#7: "%3F%23iefix" is treated identically to "?#iefix" by browsers and web servers. It shouldn't make a difference. What am I missing?

Also, the major concern for worsened cacheability isn't browsers, but proxies.

In #1, I said query strings are stripped. According to #7, they're not, they're just URL-encoded. I think #2 means that I'm explicitly not adding the css_js_query_string query string, but I actually can't find any such modifications in cdn.basic.css.inc, nor are those changes documented.
It seems obvious that the unit tests need to be expanded to test this as well.

Is there a reason to URL encode filenames in css? Apparently some browsers have trouble with this (I can't see which ones in our logging but I have an idea about the culprit).

Yes, it is necessary for special characters such as "é", "±" and whatnot. That being said, the query string you're seeing *shouldn't* be URL encoded, I think.

That's why I said in #8 that unit tests should be expanded in this area.

Basic (failing) testcase in attach which demonstrates the issue.

I pinpointed the problem to _cdn_build_css_path() which uses
return 'url(' . file_create_url($path) . ')';
which just encodes $path without regard for querystrings (the '?' mainly). This is actually the part which differs from _drupal_build_css_path() which doesn't has this issue.

Patch in attach for the 7.x-2.x branch which fixes the issue for us. There is no browser specific parsing in this patch, it just removes the querystring from the part of the url that is url-encoded.

It seems to me that this is not only necessary for this font issue but also for a quite common practice of putting ?v2 at the end of images in a css file. Whether that is a good approach is another discussion, but the cdn module shouldn't break this intended behaviour IMHO.

+++ b/cdn.basic.css.incundefined
@@ -127,7 +127,7 @@ function _cdn_build_css_cache($css, $suffix = '') {
+ * Near-identical to @see _drupal_build_css_path().

Thanks :)

+++ b/cdn.basic.css.incundefined
@@ -146,5 +146,17 @@ function _cdn_build_css_path($matches, $base = NULL) {
-  return 'url(' . file_create_url($path) . ')';
+  // When the css contains a querystring part then don't pass that part to file_create_url() since
+  // that function performs drupal_encode_path() on $path which incorrectly escapes the querystring part
+  $url = null;
+  $query_start = strpos($path,'?');
+  if ($query_start > 0) {
+    $query = substr($path,$query_start);
+    $path = substr($path,0,-strlen($query));
+    $url = file_create_url($path) . $query;
+  } else {
+    $url = file_create_url($path);
+  }

This is too long for doing something so simple. You don't follow coding style, comments don't wrap at 80 cols, etc. :(

Can you review the attached patch? :)

Something so simple is trickier than one might think.
The patch in #17 doesn't include the fragment (the #) for "geo-md-webfont.eot?#iefix".

$parts = parse_url($path);
$suffix = (isset($parts['query']))? '?' . $parts['query'] : '';
$fragment = (isset($parts['fragment']))? '#' . $parts['fragment'] : '';
return 'url(' . file_create_url($parts['path']) . $suffix . $fragment . ')';

This adds the fragment, but the '?' is left out because the querystring is empty resulting in "geo-md-webfont.eot#iefix". I'm not sure if this is still ok for the iefix to work (why would they add it otherwise).

This is the reason I use strpos and substr. Also, I think parse_url is too heavy for what is needed since this will be called a lot, but this claim would need to be validated with a test.

About the coding style: I agree I should be reading up on this for Drupal style. But this is what tools are for. I just provide the fix.

Coding style is more than just beauty. It's also about making sure that anybody familiar with the coding style can dive into the code more easily ;)

You're right about speed (testing with 100 URLs):

$ php profile_parse_url.php 
parse_url:                    0.00094008445739746
strpos+substr:       0.00029802322387695

(Test script attached.)

However, the file system is also being hit for every file when calculating unique URLs for Far Future expiration. That makes this performance win negligible. Hence this is a clear case of premature optimization. Keep your code clean. Only resort to lower level things where it actually makes sense.

F*ck you, drupal.org.

I'm very aware of what premature optimization means, thank you. As stated, not performance but a correct fix (because there's a fragment and I guess this won't work when you encounter a full URL in css because the scheme or hostname won't be included) was the reason for this code. Besides that, not everyone uses the Far Future option.

I think I am having a related issue, and the patch in #17 dosn't seem to fix it.

I'm not using far future expiry, but I do have a CSS url with a fragment in for a filter the rule is this.

filter: url("http://www.example.com/sites/www/themes/example/css/desaturate.svg#greyscale");

This is being escaped to

filter: url("http://www.example.com/sites/www/themes/example/css/desaturate.svg%23greyscale");

So I guess there are two issues, one is that it is not being recognised as a svg file and 2 is that it is escaping the #

(The escaping should be ok, but firefox seems not to recognize it)

The fragment can't be chopped as it is a valid part of the address.

Is this the same as this issue, or is it unrelated? The patch just seems to chop the fragment which is not ok.

The problem lies in the function _cdn_build_css_path() in cdn.basic.css.inc.

The problem is the call file_create_url() in the last line of the function. file_create_url() "creates a web-accessible URL for a stream to an external or local file" and the problem is that it expects a file path as argument and _cdn_build_css_path() calls it with an url path. Hence the wrong encoding of the # (and maybe ? and & as well?).

Solution? Use something else than file_create_url()? Chop of query parameters and/or fragments before calling file_create_url() and reapplying them afterwards?

A patch implementing my idea from #23. Chops off query part / fragment and reapplies after file_create_url().

Combining #17 and #24 into one patch. This seems to solve the problem for me, please review.

This patch got my drupal broken

It seems like patch #25 is working - at least for me, thanks. Although now I can't add font files to cdn(Subdomain) - adding to cdn stops loading them at all. Before using this patch loading fonts from cdn worked(At least I think so. :D), but in IE(Every version.) it took like 1 second to load(Even if I used only css, or js or images in cdn.) - with every page load showed wrong font and then after 1 second loaded correct font. But now they are not loading from cdn at all. It's not big problem, at least now I can use this module and IE users see correct font right from start of page load.

Please try version 2.6, in #1926884: CDN module is not compatible with security fix in Drupal core update 7.20 a related change was made.

The issue still occurs in version 2.6.

The patch in #25 which really is my patch from #18 isn't complete since it removes the '?' from the URL resulting in "webfont.eot#iefix". This may be the reason it loads slow in IE.

My patch from #12 is the only one which works for us since it keeps both the '?' and the '#' in the url resulting in "webfont.eot?#iefix". This patch doesn't use parse_url() but just keeps everything after the '?' and appends it to the generated URL. It can probably be made fancier with a regex, but here is a simple version with explode():

$parts = explode('?',$path);
$url = file_create_url($parts[0]);
if (!empty($parts[1])) {
  $url .= '?' . $parts[1];
}
return "url($url)";

Okay, thanks! The good news for you is that once #1926884-10: CDN module is not compatible with security fix in Drupal core update 7.20 (i.e. #1936176: image.module uses file_create_url() incorrectly) is solved, I'll have to make query strings in file URLs work anyway, and then this will effectively be implemented & allowed :)

That means instead of inserting hacks in the CSS aggregation layer, it will be once again be allowed to do it the way the very first patch on this issue did it: by modifying cdn_file_url_alter(). This time, however, it will need to be covered by unit tests.

Any takers? :)

Maybe this issue is related to the Drupal javascript for stylesheet loading

I've just hit the iefix issue as well on our site. It's happening to me even on IE9, I think because the font directives aren't qualified with specific IE versions.

I'm happy to provide any info or try something if it helps in getting this addressed. I'm not a drupal expert, probably can't fix it myself.

What's the easiest workaround in the meantime? Does the patch in #12 work? It looks like it fails tests -- is that ok? Or is there an easy CSS tweak? We have to support down to IE8.

ps. I hate IE :)

I'm not sure myself anymore which version you should use, but one thing remains certain: test coverage is still missing.

Hello guys,

I think this is a hard issue because it's broke IE8 and Firefox:
- Firefox uses SVG fonts and needs #font-id at the end of URL to render correct internal ID of SVG and it isn't working, it's already quoted on #22.
- For IE8 with patch #2 it's worked, but just for IE issue.

Someone have any update?

Thanks.

Decoding urls because Firefox, IE, and other browsers/versions can't handle encoded URLs in css files, for instance in svg paths containing encoded hashes "#" mentioned in #34.

This patch is a different approach to solving #35.

Please see the reasoning in comments #5 and #6 of AdvAgg's issue #2112067.

I want to discuss two points Wim made that I believe are wrong. I believe your adherence to these philosophies are guiding you to solutions that are inappropriate or unnecessarily complex.

From comment #1:

Actually, the CDN module explicitly *strips* query strings because they're bad for cacheability!

In some cases query strings are bad for cacheability. In the case of files inside css I do not believe this to be an accurate or particularly relevant statement. Query strings on assets referenced in css should be managed explicitly so that they are cached when needed and break the cache when needed. Sass has the asset_cache_buster function specifically for this purpose. As a workaround, we now version our files like "sprite-1.png" and then bust the cache by making it sprite-2.png. This form of cachebusting is required by the deficiency of CDN.

From comment #8:

#7: "%3F%23iefix" is treated identically to "?#iefix" by browsers and web servers. It shouldn't make a difference. What am I missing?

WAT? Here are three images from d.o. but only two of them will load (tested in Firefox and Chrome latest on MacOS10.8).

• No trailing anything:
• Trailing ?#iefix
• Trailing %3F%23iefix

I think that makes it clear that when it comes to assets, ?#iefix and %3F%23iefix are very different things.

Also, there actually are tests (not sure how good they are) in the zip in comment #11. Attached here as a patch. I'm hopeful they "fail". I modified them by removing windows newlines from the files, but otherwise they are the same as the zip.

Not sure why that didn't apply. I rebased immediately before creating it. Maybe it's a matter of version?

#37: 1514182_tests_should_fail.patch queued for re-testing.

OK, my method of creating a diff with new files wasn't working. Let's try this.

I guess the patch applies but the tests were not detected because the number of tests is the same as comment #36. At least the goal is achieved that they are more readily accessible than they were when hidden in a test zip file. I don't have time to work on this further now, but hope others will be aided by having some potential tests.

For what it's worth, my process to make the patch:

git checkout -b 1514182
...move in the zip file contents...
git add css/ cdn_css.test
git commit -am"Issue #1514182 tests"
git diff 7.x-2.x > ~/Desktop/1514182_tests_should_fail_42.patch

With thanks to c4rl for how to make the patch.

Seems like Chrome don't support anymore SVG stacks: https://code.google.com/p/chromium/issues/detail?id=128055#c6

The tests didn't run in the last patch because .info didn't recognize the new file.

I consolidated and cleaned-up some.

I utilized parse_url()to rebuild the url with a tip from http://www.php.net/manual/en/function.parse-url.php#106731 and a slight caveat for blank querystrings (i.e. we have to add the "?" back in manually).

Trailing whitespace FTL.

Hm look like clean URLs are impacting the test. I'll see what I can do to fix this.

Let's try this. Also created patch with -C and -M options to reduce size.

I was using the last stable release and had this bizarre bug of font-face not working when aggregating my css.

This issue seems to be fixed in the dev version.

oops too quick... sorry, not fixed.

Some may find themselves here following an attempt to set the CORS policy with apache mod_header, and subsequently realizing that's not the problem with their font not showing.

We couldn't seem to solve our issue from this thread, so our current project's approach, for the present, was the roll the following module. It's not awesome, but it is working for us. We're using icomoon locally, and did not want to alter anything in the package.

cdn_fontface_fix.info :

name = CDN Fontface Fix
description = Addresses an incompatibility with certain fontface includes and CDN + Drupal CSS Parsing
core = 7.x
php = 5.2.4
version = 7.x-1.0
dependencies[] = cdn

cdn_fontface_fix.module:

/*

REASON FOR THIS MODULE

Issue: https://drupal.org/node/1514182

There is presently a problematic bug involving the combination of the CDN module,
approved css browser "hacks" for cross-browser font support, and Drupal css preprocessing.

This solution is to preprocess properly with a custom add_css function.


EXAMPLE USAGE (ICOMOON)
 
function your_theme_preprocess_html($path)
  if (function_exists('cdn_fontface_fix_add_css')) {
	cdn_fontface_fix_add_css(path_to_theme() . '/fonts/icomoon/style.css', base_path() . path_to_theme() . '/fonts/icomoon');
  }
  else {
	drupal_add_css(path_to_theme() . '/fonts/icomoon/style.css');
  }
}


REQUIREMENTS

All url(..) definitions in a given stylesheet be relative.


RECOMMENDATIONS

Only use cdn_fontface_fix_add_css for lightweight stylesheets that center on @font-face declaration(s)

*/

define("CDN_FONTFACE_FIX_URI_BASE", "public://cdn_fontface_fix");

function cdn_fontface_fix_add_css($path, $path_prefix = '') {    
  if ($file = cdn_fontface_fix_process_css_file($path, $path_prefix)) {
    drupal_add_css(
      $file,
      array(
        'group' => CSS_THEME,
        'type' => 'file',
        'media' => 'screen',
        'preprocess' => FALSE,
        'weight' => '9999',
      )
    );
  }
}

function cdn_fontface_fix_process_css_file($path, $path_prefix) {
  
  if (!cdn_status_is_enabled()) {
    if (file_exists($path)) {
      return $path;
    }
  }

  $cached_path = CDN_FONTFACE_FIX_URI_BASE . '/' . $path;
  
  if (cdn_status_is_enabled()) {
    
    if (file_exists($cached_path)) {
      return $cached_path;
    }
    
    // Assumes you've included a line such as the following in CDN Mapping field.
    // http://your-cdn-domain.com|.css .js .jpg .gif .png .woff

    $uri = 'font.woff';
    $mode = variable_get(CDN_MODE_VARIABLE, CDN_MODE_BASIC);
    cdn_load_include(($mode == CDN_MODE_BASIC) ? 'basic' : 'advanced');
    $servers = ($mode == CDN_MODE_BASIC) ? cdn_basic_get_servers($uri) : cdn_advanced_get_servers($uri);
    
    $server = '';
    
    if (!empty($servers)) {
      $server = $servers[0]['server'];
    }
    
    if (file_exists($path)) {
      
      $css = file_get_contents($path);
      
      if ($server != '') {
    	$css = str_replace("url('", "url('$server$path_prefix/", $css);
      }

      $path_parts = pathinfo($cached_path);      
      drupal_mkdir($path_parts['dirname'], NULL, TRUE);
      return file_unmanaged_save_data($css,$cached_path);
    }
    else {
	  return FALSE;
    }

  }

}

/**
 * Implements hook_flush_caches().
 */
function cdn_fontface_fix_flush_caches() {
  file_unmanaged_delete_recursive(CDN_FONTFACE_FIX_URI_BASE);
}

@Marc Angles, @bartclarkson - could you try the patch in #50?

A note that the Fontello project also runs into this because the fontello service generates a bundle including css for the fonts such as:

@font-face {
  font-family: 'myfont';
  src: url('../font/myfont.eot?22113345');
  src: url('../font/myfont.eot?22113345#iefix') format('embedded-opentype'),
       url('../font/myfont.woff?22113345') format('woff'),
       url('../font/myfont.ttf?22113345') format('truetype'),
       url('../font/myfont.svg?22113345#myfont') format('svg');
  font-weight: normal;
  font-style: normal;
}

We've applied cdn-1514182-50.patch on http://drupalwatchdog.com/ -- works great, thanks!

+1 this needs to be committed

Ditto on being RTBC.

I can't even get patch 50 to apply. I have tried with drush and terminal on Mac OSX with different results

Using Drush

cdn-7.x-2.x-dev downloaded. [60.33 sec, 13.47 MB] [ok]
/home/o2.ftp/.drush/cache/download/http---drupal.org-files-issues-cdn-1514182-50.patch retrieved from cache. [60.33 sec, 13.42 [notice]
MB]
patching file b/cdn.basic.css.inc [notice]
Hunk #1 FAILED at 133.
Hunk #2 FAILED at 145.
2 out of 2 hunks FAILED -- saving rejects to file b/cdn.basic.css.inc.rej
patching file b/cdn.info
Hunk #1 FAILED at 4.
1 out of 1 hunk FAILED -- saving rejects to file b/cdn.info.rej
patching file b/tests/cdn.test
Hunk #1 FAILED at 547.
1 out of 1 hunk FAILED -- saving rejects to file b/tests/cdn.test.rej
patching file b/tests/css/orig/url.css [60.42 sec, 13.43 MB]
Unable to patch cdn with cdn-1514182-50.patch. [60.42 sec, 13.09 MB] [error]

Using terminal

MacBook-Pro:cdn $ patch < cdn-1514182-50.patch
patching file cdn.basic.css.inc
patching file cdn.info
Hunk #1 FAILED at 4.
1 out of 1 hunk FAILED -- saving rejects to file cdn.info.rej
patching file cdn.test
patching file url.css

Here's what I did to get it to apply:

➜ ~/checkouts git clone --branch 7.x-2.x http://git.drupal.org/project/cdn.git
Cloning into 'cdn'...
remote: Counting objects: 1506, done.
remote: Compressing objects: 100% (999/999), done.
remote: Total 1506 (delta 1011), reused 741 (delta 492)
Receiving objects: 100% (1506/1506), 438.94 KiB | 0 bytes/s, done.
Resolving deltas: 100% (1011/1011), done.
Checking connectivity... done.
➜ ~/checkouts cd cdn
➜ ~/c/cdn git:(7.x-2.x) wget https://www.drupal.org/files/issues/cdn-1514182-50.patch
--2014-07-27 14:30:13-- https://www.drupal.org/files/issues/cdn-1514182-50.patch
Resolving www.drupal.org... 72.21.91.99
Connecting to www.drupal.org|72.21.91.99|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: 5021 (4.9K) [text/plain]
Saving to: 'cdn-1514182-50.patch'

100%[========================================================================================================>] 5,021 --.-K/s in 0s

2014-07-27 14:30:15 (73.7 MB/s) - 'cdn-1514182-50.patch' saved [5021/5021]

➜ ~/c/cdn git:(7.x-2.x) ✗ git apply < cdn
cdn-1514182-50.patch cdn.advanced.inc cdn.basic.inc cdn.fallback.inc cdn.module
cdn.admin.css cdn.basic.css.inc cdn.constants.inc cdn.info cdn.stats.inc
cdn.admin.inc cdn.basic.farfuture.inc cdn.css cdn.install cdn.test
➜ ~/c/cdn git:(7.x-2.x) ✗ git apply < cdn-1514182-50.patch
➜ ~/c/cdn git:(7.x-2.x) ✗ git status
On branch 7.x-2.x
Your branch is up-to-date with 'origin/7.x-2.x'.

Changes not staged for commit:
(use "git add/rm ..." to update what will be committed)
(use "git checkout -- ..." to discard changes in working directory)

modified: cdn.basic.css.inc
modified: cdn.info
deleted: cdn.test

Untracked files:
(use "git add ..." to include in what will be committed)

cdn-1514182-50.patch
tests/

no changes added to commit (use "git add" and/or "git commit -a")
➜ ~/c/cdn git:(7.x-2.x) ✗

Which I guess is worth mentioning that to commit this it will be necessary to "git add" the tests directory to ensure those aren't lost.

Nice one, thanks for that very helpful, at least I know the patch _should_ work...

My issue is that I use drush make files to build custom profiles. My only solution which is a rather rubbish one is to create a repository just for this module (manually patched) and then pull that in using the .make file and drush.

Weird thing is I have 3-4 other modules which patch just fine.

Working Example

projects[remote_stream_wrapper][subdir] = "contrib"
projects[remote_stream_wrapper][version] = "1.0-beta4"
projects[remote_stream_wrapper][type] = "module"
projects[remote_stream_wrapper][patch][] = http://drupal.org/files/broken-image-style-7.20-1926434-0.patch

Non-working Example

projects[cdn][subdir] = "contrib"
projects[cdn][version] = "2.x-dev"
projects[cdn][type] = "module"
projects[cdn][patch][] = http://drupal.org/files/issues/cdn-1514182-50.patch

I wonder if..

"Let's try this. Also created patch with -C and -M options to reduce size."

Does anything special to the patches that might cause this.

@greggles - I removed the cdn_fontface_fix.module, it has some some bugs. And applied the patch from #50, it seems to fix the issue. @bartclarkson was busy, so I responded.

Heads up that this will still break SVG in CSS
#2362643: Drupal alters svg fill paths with base url -> broken svg

Rerolled against dev. This did work for its stated purpose. If this passes we should re-RTBC.

Also hiding some older patches.

Rerolled+ minor simplification that IMHO improves readability.

@paranojik - thanks for posting the interdiff. In the first hunk it looks like you changed the order of concatenation of the url. Can you clarify if that was intended and why that makes sense? My understanding is that the order should be $scheme . $user . $pass . $host . $port . $path but it looks like you put $user and $pass after the host and port. I'm not sure your order works.

Ouch... You're right @greggles, thanks!

Relevant files attached.

I can confirm the previous patch. It has worked for me in conjunction with Amazon Cloudfront.

Confirming that patch in #66 resolves the issue for us. In our case it was also breaking the assets on the Ember theme as they're versioned, e.g. 'url('../images/ui-icons-222222-256x240.png?1399128626')'.

Patch #68 applied to the dev version resolves the issue for me, using fontello. But for whatever reason, the patch didn't apply fully, so I had to fix it by hand:

--- cdn.info
+++ cdn.info
@@ -4,4 +4,4 @@
core 7.x
package = Performance and scalability
configure = admin/config/development/cdn
-files[] = cdn.test
+files[] = tests/cdn.test

Yeah usually *.info changes don't apply without the git version because the packaging info is in that file from the download.

Hey Joel,
Ah I see... you learn something new everyday. Thanks for letting me know.

The newly added test causes failures if it is not run separately (testbot runs each test separately). i.e. if you run the CDN test suite (i.e. all tests) through the UI, the addition of this test causes many failures.

Figuring out a work-around.

(I tried converting to a unit test already, but that doesn't work, because the function called for the test invokes variable_set(), which we cannot simply mock. This is where D8's dependency injection shines, it'd be easy to test there…)

Found a way. With these changes, the CDN module now has base classes for both unit tests and web tests. Rerolled with -M, to have a much smaller patch.

Yay, also green :)

Sorry for taking far too long to commit this, I know this affects quite a few sites/people. It's unfortunate though that I was still able to find the significant problems above.

Glorious :)

If you still use that adjective after testing the 2.7 beta, I'll actually start agreeing with it :P