Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#51 | drupal_212560.patch | 1.03 KB | stella |
#49 | drupal_212560.patch | 1001 bytes | stella |
#43 | drupal_212560.patch | 958 bytes | stella |
#19 | ga_hook_init.patch | 1.71 KB | hass |
Comments
Comment #1
hass CreditAttribution: hass commentedCould 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.
Comment #2
kylehase CreditAttribution: kylehase commentedI'm already using the latest stable release of Lightbox V2 6.x-1.3.
Also, according to the lightbox page
So it's probably not the same bug. I'll get you those firebug logs.
Comment #3
hass CreditAttribution: hass commentedI 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
Comment #4
hass CreditAttribution: hass commentedComment #5
stella CreditAttribution: stella commentedHi,
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:
When I enable the Lightbox2 module, I get the following error:
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:
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.
Cheers,
Stella
Comment #6
hass CreditAttribution: hass commented1. 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.
Comment #7
stella CreditAttribution: stella commentedI'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
ormisc/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
Comment #8
hass CreditAttribution: hass commentedYep, 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?
Comment #9
hass CreditAttribution: hass commentedComment #10
stella CreditAttribution: stella commentedI 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
Comment #11
stella CreditAttribution: stella commentedI'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 (intemplate_preprocess_page()
) by the time hook_footer() functions are invoked intheme_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:
Cheers,
Stella
Comment #12
hass CreditAttribution: hass commentedLooks 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.
Comment #13
PanchoJust a more meaningful title, as the issue has become a core one.
Comment #14
hass CreditAttribution: hass commentedComment #15
hass CreditAttribution: hass commentedAdditional if i try to use drupal_add_js() inside hook_preprocess_page() i cannot add JS files to footer.
Comment #16
stella CreditAttribution: stella commentedThis is nothing to do with lightbox2 - this happens even with lightbox2 disabled.
Comment #17
agentrickardIt 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
.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.
Comment #18
catchEither way, I see no reason why this is critical. Looks like either 'by design' or a documentation issue.
Comment #19
hass CreditAttribution: hass commentedI 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.
Comment #20
catchBack to normal unless there's some critical js implementation that will actually get broken by this.
Comment #21
agentrickardI 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().
Comment #22
hass CreditAttribution: hass commentedGA is broken...
Comment #23
hass CreditAttribution: hass commented@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 :-(
Comment #24
Gábor HojtsyGA bug then
Comment #25
hass CreditAttribution: hass commentedGabor: This is not a GA bug. GA is affected, but this is a core bug
Comment #26
catchDocs issue then. Unless you can give an example of something fundamentally broken here, that's what it is.
Comment #27
Gábor Hojtsyhass: 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
Comment #28
hass CreditAttribution: hass commented@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?
Comment #29
Gábor Hojtsyhass, 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.Comment #30
hass CreditAttribution: hass commentedThis 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
, 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.
Comment #31
Gábor HojtsyHass, 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.
Comment #32
Gábor HojtsyWell, I tried to set up a test module for you.
hasstest.info
hasstest.module
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.
Comment #33
mfer CreditAttribution: mfer commentedIf you want to add javascript using drupal_add_js in a hook_preprocess_page function try something like.
This will rebuild the javascript with the newly added script included.
Comment #34
hass CreditAttribution: hass commentedThis will not work (i tried this earlier as described in #19):
Shouldn't it work?
Comment #35
hass CreditAttribution: hass commentedI 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 inhook_init()
andhook_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.Comment #36
hass CreditAttribution: hass commentedAside - 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.
Comment #37
Gábor Hojtsyhass (#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.
Comment #38
stella CreditAttribution: stella commentedHi,
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:
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
andmisc/jquery.js
are not present in the header, as they are only placed in the header whendrupal_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
Comment #39
agentrickard@snpower. See #21 for tips about updating the documentation (and why it is misleading).
Comment #40
stella CreditAttribution: stella commentedI 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.
Comment #41
catchsnpower - 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 :)
Comment #42
mfer CreditAttribution: mfer commented@hass - for the footer you would need to rebuild the closure variable...
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.
Comment #43
stella CreditAttribution: stella commentedI'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
Comment #44
Gábor HojtsySince the 7.x release, fixes go to 7.x and then backported.
Comment #45
mfer CreditAttribution: mfer commenteddrupal_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.
Comment #46
mfer CreditAttribution: mfer commentedOops. Cross posted.
I, also, wonder if it's worth putting in to change
in template_preprocess_page to
This would let you add javascript to the header from hook_footer and shouldn't affect anything else that I can see. Thoughts?
Comment #47
hass CreditAttribution: hass commentedmfer: if we do such a change... why not moving 'css' and 'style' to the end, too? Same problem...
Comment #48
Robin Monks CreditAttribution: Robin Monks commented+1 on the documentation patch.
Robin
Comment #49
stella CreditAttribution: stella commentedRe-rolled patch for Drupal 7
Cheers,
Stella
Comment #51
stella CreditAttribution: stella commentedNot 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.
Comment #52
Robin Monks CreditAttribution: Robin Monks commentedThe borg assimilated DrupalTestbedBot, I wouldn't hold out any hope of it behaving correctly here ;)
Robin
Comment #55
Robin Monks CreditAttribution: Robin Monks commentedHeh, yup :)
Robin
Comment #56
stella CreditAttribution: stella commentedSo DrupalTestbedBot can't handle this patch because it's against the docs? The patch applies correctly for me against HEAD.
Comment #57
anantagati CreditAttribution: anantagati commentedmfer: Thank you for #33, it solved my problem to not be able to add js in hook_preprocess_page().
Comment #58
Dave ReidCommitted to docs and working on backporting to previous docs. Side note, anyone with CVS acccount can commit changes to the developer documentation.
Comment #59
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #60
babbage CreditAttribution: babbage commentedI 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. :)
Comment #61
hass CreditAttribution: hass commentedYou 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.
Comment #62
hass CreditAttribution: hass commentedNote: This issue seems to block implementation of #648284: Support for Asynchronous Tracking.