Updated after comments #6, #7

Paths embedded in CSS are things like links to background image files, font files, etc.

In Advagg 7.x-2.0 (see original issue, below), these paths were raw URL encoded, which means they would return 404 errors (at least in most browsers).

The fix in comment #4 of this issue (committed to -dev) means that CSS paths are (at least sometimes) written out correctly.

But the process is now a bit perverse - currently, URLs are encoded, and then immediately decoded.

Original issue

I'm using the FontAwesome module.

FontAwesome's CSS does this:

@font-face {
  font-family: 'FontAwesome';
  src: url('../font/fontawesome-webfont.eot?v=3.2.1');
  src: url('../font/fontawesome-webfont.eot?#iefix&v=3.2.1') format('embedded-opentype'), url('../font/fontawesome-webfont.woff?v=3.2.1') format('woff'), url('../font/fontawesome-webfont.ttf?v=3.2.1') format('truetype'), url('../font/fontawesome-webfont.svg#fontawesomeregular?v=3.2.1') format('svg');
  font-weight: normal;
  font-style: normal;
}

Everything was working fine until upgrading to the latest version of AdvAgg (from 2.0-rc4 to 2.0).

AdvAgg now appears to rewrite ?v=3.2.1 to %3Fv%3D3.2.1, which causes 404 errors and of course FontAwesome doesn't load.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeytown2’s picture

Off the top of my head I think it might have something to do with this patch #2103183: Implement #1961340 in AdvAgg

mikeytown2’s picture

pjcdawkins’s picture

OK, yes it seems #1514182 is closely related. I don't (yet) have the CDN module enabled, by the way.

I don't really understand what's going on here (why does the URL need to be encoded at all?)

mikeytown2’s picture

Status: Active » Fixed
FileSize
919 bytes

Following patch has been committed. Let me know if this doesn't fix the issue.

pjcdawkins’s picture

That does fix the issue but I am concerned that the paths are being raw-URL-encoded and then unencoded - surely that's not right?!

