Enabling google Analytics 6x-1x-dev (2007-Dec-31) breaks lightbox V2. Anyone else using this combination?

Lightbox modal window will flash quickly on screen then disappear and a plain page is displayed. Disabling this module restores correct lightbox functionality.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hass’s picture

Status: Active » Postponed (maintainer needs more info)

Could you please try the latest lightbox version. I think this could be caused by http://drupal.org/node/200731. I've never heard about lightbox, nor i'm using it. If this is not fixed, please give us the errors you get in firebug or so.

kylehase’s picture

I'm already using the latest stable release of Lightbox V2 6.x-1.3.
Also, according to the lightbox page

Lightbox2 5.x-2.0 and later versions now all use the jQuery library instead of the Scriptaculous and Prototype libraries

So it's probably not the same bug. I'll get you those firebug logs.

hass’s picture

Title: breaks lightbox module » Broken with google_analytics
Project: Google Analytics » Lightbox2
Category: support » bug
Status: Postponed (maintainer needs more info) » Active

I tried to repro this bug myself. Installed Lightbox 6.x-1.3, added 4 example images to a page as written in the readme.txt of lightbox2. However lightbox have a version of 1.3 it looks buggy and not finished work to me. I leave this to someone else to waste his time, but i'm documenting now what i found before i stopped my testing.

Standard install (no light version):
IE6: If i click on the link, everything looks ok, popup comes up, background opacity works, close works, etc
FF: Popup comes up, loading.gif is displayed shortly, and a second later the page shows only the image. Need to press browsers back button to get back to the page.

Changed to lite version:
IE6: Popup works, click on the top X gives a JS error. "press x to close" in the bottom closes without an JS error.
FF: same as before

hass’s picture

Title: Broken with google_analytics » Broken module
stella’s picture

Title: Broken module » google_analytics conflict with lightbox2 module
Project: Lightbox2 » Google Analytics

Hi,

I've enabled the google analytics 6.x-1.x-dev module on my D6 test site. I get the following error in the Firefox error console with the Lightbox2 module disabled:

Error: Drupal is not defined
Source File: http://d6.travel-with-me.eu/sites/all/modules/google_analytics/downloadtracker.js
Line: 39

When I enable the Lightbox2 module, I get the following error:

Error: Drupal.settings.googleanalytics has no properties
Source File: http://d6.travel-with-me.eu/sites/all/modules/google_analytics/downloadtracker.js
Line: 4

There is a difference between the two errors because the Lightbox2 module defines Drupal.settings.lightbox2. I do see the following line in the google_analytics module but am unsure why it's not loading:

drupal_add_js(array('googleanalytics' => array('trackDownload' => $trackfiles, 'LegacyVersion' => $legacy_version)), 'setting', 'header');

When I enable the lightbox2 module, I correctly see the Drupal.settings.lightbox2 settings in the generated source code (see below), but no Drupal.settings.googleanalytics. I still see no Drupal.settings.googleanalytics when the lightbox2 module is disabled.

<script type="text/javascript">jQuery.extend(Drupal.settings, { "lightbox2": { "rtl": "0", "display_image_size": "", "overlay_opacity": "0.6", "use_alt_layout": 0, "disable_zoom": 0, "force_show_nav": 0, "image_node_classes": "img.inline,img.flickr-photo-img,img.thumbnail, img.image-thumbnail", "group_images": 1, "disable_for_gallery_lists": 0, "disable_for_acidfree_gallery_lists": true, "node_link_text": "", "image_count": "Image !current of !total", "lite_press_x_close": "press \x3ca href=\"#\" onclick=\"hideLightbox(); return false;\"\x3e\x3ckbd\x3ex\x3c/kbd\x3e\x3c/a\x3e to close" } });</script>

Cheers,
Stella

hass’s picture

1. While you tested this with GA only, was the page fully loaded?
2. What D6 DEV version are you using?

Sounds like a drupal_add_js bug if not both variables are added. The not defined "Drupal" sounds like the page is not fully loaded.

