There's a TODO in the documentation to cache the requests, which is really critical IMO. I've created a patch that allows you to pass in a unique $cid, desired expiration time, and optionally to force a refresh. The cache is updated only for successful requests so you should be able to fall back to the previous request if it fails. By letting the end user provide a $cid they can choose meaningful names that they can re-use if necessary to give them more control of the caching process and to avoid unnecessary requests.

CommentFileSizeAuthor
#4 google-analytics-api-679008-4.patch7.66 KBjonskulski
cache.patch3.75 KBkarens

Comments

karens’s picture

karens commented

Oh, and I changed drupal_message() to watchdog() so you'll have meaningful information if this is running in cron.

I just realized that making the $cid required will break the reports module. I suppose we could make it optional and auto generate some sort of random value if it is missing. But it would be better for the reports module to do something like provide a unique cid for each node and then re-use it so you're not re-requesting the same node's stats more than every 24 hours or so.

jonskulski’s picture

jonskulski commented

Testing, subscribing, etc.

@KarenS I agree that the reports module should be responsible for the cid. No need to try to build some sort of unique auto generator.

jonskulski’s picture

jonskulski commented
Status: Needs review » Needs work

We've been using this for a couple weeks haven't had any problems. Always refreshed. I think it's solid.

As a side note, a good method to create a per-request cache id could be something like

array_walk_recursive($request, create_function('&$v, $k, &$t', '$t[] = $v;'), &$flattened);
$cid = md5($flattened);

Then identical requests (any reqeust, not just node requests) would get cached with the same cid automatically and the calling function wouldn't have to worry about caching at all. This goes against my first point but I think it would be reliable enough.

I can reroll unless there are objections.

jonskulski’s picture

jonskulski commented
StatusFileSize
new google-analytics-api-679008-4.patch7.66 KB

Rerolled!

Changes:

  • set $cid to optional. if its not present, we'll generate one by flattening the request array and md5 hashing it.
  • swapped out the array_walk_recursive method for some PHP standard library loving. The array_walk method will through errorsbecause we I was erroneously using call-time pass by reference ;(
  • fixed an issue with the caching conditional. setting a variable ($cache = cache_get('..')) usually is true and thus is a false indicator of cache-ness.

Happy caching

jonskulski’s picture

jonskulski commented
Status: Needs work » Needs review

Setting to review.

omerida’s picture

omerida commented

Instead of using array_walk_recursive, would it be faster to use serialize?


$cid = md5(serialize($request));

jonskulski’s picture

jonskulski commented

I like omerida's idea though I'm not sure on speed. It certainly is cleaner and sufficient.

raspberryman’s picture

raspberryman commented
Assigned: Unassigned » raspberryman
Status: Needs review » Closed (fixed)

Fixed in a new API class - thanks all for your insight! http://drupalcode.org/project/google_analytics_api.git/commitdiff/d50f5e8

Note that we don't have the reports side working with this new code, just the API side. Coming soon :)