The new module at https://www.drupal.org/project/cacheable_cookie_handling provides a framework for handling cookies similar to the cookie this module sets.

It would be useful to integrate with that, since it results in fewer Ajax requests on sites that also use that module for other purposes (one consolidated Ajax request to set a variety of cookies, rather than the Cacheable CSRF module doing its own custom Ajax request for the same purpose).

It's possible to integrate with that module without requiring it, however making it a strict dependency simplifies the code, and also automatically fixes #2786659: Prevent multiple Ajax requests from firing at once. So for now this patch makes it a strict requirement.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein created an issue. See original summary.

David_Rothstein’s picture

Status: Active » Needs review
FileSize
3.41 KB

Here is a patch.

David_Rothstein’s picture

-function cacheable_csrf_menu() {
-  $items['cacheable-csrf'] = array(
-    'type' => MENU_CALLBACK,
-    'page callback' => 'cacheable_csrf_set_token',
-    'access arguments' => array('access content'),
-  );

With this patch we aren't checking "access content" anymore. We could do it manually in the new hook (before setting the cookie) but I didn't see any obvious reason this permission check was needed in the first place? (Seems like it should be theoretically possible to deliberately use this module with users who can't access content, if you want to...)

-  drupal_add_http_header('Cache-control', 'no-cache, no-store');
-
-  // Support Akamai Edge-control headers.
-  drupal_add_http_header('Edge-control', 'bypass-cache');

This is removed in the patch (and is not something that the Cacheable Cookie Handling module does in its Ajax callback). However since in practice the Ajax callback is called via POST, it shouldn't matter?

If it's needed after all for some reason, we can do a separate patch for Cacheable Cookie Handling that adds these headers there.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - the integration looks good to me.

marco’s picture

marco’s picture

FileSize
3.53 KB

re-rerolled.

hadsie’s picture

FileSize
3.41 KB

reroll of Marco's patch attached.

  • douggreen committed 9f5db27 on 7.x-1.x authored by hadsie
    Issue #2789019: Add a dependency on the Cacheable Cookie Handling module
    
douggreen’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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