Javascript should be attached to page_bottom, not footer (otherwise it won't ever be included by some themes)

I am not positive about the change to $scope, but the rest looks correct...

Changing the title to explain the effect of the bug. In D7, there is no requirement that themes ever have a region called 'footer' or print it in their templates (for example, the core Seven theme does not even have a region with that name). Any theme like that currently will never add this Google Analytics JavaScript to the page, which is bad.

As an aside, all this code is in hook_page_alter()... shouldn't it be in hook_page_build() instead? (It basically just seems to be adding to the page, not altering it.)

Something must be wrong here. At least the new default scope is header. If i remember correctly my previous issue was #812606: Cannot add inline JS code to regions via hook_page_alter(). It was very confusing to me, too. Have something of this changed since My case has been closed?

My D7 theme do not have a $closure variable and only have the new $page_bottom, but the code is attached to the $page_bottom nevertheless the code says "footer". The keywords "footer" is used in drupal_add_js() as scope and this must be the same. I complained in the linked case about how bad this inconsitent/confusing api is and this is now 6 months ago and nobody cared about... as attaching of code to the $page_bottom has worked with 'footer' I have not changed anything. I still wish I could attach code to all regions of a page, but header and footer are not regions... confusion squared.

I'm still open to optimize the code and remove all #attach sh** or use hook_page_build() if this may automatically solve #231451: Add hook to alter data before sending it to browser, but I have no clue if this is possible as I'm stil confused about this core changes and hope to see some more code examples from other solutions, but at the time of writing there was nothing. Maybe today...

Is this D7 grap confusing or is it confusing? :-)

Yeah, those docs in core are flat-out wrong, basically. There should be an issue filed to fix them.

And in this case, drupal_add_js() definitely makes plenty of sense; #attached is only really useful for JavaScript that actually has something to do with a particular element on the page, which Google Analytics doesn't.

+/**
+ * Delete the variable for scope since it should be page_bottom, not header.
+ */

Should say 'footer', not 'page_bottom'?

     // Custom tracking. Prepend before all other JavaScript.
-    if (variable_get('googleanalytics_trackadsense', FALSE)) {
-      $page['footer']['googleanalytics']['#attached']['js']['window.google_analytics_uacct = ' . drupal_json_encode($id) . ';'] = array(
-        'type' => 'inline',
-        'scope' => 'header',
-        'weight' => JS_LIBRARY - 21,
-      );
-    }
-
+    drupal_add_js('window.google_analytics_uacct = ' . drupal_json_encode($id) . ';', array('type' => 'inline', 'weight' => JS_LIBRARY - 21));

Did you deliberately change the use of the 'googleanalytics_trackadsense' variable?

(By the way, all these uses of 'weight' are incorrect; it should actually now be 'group' => JS_LIBRARY, 'weight' => -21 I believe.)