stella’s picture

I'm using the latest 6.x-1.x-dev versions for both modules - downloaded this morning.

I'm using the default GA settings, except for the UA id (which I set to my own id) and the roles (enabled it for admin=1 and authenticated users). I didn't modify any of the other settings.

The page was fully loaded in both scenarios and the other GA javascript (e.g. downloadtracker.js) was loaded in the footer. I double checked now to be sure and also noticed that in the GA-only enabled scenario, neither misc/jquery.js or misc/drupal.js were loaded, which is kinda strange - I tried this with the legacy option disabled and enabled, just to see if it made any difference.

If you need any more info, please let me know.

Cheers,
Stella

hass’s picture

Yep, latest module is ok... but what Drupal version are you using (build date)? If not latest, please upgrade first.

I remember there was a bug long time ago... i thought it is fixed... i remember the misc/jquery.js or misc/drupal.js are missing on my site, too. Without this files linked you get the above errors for sure. But happens many weeks if not months ago...

What theme are you using? Garland should be ok... but if you theme is missing the $script variable you get this results, too?

hass’s picture

Status: Active » Postponed (maintainer needs more info)
stella’s picture

Status: Postponed (maintainer needs more info) » Active

I was using a build from the 10th Dec. This morning I upgraded to release candidate 4 but the problem still occurs. I am using the default garland theme - the $scripts variable is not missing, which is proven by the fact that these files are present when the lightbox2 module is enabled. The downloadtracker.js file is also loaded.

Cheers,
Stella

stella’s picture

I've done a bit more investigating. I think the problem is that no javascript can be added to the header in an implementation of hook_footer(). The header javascript files have already been assigned to the output variable (in template_preprocess_page()) by the time hook_footer() functions are invoked in theme_closure(). I tested by changing the scope of the downloadtracker.js file from 'footer' to 'header'. When set to 'footer', the script correctly appeared in the footer area of the page. However, when I changed the scope to be 'header', it no longer appeared any where in the generated html source code.

Since the Drupal.settings is defined in the header, along with jquery.js and drupal.js, I think moving the line to a different function might fix the problem:

drupal_add_js(array('googleanalytics' => array('trackDownload' => $trackfiles, 'LegacyVersion' => $legacy_version)), 'setting', 'header');

Cheers,
Stella

hass’s picture

Project: Google Analytics » Drupal core
Version: 6.x-1.x-dev » 6.x-dev
Component: Code » base system
Priority: Normal » Critical

Looks like a critical drupal bug... i have no idea why it shouldn't be allowed to use drupal_add_js in hook_footer.

I cannot find any docs about this... but one thing that could be the cause is the lightbox2 call to theme() in hook_init. This will init the theme system and i know from D5 this would cause big troubles. See http://drupal.org/node/47788.

Pancho’s picture

Title: google_analytics conflict with lightbox2 module » drupal_add_js doesn't work in hook_footer

Just a more meaningful title, as the issue has become a core one.

hass’s picture

Title: drupal_add_js doesn't work in hook_footer » drupal_add_js in hook_footer does not add JS files
hass’s picture

Additional if i try to use drupal_add_js() inside hook_preprocess_page() i cannot add JS files to footer.

stella’s picture

This is nothing to do with lightbox2 - this happens even with lightbox2 disabled.

agentrickard’s picture

It appears that you can't add js files to the header inside of hook_footer(). This is due to the processing order in template_preprocess_page.

  $variables['css']               = drupal_add_css();
  $variables['styles']            = drupal_get_css();
  $variables['scripts']           = drupal_get_js();  <-- this defaults to HEADER region
  $variables['tabs']              = theme('menu_local_tasks');
  $variables['title']             = drupal_get_title();
  // Closure should be filled last.
  $variables['closure']           = theme('closure'); <-- pulls drupal_add_js() for FOOTER region.

theme('closure') calls hook_footer, which expressly calls javascript for the footer region.

You _should_ be able to add footer JS inside hook_footer().

