I am using this module to convert the ajax post requests to ajax get request so that they can be cache in varnish. The conversion is very smooth but those urls are not cached by varnish.

After a bit debugging I found that varnish uses the information provided in headers, and for those get request we have request header set "Cache-Control: no-cache", so they are invalidated by varnish.

I am trying to resolve this and hopefully will come up with a solution.

CommentFileSizeAuthor
#14 views_ajax_get-fix_drupal_page_is_cacheable-2356133-9249385.patch757 bytesleon kessler
#13 views_ajax_get-fix_drupal_page_is_cacheable-2356133-9249385.patch757 bytesleon kessler
#8 views_ajax_get_add_cache_header_2356133_8.patch839 bytesgaurav.goyal
#6 views_ajax_get_add_cache_header_2356133_6.patch844 bytesgaurav.goyal
#1 views_ajax_get_add_cache_header_2356133_1.patch1.01 KBgaurav.goyal

Comments

gaurav.goyal’s picture

gaurav.goyal commented
StatusFileSize
new views_ajax_get_add_cache_header_2356133_1.patch1.01 KB

Patch attached.

Used the below code for adding headers and it works for me..

function views_ajax_get_views_ajax_data_alter(&$commands, $view) {
  drupal_add_http_header('Cache-Control', 'public, max-age=' . variable_get('cache_lifetime', 300));
}

Thanks for the module, a good one..:)

gaurav.goyal’s picture

gaurav.goyal commented
Status: Active » Needs review
gaurav.goyal’s picture

gaurav.goyal commented
Category: Bug report » Support request
Priority: Critical » Normal

Changing the tags, as i realized this module does not provide this feature.

leon kessler’s picture

leon kessler commented
Category: Support request » Feature request
Status: Needs review » Needs work

Hi Gaurav,

Thanks for reporting this. At first I didn't think this module should be responsible for cache-control headers. But thinking about it more, I don't see why not. Anyone using this module is only doing so to help with caching, so makes sense to have this as part of the module.

A few comments on your patch though:

  1. +++ b/includes/views/views_ajax_get.views.inc
@@ -0,0 +1,12 @@
+/**
+ * Implementation of hook_views_ajax_data_alter().
+ */
+function views_ajax_get_views_ajax_data_alter(&$commands, $view) {
+  drupal_add_http_header('Cache-Control', 'public, max-age=' . variable_get('cache_lifetime', 300));
+}

    1) This should be moved to views_ajax_get.module
    2) Let's add a line in the docblock comment explaining what the function is doing.
    3) We need to check that the Views isn't excluded in variable_get('views_ajax_get_exemptions'). We don't wanna send cache-control headers when it's a POST request.
    4) We should also add in drupal_page_is_cacheable(TRUE); to let other modules know the request is cacheable. 

  2. +++ b/views_ajax_get.module
@@ -40,3 +40,13 @@ function views_ajax_get_preprocess_views_view(&$vars) {
+
+/**
+ * Implementation of hook_views_api().
+ */
+function views_ajax_get_views_api() {
+  return array(
+    'api' => 3,
+    'path' => drupal_get_path('module', 'views_ajax_get') . '/includes/views',
+  );
+}

    hook_views_api isn't required, we're only implementing an alter hook.

gaurav.goyal’s picture

gaurav.goyal commented

Hi leon.nk,

Thanks for the detailed feedback.

Yes I completely agree with you, this should be a part of this module.

gaurav.goyal’s picture

gaurav.goyal commented
Status: Needs work » Needs review
StatusFileSize
new views_ajax_get_add_cache_header_2356133_6.patch844 bytes

Hi leon,

Attached is the updated patch.

leon kessler’s picture

leon kessler commented
Status: Needs review » Needs work

Okay great!

Looks good, sorry couple more nitpicks :-)

  1. +++ b/views_ajax_get.module
@@ -40,3 +40,17 @@ function views_ajax_get_preprocess_views_view(&$vars) {
+/**
+ * Adding the headers for cache control so that these requests can be cached.
+ *
+ * Implementation of hook_views_ajax_data_alter().
+ */

    Should be Implements hook_views_ajax_data_alter(). As per https://www.drupal.org/coding-standards/docs#hookimpl

    Also, "Implements hook_views_ajax_data_alter()." should appear first 

  2. +++ b/views_ajax_get.module
@@ -40,3 +40,17 @@ function views_ajax_get_preprocess_views_view(&$vars) {
+    drupal_add_http_header('Cache-Control', 'public, max-age=' . variable_get('cache_lifetime', 300));

    Let's change variable_get('cache_lifetime', 300) to variable_get('cache_lifetime', 0)
    To mimic core's default value for this variable.

gaurav.goyal’s picture

gaurav.goyal commented
Status: Needs work » Needs review
StatusFileSize
new views_ajax_get_add_cache_header_2356133_8.patch839 bytes

Hi leon,

Ahh..my bad..thanks for pointing out..:)

Patch is modified, i think now it looks more cleaner...:)

  • leon.nk committed 24db988 on 7.x-1.x authored by gaurav.goyal 
    Issue #2356133 by gaurav.goyal - add cache-control headers to GET...
leon kessler’s picture

leon kessler commented
Status: Needs review » Fixed

Great thanks a lot! Patch committed to dev.

  • leon.nk committed ff2b92a on 7.x-1.x 
    Issue #2356133 by gaurav.goyal - check if page is cacheable before...
leon kessler’s picture

leon kessler commented

Corrected something in the commit. The use of drupal_page_is_cacheable() was incorrect. We should be checking that caching is enabled before setting the headers (and not setting it explicitly). Otherwise we will be caching responses for logged in users, which could lead to unexpected results.

leon kessler’s picture

leon kessler commented
StatusFileSize
new views_ajax_get-fix_drupal_page_is_cacheable-2356133-9249385.patch757 bytes
leon kessler’s picture

leon kessler commented
StatusFileSize
new views_ajax_get-fix_drupal_page_is_cacheable-2356133-9249385.patch757 bytes

Patch was wrong way round!

Status: Fixed » Closed (fixed)

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