The main focus of this patch, is to make the implementation of mixpanel_track() more Drupal-y - but it also fixes some small bugs in mixpanel_get_defaults(). If necessary, I can break that out into it's own patch - just let me know!
In any case, here are the main un-Drupal-y things in the current implementation of mixpanel_track() which my patch fixes:
- It calls "curl" on the command line. This won't work on Windows machines or UNIX-like ones without "curl". However, the standard Drupal way of making an HTTP request is drupal_http_request(), which the patch uses.
- It depends on the "&" shell operator to contact the Mixpanel API outside of the current request. Again, this will fail under Windows. In Drupal, the standard way to do this is on cron and in Drupal 7 this means the Queue API. My patch will use the drupal_queue module to defer API calls to cron if it's installed. The drupal_queue module is a backport of the Drupal 7 Queue API, so when we port mixpanel to Drupal 7, the same code will work without needing any module.
- It directly gets $_SERVER['REMOTE_ADDR'] rather than using ip_address(). This also means it won't work behind a proxy.
- There is no logging in the case of a failed call to the API. My patch will log failed requests to watchdog().
Additionally it makes the following small fixes to mixpanel_get_defaults():
- It makes the static cache dependent on the $account passed in. The current implement will return the first defaults generated even if subsequent calls to mixpanel_track() pass in a different $account.
- 'mp_tag_name' was being set to $user->name rather than $account->name. So, it would use the current user regardless of what $account variable was passed in.
- It also sets the 'time' property (one of the magic mixpanel ones) so that the deferred calls sometimes made (per the above points) will have the appropriate time.
Please let me know what you think!
Best regards,
David.
Comments
Comment #1
dsnopekAttached is an updated version of this patch. I made a pretty serious think-o setting the time property in mixpanel_get_defaults(). This caused it to be cached for the length of the request. This update fixes that!
Comment #2
dsnopekHere is an updated version of this patch against the latest git 6.x branch. The changes are really minor but it will make any review a little easier.
Comment #3
dsnopekCommited to git!