-      $page['footer']['googleanalytics']['#attached']['js'][] = array(
+      $page['#attached']['js']['ga_link_tracking'] = array(
         'type' => 'setting',
         'data' => array('googleanalytics' => $link_settings),
       );

Why not use drupal_add_js() here too?

Also, no $scope? To clarify, this patch overall makes the $scope variable only relevant for the tracking code itself, not for any of the other JavaScript that is added. That may be the right behavior, but it confused me at first - maybe a code comment as to why?

Also putting back to 'major'. This bug makes the Google Analytics module not work at all, on many themes.

Without having a proper knowledge of scopes - how can you write a patch? I guess you are only reviewing code without any real life test and you may be a victim of #844810: Statistics in Google Analytics wrong, way to low after changing to async mode (head section).

Read the linked case about my #attach try outs, please!

1. I cannot speak for D7 core beta1, but as I know - the module worked correct when it was implemented and months afterwards, too.
2. drupal_add_js() hasn't worked in hook_page_alter() and #attach was the ONLY way to attach the JS stuff in past.
3. There was also documentation that hook_footer should be upgraded to hook_page_alter().
4. 'weight' => JS_LIBRARY - 21, was also correct at the time of writing, but may have changed and the 'group' is newer.
5. I have no clue what googleanalytics_update_7002() should do here and why we need this.
6. We are NOT attaching ASYNC GA JS by default to the footer. I have no clue why you are changing header to footer! Read Google docs first - before posting any patch, please.
7. Nevertheless there may be 'footer' used it does NOT mean there need to be the previous $closure variable nor a region. Suxxxx documentation may tell you something wrong.

I also said WTF, but it was the only way how I was able to get the module working and Gabor was the only guy who have commented with an helpful tip in the linked case...

From my point of view I'm NOT aware about any issues with the current code nor any incompatibility with any theme that do not have a footer region. I may be wrong, but than something must have been changed within the last days/very few weeks. I will do some testing with these ugly seven theme myself.

Your code changes have for 1000% not worked in previous D7 version. Always keep this in mind. I cannot change the code every day and verify if someone have broken something again. This is also the reason why there will be no beta. #attached was the ONLY way to attach the JS code to whatever region/variable you'd like and it still works. As page_bottom is not a "region" something more was not working - but I cannot remember. The linked case is more or less a log of my try outs.

It's not really important to delete the scope variable as a simple settings form post will re-create them again. So, pretty useless on the end of the day and only an upgrade hook that is not required.

WRONG: Outdated - http://www.google.com/support/analytics/bin/answer.py?hl=en&answer=55488

Note: This article is for the traditional version of the tracking code. We recommend you update your tracking code to use the latest (asynchronous) version. For instructions on using the latest version, see this article.

CORRECT: ASYNC mode - http://www.google.com/support/analytics/bin/answer.py?answer=174090

Copy and place the code snippet
Once you find the code snippet, copy and paste it into your web page, just before the closing </head> tag*. If your website uses templates to generate pages, enter it just before the closing tag in the file that contains the <head> section. (Most websites re-use one file for common content, so it's likely that you won't have to place the code snippet on every single page of your website.)
For the best performance across all browsers we suggest you position other scripts in your site in one of these ways:
before the tracking code snippet in the <head> section of your HTML
after both the tracking code snippet and all page content (e.g. at the bottom of the HTML body)

In rare cases we'd like to add JS code to the footer and in most cases to the head as Google suggests.

The module need to add the JS code in on of the very latest hooks we have and hook_page_alter is a place that is known to work. You cannot track 403/404 status code if you use hook_init() as the status codes are not yet set by core or modules. Maybe something has changed here in D7, but need to be verified again. Using hook_init() in past and hook_footer() was only for one reason - it wasn't possible to add JS code from hook_footer to the head section. I was told by Gabor it is tooo late. But great D7 have a fix - nevertheless we need to run the code in a very late stage.

I tried drupal_add_js($inline_code, array('type' => 'inline', 'scope' => 'page_top')) in past, but this strange #attach API was inconsistent and not easy to understand. The reason was there is also inconsistent Google documenation that says you should add Async code after the opening BODY tag and 'page_top' is the place we may need to go. Later I found they also recommend adding async code to HEAD, but we still have the #844810: Statistics in Google Analytics wrong, way to low after changing to async mode (head section) report. I wish we would also be able to asign the code to 'page_top'.

'group' => JS_LIBRARY, 'weight' => -21 seems a correct update after reading the API docs.

I hated the #attach stuff from the first day. Reverting to drupal_add_js() get's all my commitment.

I also don't know why you have moved drupal_add_js(drupal_get_path('module', 'googleanalytics') .'/googleanalytics.js'); out of the IF. This is also wrong.

As per http://drupal.org/node/812606#comment-3455596, from effulgentsia

#attached can be used instead of drupal_add_js() if the call to drupal_add_js() needs to be conditional on something being rendered. For example, if you only want to have drupal_add_js() called if a particular region/block/node/whatever is rendered, then the right way to do that is to add #attached to that element. Probably GoogleAnalytics code doesn't fall into this category. In any case, #attached is simply a wrapper to drupal_add_*() that only runs when the element is being rendered.

Fixed patch attached.

May need to be roled back as per #929096: Test that JS files added in hook_page_alter()/hook_page_build() are aggregated

I commented on #929096: Test that JS files added in hook_page_alter()/hook_page_build() are aggregated what I think is going on there. In short, I think there is no bug :)

By the way, @hass, thanks for rolling/committing the final version of this patch! The code in the latest patch here looks right, and we've been using this for a while and haven't run into any problems. (I can confirm the files are getting aggregated on the sites we are using this on, also.)

Ok, as this may have been an alpha7 bug, let's close.