2 hooks that could reduce the amount of code used in the cdn module; both located in the advagg_get_css_aggregate_contents function.
hook_advagg_get_css_file_contents_alter - Alter a files content.
- JS version currently implemented in advagg_js_compress module
hook_advagg_get_css_aggregate_contents_alter - Alter an aggregates content.
- Currently implemented in advagg_css_compress module
When using a hook that modifies the content of an aggregate, and said hook can have different output depending on a setting or variable, be sure to add it in via hook_advagg_get_hooks_hash_alter(); example in advagg_css_compress module. Using the advagg_css_compress module as an example, advagg_css_compress_advagg_get_css_aggregate_contents_alter() is controlled by the advagg_css_compress_agg_files and advagg_css_compressor variables thus they are added in via the advagg_css_compress_advagg_get_hooks_hash_alter() function.
Taking a quick peak at the CDN module I would guess these are the variables & settings one would need to add in via hook_advagg_get_hooks_hash_alter().
variable_get(CDN_MODE_VARIABLE, CDN_MODE_BASIC)
cdn_check_protocol()
cdn_check_drupal_path($_GET['q'])
cdn_serve_from_https()
These variables and settings control how _cdn_build_css_path should be used; a change in one of these could change the contents of the aggregate.
And I would implement hook_advagg_get_css_file_contents_alter() running _cdn_build_css_path() over the contents after running various checks and settings things accordingly.
Comment | File | Size | Author |
---|---|---|---|
#19 | cdn-1942230-18-advagg-hooks.patch | 3.53 KB | mikeytown2 |
#14 | interdiff-8-12-1942230.txt | 2.7 KB | mikeytown2 |
#14 | cdn-1942230-12-advagg-hooks.patch | 4.33 KB | mikeytown2 |
#8 | cdn-1942230-8-advagg-hooks.patch | 4.74 KB | mikeytown2 |
#6 | cdn-1942230-6-advagg-hooks.patch | 4.46 KB | mikeytown2 |
Comments
Comment #1
mikeytown2 CreditAttribution: mikeytown2 commentedThe one thing I would need to work on is if the file is not to be preprocessed it still runs though _cdn_build_css_cache. This is something that advagg does not do but it could.
Edit: Looks like I need to rethink hook_advagg_get_hooks_hash_alter as you're using
$_GET['q']
and for 404 generation that info isn't available. Will be working on this more... might end up with another table instead of a variable by the end of this.Comment #2
mikeytown2 CreditAttribution: mikeytown2 commented#1942326: advagg_get_hooks_hash needs to be a table and not use the variables table is done. Semi new explanation on how to make this fly below:
The setting used will be saved into the db and associated with the generated filename. When the advagg file is generated it will pass the array into the hooks that get called. Quick explination from advagg_css_compress module's code. Instead of calling variable_get we access the $aggregate_settings variable for settings.
Comment #3
Wim LeersWow, lots of information, not sure it's entirely clear to me.
1. Are you saying that the CDN module should depend on the AdvAgg module? I don't want to introduce that dependency.
2. Assuming 1, then how can I make sure that the AdvAgg module can take over CSS aggregation from the CDN module when both are installed? Plus, the AdvAgg module will still call
file_create_url()
on files referenced in CSS (background images etc.), right?3. The *only* reason I'm overriding CSS aggregation is because Drupal core does not call
file_create_url()
on files referenced in CSS files. I was thinking that it should be possible to still get that in Drupal, because it would only involve a pretty small change and wouldn't break anything.Comment #4
Peter Bowey CreditAttribution: Peter Bowey commentedmikeytown2 and Wim Leers
It would be great to see a serious integration 'concept' over these two very valid modules.
I have dealt with this same CDN + AdvAgg issue back in the [past] Drupal 6 days.
CDN without the 'help' of AdvAgg has some issues and I solved this via the hooks within AdvAgg.
Wim, worst case you can add a check for AdvAgg 7.x-2.x Integration, and then add the essential code on failing to find the module / hooks avail.
Otherwise, merge the two modules into one unit!
Comment #5
Wim LeersYes, I'm totally fine with adding checks for AdvAgg to the CDN module, I just don't want them to depend on each other :)
Comment #6
mikeytown2 CreditAttribution: mikeytown2 commentedI can't get farfuture working but advagg integration does appear to be working if that option is unchecked.
Looks like I forgot to give _cdn_advagg_file_create_url() proper documentation. Will fix that after initial thoughts on this patch. Documentation for function as of right now
Comment #7
mikeytown2 CreditAttribution: mikeytown2 commentedI just committed a fix for far future inside of AdvAgg
#2046299: Add in support to bypass far future mode in the CDN module
Comment #8
mikeytown2 CreditAttribution: mikeytown2 commentedSame code as #6 but with the documentation included in the patch. I've had many reports of this patch working well.
Comment #9
Wim LeersWhat you said in #7 worries me, mikeytown2. Or rather, the "fix" you committed does this:
First: I presume AdvAgg does generate unique file URLs on its own. If so, then that part is indeed not needed.
Second: the way the CDN module *serves* assets when the Far Future setting is enabled is also crucial for it to be effective. Look at the many headers that are being set in
cdn_basic_farfuture_download()
.AFAICT the first is not necessary if you're using the second.
Why don't you need these in AdvAgg already?
More duplication.
Comment #10
jhinxed CreditAttribution: jhinxed commentedHi, I just tried out this patch and an error popped up. Please see below. Any help would be greatly appreciated.
Comment #11
mikeytown2 CreditAttribution: mikeytown2 commentedAdvAgg does generate unique file URLs on its own and it also sends out far-future style headers. I actually reference the CDN module where AdvAgg sets the headers.
One thing I recently changed was the removal of Last-Modified header. This prevented Apache from sending out a 304. #1431668-4: can't get browser control cache with bundle : always OK 200 reponse
So
cdn_check_protocol
will coverCDN_HTTPS_SUPPORT_VARIABLE
andcdn_serve_from_https
?Comment #12
jhinxed CreditAttribution: jhinxed commentedCan anyone help me with the issue I'm facing on my last post?
Comment #13
mikeytown2 CreditAttribution: mikeytown2 commented@jhinxed
#2078049: Connection timed out. TCP Connect Timeout. code -10060.
Comment #14
mikeytown2 CreditAttribution: mikeytown2 commentedI've reduced the number of variables used in the hashing function with this latest patch
Comment #15
lsolesen CreditAttribution: lsolesen commentedI could apply the patch. However, it seems that background images served in the css is not called correctly when enabling advagg. When disabling all the images display correctly.
Comment #16
thijsvdanker CreditAttribution: thijsvdanker commentedSame problem as Isolesen in #15, background images are not served.
The css contains the cdn url twice:
background-image: url('//random123.cloudfront.net//random123.cloudfront.net/path/to/img/background.png')
Comment #17
mikeytown2 CreditAttribution: mikeytown2 commentedDo you have this patch applied to core?
#1961340-26: CSS aggregation breaks file URL rewriting because it abuses it
I'm going to assume this is the cause of the issue and make sure this patch works with that core patch applied.
Comment #18
mikeytown2 CreditAttribution: mikeytown2 commentedI see the issue; going to reformulate the patch and some code in AdvAgg... will take some time. Thanks for the report :)
Comment #19
mikeytown2 CreditAttribution: mikeytown2 commentedThis patch requires the latest dev of AdvAgg in order to work correctly.
AdvAgg issue: #2103183: Implement #1961340 in AdvAgg
Comment #20
lsolesen CreditAttribution: lsolesen commentedIt seems that this patch works correctly for the background images.
Comment #21
basvredelingIs this still applicable after the latest stable release? (which is more recent than the latest comments)
Comment #22
mikeytown2 CreditAttribution: mikeytown2 commentedPatch is not required (as of October 2nd), but still recommended as AdvAgg needs to set the correct context for cdn_file_url_alter(). Without the patch, the CDN module could end up using the page context of public://advagg_css/ OR httprl_async_function_callback; in reality it should be using the context on the page that the CSS file is included on (for example node/8). The CDN patch in #19 allows for the context to be saved and then reloaded when the aggregate file gets generated.
Code that makes the CDN patch no longer required used the backported D8 code from Wim Leers in this issue #1961340-26: CSS aggregation breaks file URL rewriting because it abuses it. The D8 patch allows for the contents of the CSS url attribute to be passed through file_create_url(). We did encounter an issue with fonts and url encoding, and in AdvAgg that issue has been patched.
Comment #23
basvredelingAh, just found out why the adv_agg patch wasn't working. It wrote the new file to my drupal root, not to the module root. I'll keep patching the module until the D8 issue trickles down, I guess. Thanks for the reply. Otherwise I can confirm this patch works lovely.
Comment #24
cllamas CreditAttribution: cllamas commentedCan anybody tell me what patch do I have to apply for the 7.x-2.1 version?
Comment #25
cllamas CreditAttribution: cllamas commented19: cdn-1942230-18-advagg-hooks.patch queued for re-testing.
Comment #26
mikeytown2 CreditAttribution: mikeytown2 commented@cllamas
#19 is what you want.
Comment #27
almamun CreditAttribution: almamun commentedHow can I apply this patch manually?
Comment #28
alexweber CreditAttribution: alexweber commented@almamun https://drupal.org/node/1399218
Comment #29
RaulMuroc CreditAttribution: RaulMuroc commentedTo me the latest patch in #19 didn't apply. I'm using the latest cdn -dev version.
Comment #30
mikeytown2 CreditAttribution: mikeytown2 commented@RaulMuroc
I just did a fresh git pull and the patch in #19 still applies without any issues.
Comment #31
ikarpouc CreditAttribution: ikarpouc commentedI am using 7.x-2.x-dev version but I can't find the file cdn.advagg.inc to patch it.
Am I missing something?
Thanks in advance
John.
Comment #32
mikeytown2 CreditAttribution: mikeytown2 commentedThe patch creates the cdn.advagg.inc file
Comment #35
webfunkin CreditAttribution: webfunkin commentedThe latest patch does not work with 7.x-2.6 or the current dev release.
Comment #36
mikeytown2 CreditAttribution: mikeytown2 commentedComment #37
mikeytown2 CreditAttribution: mikeytown2 commentedPatch still works! But due to the reported issues that I can not reproduce (#27, #29, #31, & #35) and thus can not fix, I've moved this code into the AdvAgg module. Now with the latest dev version of AdvAgg you will no longer need to patch the CDN module.
Comment #39
alexweber CreditAttribution: alexweber commentedGreat news, don't forget to update the project page! :)
Comment #40
mikeytown2 CreditAttribution: mikeytown2 commented@alexweber
Thanks for the reminder. Will do so with the 7.x-2.8 release.
Comment #42
Wim LeersVery sorry for my long silence — but looks like this got resolved in the mean time by the ever awesome @mikeytown2, yay! :)
Comment #43
tmin CreditAttribution: tmin commentedI think this is related: the issue is indeed fixed (I have manually checked the aggregates and Content-Encoding is gzip) but there is still a warning in admin/reports/status:
The web servers configuration will need to be adjusted. In most cases make sure that the webroots .htaccess file still contains this section "Rules to correctly serve gzip compressed CSS and JS files". Certain default web server configurations (nginx (link is external)) do not gzip HTTP/1.0 requests. If you are using cloudfront you will have to add metadata (link is external) to the .gz files. There are some other options if using cloudfront (link is external).
I am using cdn module and serving the files through cloudfront. When requesting the css aggregate (directly or in a page) that is mentioned in the warning it is served gzipped through cloudfront, so, I'm not sure what's the problem here.
The only "out of the ordinary" thing in my configuration is that cloudfront uses https to serve the files. Is it possible for https to be creating a "false positive" warning in the error logs?
Comment #44
mikeytown2 CreditAttribution: mikeytown2 commented@tmin
"false positive" warnings in the error logs can happen. You should open up a issue over in advagg describing in detail what's going on. gzip could be failing for http 1.0 requests but working for http 1.1. https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_ht... can only use 1.0