Posted by pobster on April 26, 2009 at 10:11am
11 followers
| Project: | Advertisement |
| Version: | 6.x-2.2 |
| Component: | ad module |
| Category: | feature request |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
As per title, using jquery method prints message in ad block rather than ad, eg.
http://www.example.com/sites/all/modules/ad/serve.php?m=jquery&q=1&t=121&u=admin/content/ad/configure
Pobster
Comments
#1
Wrote huge text here but nice drupal just ignored it leaving only file attachment. Shit.
#2
In the above patch I have addressed two issues:
1) JQuery method now works
2) Now it also works together with block caching:
- no jquery.js was in html header
- div id's were not unique
Also I'd like to report that ad file cache does not work with jquery method. It just displays error messages instead of ads. I did not go deep into this issue.
#3
Can you attach patches, rather than a zip? That would allow me to review it and possibly merge. I appreciate the contribution!
#4
I was not sure about patch format so I attached zip with various options above.
Below is a patch made with diff -up
Is it right format?
#5
Awesome, thanks. Reviewing now.
#6
I cleaned up the coding style, and removed unrelated changes, but I'm unable to get this working. Please find my cleaned up patch attach. Am I missing a key change?
#7
#8
Yes, you did.
This was the key part:
if (user_access('show advertisements')) {
- if (isset($options['div']) && $options['div'] !== FALSE) {
- return theme('ad_display', $group, $output, $options['ad_display']);
- }
- else {
- return theme('ad_display', $group, $output, 'raw');
- }
+ return theme('ad_display', $group, $output, $options['ad_display']);
'div' option was never created so I deleted it.
The following was essential for block caching to work properly:
- return "\n\n _script type=\"text/javascript\">\n//\n _/script>\n\n";
+ $div_id='group-id-'. $group .'-'. $id .'-'. mt_rand(); // trying to make id unique - useful for block caching
+ return "\n\n _script type=\"text/javascript\">\n//\n _/script>\n\n";
I found in some cases there were duplicate group-id-* used when block caching was ON. So I reinforced uniqueness by mt_rand().
#9
The 'div' options is used by the ad_remote module. I'm still trying to remember exactly why it is necessary -- I suspect if anything that the "if" statement needs to be improved, not simply ripped out.
And the random ID feels like a rather ugly kludge. What good is a random ID? How would you use it in a theme?
Giving this more though, I don't see any reason you'd want to not cache the ad block -- it should be cached. Just enable JavaScript (or jQuery when working) and then it should randomly display ads just fine.
#10
> The 'div' options is used by the ad_remote module. I'm still trying to remember exactly why it is necessary -- I suspect if anything that the "if" statement needs to be improved, not simply ripped out.
Not sure about rip out. But as far as I could see within options array lifetime (it is created within the same function), there is no chance for 'div' element to appear in it. It could be though that I just did not understand something.
> And the random ID feels like a rather ugly kludge. What good is a random ID? How would you use it in a theme?
I thought we need this ID mainly for JQuery to target load() command in the script. If we need ID for theming, we could add another ID I think.
> Giving this more though, I don't see any reason you'd want to not cache the ad block -- it should be cached. Just enable JavaScript (or jQuery when working) and then it should randomly display ads just fine.
I do want to cache it. Random ID is not to prevent caching, but to guaranty that there are no duplicate ID's on the same page. When there are duplicates, banners get loaded in wrong places, which is quite a disaster.
#11
Sorry, options array could be created outside ad() function. But in regular scenario it is empty (when called from ad_block()).
You should know better how to handle 'div' option of cause. I just highlighted in my patch the problematic area and suggested a solution.
In your 2.0 code theme_ad_display() was always receiving 'raw' for $method. That was why jquery method did not work.
#12
Yes, thank you very much for tracking that down. I'm out of time for today, but I hope to resolve this within the next day or two. Your patch and feedback is quite valuable in better understanding the problem.
#13
Sorry... any news about this issue? I'd better stick with javascript method so far?
Thanks a lot!
#14
Bump, what's happening?
#15
JQuery display doesn't work "as advertised" for me either. ;-) Same symptom. Subscribing.
#16
Im having the same issue, subscribing
#17
In case you are not using ad_remote feel free to apply my patch. It works for me on a production server for 2 months already without problem.
#18
Although I couldn't patch the file myself, I snagged the patched module from the .zip and am using it successfully (so far) with 6.x-2.1-rc1. Seems to get jQuery working fine.
#19
Fix committed -- minus the randomization of the id which seems to break theming. If this proves to be a problem, please open a new issue and we'll dig into it further.
#20
Automatically closed -- issue fixed for 2 weeks with no activity.
#21
This bug is still not fixed. It assumes that JavaScript files and jQuery are being loaded in the header. If you have a theme that loads them in the footer (like Blueprint) and decides to follow Google JS loading suggestions, the jQuery method of displaying ads is broken. This is because this module does not follow the Drupal Way of injecting javascript. I am opening this report back up so this issue can be properly fixed. Thank you.
#22
Google JS loading suggestions are not always right, and often break Javascript on my sites. Google alone does not make the standard. Some Javascripts I've used needed to be loaded in the header to function correctly in all browsers. I'd say maybe this should be an issue with the Blueprint theme and not the Ad module.
#23
The point is not whether loading javascript at the top or bottom of the page is correct. The point is that the Ad module should load its javascript The Drupal Way to maximize compatibility. That is why the methods have been created within Drupal.
#24
I was able to get the JQuery method to work with the Blueprint theme (loading JS in the footer) by making a few simple modifications to the ad.module file. This was tested on the ad module version 6.x-2.2.
Replace:
<?phpif ($method == 'jquery') {
return "\n<div class=\"advertisement group-$group\" id=\"group-id-$id\">\n <script type=\"text/javascript\">\n//<![CDATA[\n Drupal.behaviors.ad_module = function(context) { jQuery(\"div#group-id-$id\").load(\"$display\"); }\n //]]>\n </script>\n</div>\n";
}
?>
With:
<?phpif ($method == 'jquery') {
drupal_add_js("$(\"div#group-id-$id\").load(\"$display\"); });", 'inline', 'footer', FALSE, FALSE);
return "<div class=\"advertisement group-$group\" id=\"group-id-$id\"></div>";
}
?>
#25
The code by smthomas didn't work for me - I had to remove the extra "});" from the drupal_add_js call
Code which worked:
<?phpif ($method == 'jquery') {
drupal_add_js("$(\"div#group-id-$id\").load(\"$display\");", 'inline', 'footer', FALSE, FALSE);
return "<div class=\"advertisement group-$group\" id=\"group-id-$id\"></div>";
}
?>
#26
Thanks for the update, foxtrotcharlie. Anyone able to test this out?