I am very tempted to mark this "by design." The design specification is that if you want JS code in the footer, you should use drupal_add_js() with the 'footer' region specified.

catch’s picture

Priority: Critical » Normal

Either way, I see no reason why this is critical. Looks like either 'by design' or a documentation issue.

hass’s picture

Priority: Normal » Critical
FileSize
1.71 KB

I figured out something more:

1. you cannot add 'setting' in hook_footer()
2. you cannot add a JS file to footer in hook_preprocess_page()

The attached patch could be a workaround (needs review) for google_analytics. I tested myself and it seems working in a very short test... but moving the code to hook_init sounds dirty to me. I can really think about something that will force me to add JS 'settings' in hook_footer. And however i try to turn it... i think here is something really broken or undocumented.

catch’s picture

Back to normal unless there's some critical js implementation that will actually get broken by this.

agentrickard’s picture

I would agree that http://api.drupal.org/api/function/hook_footer/6 is outdated and potentially misleading. The documentation is from 4.6 and predated drupal_add_js().

hass’s picture

GA is broken...

hass’s picture

@agentrickard: it's not only this doc... see #19. I'm sure if i test more hooks i get more bugs... or feature/function limitations :-(

Gábor Hojtsy’s picture

Project: Drupal core » Google Analytics
Version: 6.x-dev » 6.x-1.x-dev
Component: base system » Code

GA bug then

hass’s picture

Project: Google Analytics » Drupal core
Version: 6.x-1.x-dev » 6.x-dev
Component: Code » base system

Gabor: This is not a GA bug. GA is affected, but this is a core bug

catch’s picture

Title: drupal_add_js in hook_footer does not add JS files » Update hook footer documentation
Component: base system » documentation
Priority: Critical » Normal

Docs issue then. Unless you can give an example of something fundamentally broken here, that's what it is.

Gábor Hojtsy’s picture

hass: closure building is conceptually at the end of the page. See http://drupal.org/node/159804 If it is not there, you cannot make any assumptions on what run in the request already (eg. queries run, strings used). Drupal has an architecture to build the important things first (the page content), then the stuff around it (blocks) and the footer, assembling it to a page.

With Google Analytics there are lots of positives with putting it to the page footer. Read eg: http://wimleers.com/article/improving-drupals-page-loading-performance

hass’s picture

Title: Update hook footer documentation » drupal_add_js in hook_footer does not add JS files
Component: documentation » base system
Priority: Normal » Critical

@catch: It is not footer only problem. hook_preprocess_page() should run at the very beginning, isn't it? Why is it not possible to add a JS file to footer region in this hook?

@garbor: in D5 GA worked this way and its works... drupal_add_js added the settings in hook footer to the settings array and was able to add js files to head. The drupal_add_js function is now broken in several ways. No documentation is available about such a feature breaker. No docs are available in module upgrade docs telling about hook_preprocess_page and other preprocess limitations together with drupal_add_js.

So there need to be much more docs added then hook_footer only. However a major feature is broken. If you are not able to add JS code at every code stage. Maybe i have a logic inside hook_footer that can only be there and need to add a setting, and now in D6 it cannot do this anymore.

For e.g. in D5 we have build a special XML module for requesting an external webservice that gets some code via http request and this result is saved in a global var to reduce http requests to only one and not one for every hook - calling the same data. In this case i'm accessing the global array created earlier in footer and add something to head - like meta tags. This worked well in D5, but is not possible in hook_init - or would result in very bad performance and slow page loading. Same will happen if we are forced to do something with JS here.

Are you only trying to get real issues out of the critical issue queue for a final?

Gábor Hojtsy’s picture

Priority: Critical » Normal
Status: Active » Closed (works as designed)

hass, you were using an undocumented behavior of Drupal 5. As hook_footer() docs say "This hook enables modules to insert HTML just before the </body> closing tag of web pages." This is what it does, we did not broke anything. It does not enable you to add JavaScript or meta tags to the header of the page, so your hacks don't work anymore. This does not make this a critical core bug. The fact that it did allow you to do it was just a side effect of "closure" coming before "head" in the alphabet. Look, "help" comes after "head" in the alphabet, so the "fundamental brokenness" of not being able to add JS or meta tags to the header also applies to the help hook (and others called later).

Frankly, I don't understand how using hook_footer() is less of a hack then using hook_init() (based on your comments above at the GA patch).

Why not use what's documented then? Drupal 6 has a nice array of template preprocess options where you can easily stick in your module and preprocess the variables going out for the page theme. See http://drupal.org/node/165706 and http://api.drupal.org/api/function/_theme_process_registry/6 Just implement yourmodulename_preprocess_page() and it will be called whenever a page is themed, allowing you to add whatever you wish to the header/footer.

hass’s picture

Status: Closed (works as designed) » Active

Just implement yourmodulename_preprocess_page() and it will be called whenever a page is themed, allowing you to add whatever you wish to the header/footer.

This is not correct. It is not working as said above. I cannot add JS files to footer - only as one example.

There is NO documentation telling drupal_add_js() cannot be called in hook_footer nor any docs telling it cannot set 'settings' in hook_footer nor hook_preprocess_page cannot add JS files to footer nor any docs say i MUST add the files with hook_init().

If you find something the the following or any other pages i missed and that should document this... please tell us:
http://drupal.org/node/114774
http://api.drupal.org/api/function/drupal_add_js/6
http://api.drupal.org/api/function/drupal_add_js/5
http://api.drupal.org/api/function/hook_footer/6
http://api.drupal.org/api/function/hook_footer/5

http://api.drupal.org/api/function/hook_init/6 tell us

...this hook is a typical place for modules to add CSS or JS that should be present on every page...

, but not telling us - a must. I don't know what i should think about all of this...

Aside http://api.drupal.org/api/function/drupal_set_html_head/6 must work in every page stage, too. I haven't tested in D6 hook_footer yet. Could be another bug.

Gábor Hojtsy’s picture

Hass, you can add JS in hook_footer() just fine, it will be added to the *footer* (and will be present on every page). You don't *need* to use hook_init(). The docs always said that this hook should be used to add stuff to the footer of the page. We can start documenting what you cannot do, but there are obviously more combinations of what you cannot do then what you can. drupal_add_js() and drupal_set_html_head() are good examples, which never worked in every page stage, they did not have any effect when used in the help hook or status message theming or local task theming for example. And this all went undocumented. Why? Because we documented where to put these.

Gábor Hojtsy’s picture

Well, I tried to set up a test module for you.

hasstest.info

name = "Header/footer test for hass"
description = "Show how header/footer additions work"
core = 6.x

hasstest.module

function hasstest_init() {
  drupal_add_js('test-init.js');
}

function hasstest_preprocess_page(&$variables) {
  $variables['head'] = '<badaboo />';
}

function hasstest_footer() {
  drupal_add_js('test-footer.js', 'module', 'footer');
}

This uses all hooks as documented, and does the following:

- adds test-init.js to the header (as expected)
- adds test-footer.js to the footer output HTML (as expected)
- adds the badaboo tag to the head of the page (as expected)

Admittedly, as you pointed out, drupal_add_js(), drupal_set_html_head() and friends are not usable in module implemented template preprocess functions, basically because they come after the HTML head was already built (by design, so you have something to alter), so you can only add direct HTML markup to these variables. If you would be able to use these functions, the header would not be built yet, so there would not be stuff to alter.

Now what documentation is missing here? I think I followed all current documentation when writing this simple module.

mfer’s picture

If you want to add javascript using drupal_add_js in a hook_preprocess_page function try something like.

function hasstest_preprocess_page(&$variables) {
  drupal_add_js('test.js', 'module');
  $vars['scripts'] = drupal_get_js();
}

This will rebuild the javascript with the newly added script included.

hass’s picture

This will not work (i tried this earlier as described in #19):

function hasstest_preprocess_page(&$variables) {
  drupal_add_js('test.js', 'module', 'footer');
  $variables['scripts'] = drupal_get_js();
}

Shouldn't it work?

hass’s picture

I know a command similar to drupal_set_html_head() in coldfusion. It works at all page levels. If this wouldn't be possible the command doesn't make sense. This is why i compare this functions 1:1.

If i ask what i need this command for - it's simple - i'm in a tooo late stage in page processing, but i must add something to head section. Changing code would result in unperformant code doing duplicate SQL request and overloads therefor my DB and other examples.

Such a performance degradation change have now been done for Google Analytics, too. See my commit http://cvs.drupal.org/viewcvs/drupal/contributions/modules/google_analyt... about the change. What i get with this code change is one senseless DB request to variable_get('googleanalytics_legacy_version', FALSE); that must be done in hook_init() and hook_footer(), too. If i need this in more hook i get more senseless requests. This is only a very small example what happens here and i think you will argue this data is cached and quick, etc - but as said it is only a very small example and if i need to fire some http requests to XML webservices the page loading time doubles/triples/quadruples/etc. In this situation we are no more talking about milliseconds - we talk about 5-30 seconds per http request... calling for e.g - d.o. - i would estimate 30-60 seconds per http in a hook. This will result in a very slow and crashing site.

hass’s picture

Aside - i have had an active module that used the logic from #34. This helped GA to add header 'settings' and footer JS correctly - If i'm adding JS settings to head in hook_footer. After i deactivated this module the header setting was missing. This was very annoying and therefor i was unable to repro the lightbox2 problem in the early days of this case.

Gábor Hojtsy’s picture

Status: Active » Closed (works as designed)

hass (#34): no that should not work as you probably intend. You add a JS to the footer and then fill in the header JS variable with the header JS scripts (so you make no change to the header JS). If you add a JS to the footer, drupal_get_js() wil only return it, if you ask specifically for the footer JS. Look at mfer's example. This preprocessor even runs after the hook_footer() implementations, so it is at the far end of the request.

stella’s picture

Status: Closed (works as designed) » Active

Hi,

I tend to agree with Gábor and the others. However I do feel that there is a documentation issue. The API documentation for hook_footer() states:

This hook enables modules to insert HTML just before the \ closing tag of web pages. This is useful for including javascript code and for outputting debug information.

So according to the documentation, a module can validly add javascript to the footer here, and having tested this, it is added to the footer as expected. However, take the case where there is no other javascript added by any other module. In such instances, misc/drupal.js and misc/jquery.js are not present in the header, as they are only placed in the header when drupal_add_js() is called the first time. So if no other module adds any javascript and drupal_add_js() is called for the first time in hook_footer(), then the files drupal.js and jquery.js are not added at all. As in the case with the google_analytics module, these files may be needed by the footer javascript.

Can we update the API documentation to reflect this? I'd be inclined to change it so it recommends against adding javascript in hook_footer().

I'm marking this back to active just so people can see this update. Feel free to change back to 'by design'.

Cheers,
Stella

agentrickard’s picture

@snpower. See #21 for tips about updating the documentation (and why it is misleading).

stella’s picture

I don't have the correct access to update the document, and I don't believe as a non-drupal core developer that I have the authority to say what it should be changed to either.

catch’s picture

snpower - that information is pulled from the phpdoc comments in Drupal via api.module - so if you supply a patch against core, and it gets committed - you'll both have access to change it (kinda) and you'll be a 'drupal core developer', in one fell swoop :)

mfer’s picture

@hass - for the footer you would need to rebuild the closure variable...

function hasstest_preprocess_page(&$variables) {
  drupal_add_js('test.js', 'module', 'footer');
  $variables['closure'] = theme('closure');
}

Though, this is more intensive and I would suggest not doing this. In theme_closure there is a module_invoke_all call.

The first time drupal_add_js is called with data (the first argument) it adds jquery.js and drupal.js to the header. It doesn't matter what region the javascript is placed. If you were using one of the early RCs this was not the case. There was a bug. Make sure you are running something recent or... the release.

stella’s picture

Status: Active » Needs review
FileSize
958 bytes

I've attached a patch to update the documentation for hook_footer(). I also added something similar to the page on Converting 5.x modules to 6.x as I noticed a lot of the contrib modules I have installed attempt to add js to the header from within hook_footer().

Cheers,
Stella

Gábor Hojtsy’s picture

Version: 6.x-dev » 7.x-dev

Since the 7.x release, fixes go to 7.x and then backported.

mfer’s picture

Version: 7.x-dev » 6.x-dev

drupal_add_js calls in hook_footer will only work for the footer region. This is because hook_footer and building the javascript for the footer are invoked in the function theme_closure which is called in template_preprocess_page after the rest of the javscript has been assigned to it's variable.

mfer’s picture

Version: 6.x-dev » 7.x-dev

Oops. Cross posted.

I, also, wonder if it's worth putting in to change

  $variables['site_slogan']       = (theme_get_setting('toggle_slogan') ? variable_get('site_slogan', '') : '');
  $variables['css']               = drupal_add_css();
  $variables['styles']            = drupal_get_css();
  $variables['scripts']           = drupal_get_js();
  $variables['tabs']              = theme('menu_local_tasks');
  $variables['title']             = drupal_get_title();
  // Closure should be filled last.
  $variables['closure']           = theme('closure');

in template_preprocess_page to

  $variables['site_slogan']       = (theme_get_setting('toggle_slogan') ? variable_get('site_slogan', '') : '');
  $variables['css']               = drupal_add_css();
  $variables['styles']            = drupal_get_css();
  $variables['tabs']              = theme('menu_local_tasks');
  $variables['title']             = drupal_get_title();
  // Closure should be filled last.
  $variables['closure']           = theme('closure');
  $variables['scripts']           = drupal_get_js();

This would let you add javascript to the header from hook_footer and shouldn't affect anything else that I can see. Thoughts?

hass’s picture

mfer: if we do such a change... why not moving 'css' and 'style' to the end, too? Same problem...

Robin Monks’s picture

+1 on the documentation patch.

Robin

stella’s picture

FileSize
1001 bytes

Re-rolled patch for Drupal 7

Cheers,
Stella

Patch failed to apply. More information can be found at http://testing.drupal.org/node/13982. If you need help with creating patches please look at http://drupal.org/patch/create

stella’s picture

FileSize
1.03 KB

Not entirely sure why the patch failed, but this new one was now made from just above the contributions/ CVS directory, so hopefully it will work.

Robin Monks’s picture

The borg assimilated DrupalTestbedBot, I wouldn't hold out any hope of it behaving correctly here ;)

Robin

Patch failed to apply. More information can be found at http://testing.drupal.org/node/13984. If you need help with creating patches please look at http://drupal.org/patch/create

Patch failed to apply. More information can be found at http://testing.drupal.org/node/13984. If you need help with creating patches please look at http://drupal.org/patch/create

Robin Monks’s picture

Heh, yup :)

Robin

stella’s picture

So DrupalTestbedBot can't handle this patch because it's against the docs? The patch applies correctly for me against HEAD.

anantagati’s picture

mfer: Thank you for #33, it solved my problem to not be able to add js in hook_preprocess_page().

Dave Reid’s picture

Status: Needs review » Fixed

Committed to docs and working on backporting to previous docs. Side note, anyone with CVS acccount can commit changes to the developer documentation.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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

babbage’s picture

I thought mfer's example in #42 was the solution to a problem we are having with jGrowl that sounds very similar to the original problem reported in this thread. Matt's solution may be what we need, but if so, I'm not quite getting it cause I can't get it to work. Any assistance over in the other thread greatly appreciated. :)

hass’s picture

You need to add JS file to page header?

You need to move the drupal_add_js() calls in D6 and D7 to hook_init(). That's all... you can take a look to google_analytics module if you need further examples.

hass’s picture

Note: This issue seems to block implementation of #648284: Support for Asynchronous Tracking.