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.
Comment | File | Size | Author |
---|---|---|---|
#21 | advagg-2112067-21-urldecode-for-css.patch | 4.25 KB | mikeytown2 |
#19 | interdiff-12-19.txt | 3.62 KB | paranojik |
#19 | advagg-2112067-urldecode-for-css-19.patch | 4.63 KB | paranojik |
#6 | advagg-2112067-6-prevent-url-encode-decode.patch | 1.08 KB | pjcdawkins |
#4 | advagg-2112067-4-urldecode-for-css.patch | 919 bytes | mikeytown2 |
Comments
Comment #1
mikeytown2 CreditAttribution: mikeytown2 commentedOff the top of my head I think it might have something to do with this patch #2103183: Implement #1961340 in AdvAgg
Comment #2
mikeytown2 CreditAttribution: mikeytown2 commentedyeah
#1514182-35: IE @font-face CSS hack URL broken by CDN module's overridden CSS aggregation
Comment #3
pjcdawkins CreditAttribution: pjcdawkins commentedOK, 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?)
Comment #4
mikeytown2 CreditAttribution: mikeytown2 commentedFollowing patch has been committed. Let me know if this doesn't fix the issue.
Comment #5
pjcdawkins CreditAttribution: pjcdawkins commentedThat 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?)
Comment #6
pjcdawkins CreditAttribution: pjcdawkins commentedHere's a patch against current -dev (after the patch in #4) which prevents the whole URL encode / decode chain.
Comment #7
pjcdawkins CreditAttribution: pjcdawkins commentedUpdating title now that #4 is in.
Comment #8
mikeytown2 CreditAttribution: mikeytown2 commentedThat 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.
Comment #9
mikeytown2 CreditAttribution: mikeytown2 commentedActually since the title changed, I'll move this to works as designed.
Comment #10
pjcdawkins CreditAttribution: pjcdawkins commentedI 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.
Comment #11
mikeytown2 CreditAttribution: mikeytown2 commentedReason 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.
Comment #11.0
mikeytown2 CreditAttribution: mikeytown2 commentedUpdated issue summary.
Comment #12
paranojik CreditAttribution: paranojik at Cando commentedA 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.
Comment #13
mikeytown2 CreditAttribution: mikeytown2 commentedJust 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
Comment #14
paranojik CreditAttribution: paranojik at Cando commentedIf I tested correctly, yes, because the query part of the URL is not stripped away before passing it to file_create_url().
Comment #15
mikeytown2 CreditAttribution: mikeytown2 commentedCan 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
Comment #16
paranojik CreditAttribution: paranojik at Cando commentedOne 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.
Comment #17
mikeytown2 CreditAttribution: mikeytown2 commentedAh 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?
Comment #18
mikeytown2 CreditAttribution: mikeytown2 commentedThe 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/
Comment #19
paranojik CreditAttribution: paranojik at Cando commentedWhat 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 fromhttp://example.com/sites/all/themes/omega/omega/images/misc/menu-expanded.png?1382488163
instead ofhttp://cloudfront.net/cdn/farfuture/oIMJFFhofK5GgqqI2SqKmwgzLP9ZcUIiuG6IJTQkm3I/mtime:1423226892/sites/all/themes/omega/omega/images/misc/menu-expanded.png?1382488163
.Does that make sense?
Comment #21
mikeytown2 CreditAttribution: mikeytown2 commentedI'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.