(And if it's "some browsers cannot handle", which browsers can handle encoded URLs in CSS? Why should we expect them to handle them?)

pjcdawkins’s picture

Status: Fixed » Needs review
FileSize
1.08 KB

Here's a patch against current -dev (after the patch in #4) which prevents the whole URL encode / decode chain.

pjcdawkins’s picture

Title: Version query string (for embedded fonts) is incorrectly rewritten, encoded » Paths embedded in CSS are unnecessarily encoded (and now decoded)

Updating title now that #4 is in.

mikeytown2’s picture

Status: Needs review » Fixed

That might cause unknown issues as we are now altering the path, and no one has any test coverage on this. I'm OK with encoding/decoding as I know it is safe; in this case I would rather spend an extra 0.5ms to be safe. Moving this back to fixed.

Once CDN/Core picks a way to handle the font case I'll be sure to mirror it, but for now I would like to be safe.

mikeytown2’s picture

Status: Fixed » Closed (works as designed)

Actually since the title changed, I'll move this to works as designed.

pjcdawkins’s picture

I thought making sure $path starts with a '/' (like any relative URL) was legitimate, partly because no-one wants it to be an absolute URL until file_create_url() & url() are called, and partly because the first time advagg_context_css() is called, the $base is passed in with a '/' at the end anyway. The function file_create_url() itself uses the initial '/' to tell what type of path it is, and based on that just makes a decision of whether or not to rawurlencode it.

I'm not worried about performance here, just the smell of the code.

mikeytown2’s picture

Reason why I don't want to alter anything is the fact that file_create_url() calls hook_file_url_alter(). I can't guarantee bad things won't happen thus I will be playing it safe.

I do agree that it's not elegant. If you want to get a more elegant solution in, the creation of tests for this is problem needed #1514182-33: IE @font-face CSS hack URL broken by CDN module's overridden CSS aggregation.

mikeytown2’s picture

Issue summary: View changes

Updated issue summary.

paranojik’s picture

Issue summary: View changes
Status: Closed (works as designed) » Needs review
FileSize
2.43 KB

A similar approach to the one that landed in CDN (#1514182: IE @font-face CSS hack URL broken by CDN module's overridden CSS aggregation) is needed here otherwise some of the files referenced in the aggregated CSS won't be properly rewriten (and thus not served from a CDN).
I know that there is hook_advagg_get_css_file_contents_alter(), but using that doubles the workload.

mikeytown2’s picture

Just a quick question, with the current way I have it working, does it output incorrect urls? If yes, can I get an example so I can add it to the tests I now have

paranojik’s picture

If I tested correctly, yes, because the query part of the URL is not stripped away before passing it to file_create_url().

mikeytown2’s picture

Can I get an example that fails in advagg currently?

Here are the tests I currently have
http://cgit.drupalcode.org/advagg/tree/tests/css_test_files/advagg.css

Also I would pull advagg_install_glue_url() out of advagg.install, slightly modify it (for the query case) and use it instead of having all the logic in lined, like it is in the current patch.

The code in advagg is based off of #35 in #1514182-35: IE @font-face CSS hack URL broken by CDN module's overridden CSS aggregation

paranojik’s picture

One of my cases was 'sites/all/themes/omega/omega/images/misc/menu-leaf.png?1382488163', but I don't think you'll be able to catch that with advagg tests, because this only appears when you use 'advagg' in combination with 'cdn'.

I'll modify the patch as you suggested.

mikeytown2’s picture

Ah I see what you're getting at. Does CDN removes everything after the question mark even with the CDN patch due to the way cdn_file_url_alter() works or does it encode it due to drupal_encode_path() getting called? Trying to find the root cause as to why rawurldecode() isn't doing the job.

Only happens when farfuture is used correct?

Looking into this and it seems like the _cdn_devel_page_stats would get the wrong info but the actual source would be correct in the final file due to rawurldecode().

So if I'm understanding this correctly the issue is with the stats being displayed incorrectly?

mikeytown2’s picture

Category: Bug report » Task

The CDN patch has still not been committed. I'm going to wait until it's been committed over there, unless you can show me that the current output of advagg is bad.

Using an har file would be good way to show me why the way advagg currently works is bad & how the patch fixes it.
http://devtoolsecrets.com/secret/performance-export-the-network-timeline...
http://stackoverflow.com/questions/7521942/export-data-from-chrome-devel...

har viewer
http://www.softwareishard.com/har/viewer/

paranojik’s picture

FileSize
4.63 KB
3.62 KB

What I observe is pretty simple. Images referenced from CSS files (e.g. list-style-image: url('../../../images/misc/menu-expanded.png?1382488163');) are not rewritten by CDN. So the file is fetched from http://example.com/sites/all/themes/omega/omega/images/misc/menu-expanded.png?1382488163 instead of
http://cloudfront.net/cdn/farfuture/oIMJFFhofK5GgqqI2SqKmwgzLP9ZcUIiuG6IJTQkm3I/mtime:1423226892/sites/all/themes/omega/omega/images/misc/menu-expanded.png?1382488163.

Does that make sense?

Status: Needs review » Needs work

The last submitted patch, 19: advagg-2112067-urldecode-for-css-19.patch, failed testing.

mikeytown2’s picture

Status: Needs work » Fixed
FileSize
4.25 KB

I've committed this patch. Thanks for the work in here.

I'm still having a hard time accepting your claim though because before and after the patch they both go though this code path where cdn_file_url_alter gets called.
advagg_file_create_url($path) -> file_create_url($uri) -> drupal_alter('file_url', $uri) -> cdn_file_url_alter($original_uri)

If this does indeed fix it then that means the CDN module is not altering the URL if it contains a query or a fragment; which seems like a bug in the CDN code.

  • mikeytown2 committed 8d12163 on 7.x-2.x authored by paranojik
    Issue #2112067 by paranojik, mikeytown2: Paths embedded in CSS are...

Status: Fixed » Closed (fixed)

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