Hi

I just updated Google Analytics module to version 3.0
It uses the new Asynchronous tracking from Google.

Google Analytics for Ubercart module is set-up for the non-Asynchronous tracking.

I did a quick check of the code, and just by visual inspection, I created this patch that could work with version 3 (Asynchronous).

Haven't tested.

If it works (testers are welcome) I reckon we could make the module smart enough to check for Google Analytics version and print the code accordingly.

Suggestions are welcome.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jm.federico’s picture

hass’s picture

I guess the first line must be wrong. Are the $args drupal_to_js()'d?

jm.federico’s picture

When you talk bout the first line, you mean:

$script = 'var _gaq = _gaq || [];';

I change that one to follow the code provided by Google (http://code.google.com/apis/analytics/docs/tracking/asyncUsageGuide.html). It makes sure have we have an array. If you are not talking about that one, not sure what you mean.

About the $args being drupal_to_js()'d, well, this issue is just to make the module is compatible with the new google_analytics Asynchronous code. That could be a security problem and should have its own issue.

Cheers

hass’s picture

Status: Needs review » Needs work

This line will be set by the GA module. Ubercard only appends it's own code. Keep it as $script = ''; and always make sure to run the arg values through drupal_to_js() if not already done somewhere else in the code or there is a code injection security bug!

jm.federico’s picture

New patch.
First line removed, yeap, not needed, we go after GA module, "_gaq" exists already.

$arg have been drupal_to_js()'d, nothing to change there.

jm.federico’s picture

Status: Needs work » Needs review
longwave’s picture

Status: Needs review » Needs work

Code in #5 looks good but marking "needs work" as we shouldn't break sites that are still on GA 2.x. Not sure if drupal_get_installed_schema_version() is the best way to detect whether GA 2.x or 3.x is installed, or if there's another way.

j0rd’s picture

Looking to upgrade myself. no you shouldn't break for 2.x users. I'd move ahead with installed_schema_version() and create a patch.

Should be good enough, and if we find problems we can fix them later. Key would be to create a patch, so people like me could test.

Cheers,
Jordan

--
Ubercart SEO

longwave’s picture

Status: Needs work » Needs review
FileSize
1.91 KB

Patch attached that doesn't bother attempting version checking, instead it includes both versions of the JavaScript in a way that shouldn't cause errors no matter which version is installed. The output looks okay but this should be fully tested with both GA 2.x and 3.x before it is committed.

longwave’s picture

Updated patch attached that should confuse GA less if 2.x is installed. This still could cause problems in GA 2.x, as I don't really understand how ga.js watches the _gaq object and what it does with it (if anything) in traditional mode.

hass’s picture

Don't combine more than one GA API version and do not try to hack it, please. The patch in #5 looked good to me.

NOTE: I do not plan to support GA 2.x one day longer than Ubercart have released a new version with the async API!

longwave’s picture

I am not trying to hack anything; I don't see why it shouldn't be possible to support both versions of GA in a single piece of JavaScript.

The problem I see is that some users only upgrade modules when security issues are announced ("if it's not broke, don't fix it"), so if there is a future Ubercart security release that includes this update, it will break compatibility as they won't upgrade GA to match. Then again, perhaps a note that you should upgrade to GA 3.x should be included in the release notes - I guess this is up to Island Usurper/TR to decide.

Could you not just define('GOOGLE_ANALYTICS_VERSION', '3.x') in your module - Ubercart then doesn't even need a hook, just something like:

 if (defined('GOOGLE_ANALYTICS_VERSION') && constant('GOOGLE_ANALYTICS_VERSION') == '3.x' {
  // v3.x code
}
else {
  // v2.x code
} 
hass’s picture

If a module complains in update status "it is no longer supported" - do you really think users will not update? Well - their fault... as the Google API has also changed and than it's possible that tracking is going to be broken, too as some things in the API has been deprecated by Google. I will monitor the statistics and have added a clear statement to the release notes in GA 2.3 that users should move forward to 3.x if they are not using Ubercart.

I could add this constant, but why are you not using schema version >= 6300?

TR’s picture

It's Island Usurper's call, but my opinion is uc_googleanalytics shouldn't be in Ubercart core. I would like to see it split off into a standalone module and adopted by someone who's willing and able to give it some love.

I don't think support for GA 2.x should be dropped.

xibun’s picture

subscribing

longwave’s picture

Updated patch attached. drupal_get_installed_schema_version() is an installer function so the patch reads the version directly from the database.

@TR: I don't see why uc_googleanalytics should move out of core. It doesn't really need any love except for this update - it's not particularly complicated, it just works for almost sites, and this is the first change that's been needed in over a year.

torgosPizza’s picture

Just discovered this issue. Thanks for the patch, longwave! Testing it now, will let you know the results in 24 hours :)

EDIT: Yup! Looks like it's working, as I now have ecommerce data in my Analytics report. Awesome! Many thanks.

TR’s picture

Status: Needs review » Fixed

Committed. Thanks.

@longwave: Why move it out of core? Because it just doesn't fit. GA is not core functionality. While you may argue that it's important (and I agree...), there are other important pieces (e.g. Secure Pages) left out that are even more essential. The question becomes where do you draw the line between what's in core and what's contributed. GA was put in there rather arbitrarily in the first place, so were many other features. Why UPS and not FedEx for instance? Why not uc_views? Fact is, since the maintainers aren't big users of GA, it's not going to be pro-actively maintained or extended. It would be much better if someone who did a lot of work with GA would take it over and enhance it as a contributed module. Even if it doesn't require much to maintain it, Ubercart is so big that distributing the load is essential.

IMO, there should be install profiles available that bundle all the bells and whistles, but core itself should only be the essential "can't build a store without it" functionality. That doesn't include GA.

ahimsauzi’s picture

Hey Torgos, will this work only on 6.x-2.x-dev or can be used with 6.x-2.4?

Thanks!

longwave’s picture

The patch will also apply to 6.x-2.4, or if you download the latest 6.x-2.x-dev release it will already be included.

Status: Fixed » Closed (fixed)

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

hanoii’s picture

subscribe

torgosPizza’s picture

Why are you subscribing? This has been fixed in dev.

hanoii’s picture

Sorry, I subscribed because I wanted to follow what have been done here later, but I should have bookmarked it.

keereel’s picture

922230-uc_googleanalytics-2.patch (#16) do not work for me :(

Still nothing in Ecommerce tab in GA. Last transaction was 23 of september (when I update GA module). Help please!

torgosPizza’s picture

It's working for me. Make sure you download the -dev release of the module. If you did, then the patch will of course not work.

j0rd’s picture

@longwave. I don't think it should be moved out of core, but this bug at least needs to be fixed in the stable version of Ubercart.

Aka. Ubercart 6.x needs a new bug fix release. It's been almost a year.

j0rd’s picture

This appears to be committed and is in the latest version now. Thanks a bunches.

netikseo’s picture

Does this patch work for Drupal 7? If not, how to solve this problem on drupal 7?

longwave’s picture

This is not needed in Drupal 7, the code has already been ported to the 7.x version of Google Analytics. Please open a new issue if you are having trouble with GA in Ubercart 7.x-3.x.