First of all let me say, nice work Khalid. Very fun module. I like it very much. I had one problem with it though, I figured out very quickly that the image ads did not work at all, they always came out as text. So, i dug into the code, and i found the issue, you count from 1 to 10. Counting from 1 to 10 is fine and all, but programmers are supposed to count from 0 to 9, because the computer does too. Attached is a patch to correct the problem. I also added a third radio box called both, for both text and image, ie.. adtype = text_image
Well, check out the .patch, you'll see. And once again, thank you very much for this module, hacking it up taugh me a bunch.
-J

CommentFileSizeAuthor
#2 adsense.try1.patch444 bytesjon@jony.net
adsense.patch862 bytesjon@jony.net

Comments

kbahey’s picture

kbahey commented

The patch contains only the second change (Both as ad type).

I do not see any changes for image not displaying (I never tried image ads, I don't like them). I don't understand why they would not display. Anyway, another patch for that is fine, then I will commit both.

jon@jony.net’s picture

jon@jony.net commented
StatusFileSize
new adsense.try1.patch444 bytes

No, this patch does contain both fixes. To illistrate, attached is my first try at adding the text_image type, witch did not work. You will notice there is a very suttle diffrence between this patch and the correct one already posted.

In this example text_image becomes case 3, but will never be triggered because drupal treats radio box one (text) with a value of 0, a radio box two (image) with a value of 1, and radio text box three (text_image) with a value of 2. Drupal starts counting from zero, not 1. With the attached broken patch, when you select (both) in the adsense module prefs, it outputs the code for "image", and if you select either text or image it simply outputs "text".

With out either of these patches (your virgin copy) no matter what you select, you get "text" scince there are only two radio boxes, the only possible values for variable_get('adsense_ad_type_' . $group, '0') to return are zero or one, never 2, so case 2 will never be triggered and $type allways default to $type = 'text';

Also, dont forget that you cache each ad format, so simply patching the modue, and changing your prefs will make images or text_image work, you will have to truncate your cache table or something. maybe it would be a good idea to flush the cache (not the whole cache just the adsense stuff) somewhere in the prefs. I'd add that to the patch, but i really dont know how to do that.

i hope this helps, feel free to contact me again if you have any questions.

kbahey’s picture

kbahey commented

Thanks for the clarification. I commited your fix.

As for the cache, any time you load the settings page for adsense, the cache is cleared.

This line is supposed to do it: cache_clear_all('adsense', TRUE);

Anonymous’s picture

(not verified) commented