On my test system with error_reporting = E_ALL | E_STRICT, the Ad module became unfunctional after upgrading to 6.x-2.0. Firebug revealed the calls for ads were returning many errors:

<b>Notice</b>: Undefined index: tid in <b>/path/to/drupal/sites/all/modules/ad/cache/file/ad_cache_file.inc</b> on line <b>104</b>

<b>Strict Standards</b>: date() [<a href='function.date'>function.date</a>]: It is not safe to rely on the system's timezone settings. Please use the date.timezone setting, the TZ environment variable or the date_default_timezone_set() function. In case you used any of those methods and you are still getting this warning, you most likely misspelled the timezone identifier. We selected 'America/Los_Angeles' for 'PDT/-7.0/DST' instead in <b>/path/to/drupal/sites/all/modules/ad/cache/file/ad_cache_file.inc</b> on line <b>171</b>

<b>Notice</b>: Undefined index: ad in <b>/path/to/drupal/sites/all/modules/ad/cache/file/ad_cache_file.inc</b> on line <b>183</b>

<b>Notice</b>: Undefined index: last_sync in <b>/path/to/drupal/sites/all/modules/ad/cache/file/ad_cache_file.inc</b> on line <b>221</b>

<b>Notice</b>: Undefined index: lifetime in <b>/path/to/drupal/sites/all/modules/ad/cache/file/ad_cache_file.inc</b> on line <b>222</b>

<b>Warning</b>: rewind(): supplied argument is not a valid stream resource in <b>/path/to/drupal/sites/all/modules/ad/cache/file/ad_cache_file.inc</b> on line <b>228</b>

…and then your usual "cannot modify header info" errors.

The release notes say "The entire code base has been reviewed to ensure E_ALL compliance, ensuring general correctness, and improving security," so how did this sneak through?

The Strict Standards issue was taken care of by adding ini_set('date.timezone', 'America/Los_Angeles'); to settings.php. This patch will make the others go away, but I don't particularly like it, since the fact that so many values aren't being set implies the errors are deeper and that this is a Band-Aid® Brand Adhesive Bandages solution. The fact that our ads still aren't displaying even after this fix gives credence to that.

I've never regretted making a back-up before a major update of a module before, and I'm certainly not regretting it now!

CommentFileSizeAuthor
#10 ad_cache_file.install.txt333 bytesGarrett Albright
#1 ad_cache_file_indexes.patch1.69 KBGarrett Albright

Comments

Garrett Albright’s picture

Garrett Albright commented
Title: File cache module not optional? » Hella undefined index errors in ad_cache_file.inc
Status: Active » Needs work
StatusFileSize
new ad_cache_file_indexes.patch1.69 KB

durr, i are attach teh patch.

(EDIT: See updates below before applying this - it's really not necessary.)

Garrett Albright’s picture

Garrett Albright commented
Status: Active » Needs work

Hmm.

I went to the site module list to try to disable the file cache module to see if that would allay the problems. It turns out the file cache isn't enabled in the first place. So why the heck is that code file even being called? A quick peek at the database dump of the backup shows that it wasn't enabled before upgrading either. Is this just a problem of cruft from the upgrade process? I'll investigate a little more.

Garrett Albright’s picture

Garrett Albright commented
Title: Hella undefined index errors in ad_cache_file.inc » File cache module not optional?
Status: Needs work » Active

Enabling the "File cache" module fixed the problem. Even after reverting the changes my previous patch made, the ads started displaying again without a hitch. Disabling the module again caused the problems to return.

I can't recall why I disabled File cache in the first place. Until I do, I'll run the site with it enabled. Perhaps it needs to be made a non-optional module for the base ad system install - though I don't know where that leaves folks who want to use the Memcache module.

jeremy’s picture

jeremy commented
Title: Hella undefined index errors in ad_cache_file.inc » File cache module not optional?
Status: Needs work » Postponed (maintainer needs more info)

I am unable to duplicate this. Disabling the ad_cache_file module works fine.

How are you displaying advertisements?

I suppose there could be an issue if you enable the file cache, build up a lot of cached pages on your website, and then disable the file cache. If you're enforcing a minimum cache lifetime, your cached pages would still be trying to display advertisements through the file cache. Does this sound like what happened to you?

Or, are you displaying ads through a call to ad() via PHP?

Garrett Albright’s picture

Garrett Albright commented

I'm displaying image ads in blocks using the standard "Image Ad" module, plus using a custom module to display ads before videos are played on the site (which I'm having to update -- adserve_select_ad() seems to be gone, so what should I use instead?). The custom module is only enacted when nodes of a certain type are loaded, but just to be sure I disabled it and tried again. I still got errors after disabling "File cache," though. The site is using one other custom module so far, but it's not doing anything with regards to ads.

The currently enabled modules in the Ad section are: Ad, Ad Owners, Click Filter, Image Ad, and Report.

EDIT: On a whim, I tried stripping back to just Ad and Image Ad, but the problem persisted. I'll give it some more sweat after lunch.

jeremy’s picture

jeremy commented

I recommend you view your website by source and look for calls to "serve.php", and see if any of them are setting "c=file"? It does not make sense to me that you're getting errors from the ad_cache_file module when it is disabled, and that is what needs to be tracked down.

Garrett Albright’s picture

Garrett Albright commented

I recommend you view your website by source and look for calls to "serve.php", and see if any of them are setting "c=file"?

Yes! On a page with two image ads, I'm seeing http://example.com/sites/all/modules/ad/serve.php?q=1&c=file&t=3&u=node/15 and http://example.com/sites/all/modules/ad/serve.php?q=1&c=file&t=2&u=node/15 . Uh oh.

Garrett Albright’s picture

Garrett Albright commented

I'm looking at ad.module, in the ad() function, line 102…

  $options['cache'] = variable_get('ad_cache', 'none');

Checking my site's variables, I'm seeing that ad_cache is set to "file". Hmm.

jeremy’s picture

jeremy commented

If you go to "Administer > Content management > Ads > Settings" and select "none" for the cache type at the bottom of the page, do your errors go away?

Garrett Albright’s picture

Garrett Albright commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new ad_cache_file.install.txt333 bytes

Yeah, there we go. Going to Administer > Content Management > Ads, clicking the "Settings" tab, and setting the "Cache" radio button to "None" solved the problem. (I forgot that setting was there, or else I would have tried it earlier…)

It looks like this issue could be avoided in the future if a hook_disable() were added to an .install file to make sure that the file cache isn't still active as the module is disabled. I took the liberty of hacking together just such an .install file for your consideration, and have tested it successfully.

EDIT: Our replies overlapped a bit, but I answered your question anyway. :P

jeremy’s picture

jeremy commented
Status: Needs review » Fixed

Excellent, thanks for tracking this down! Patch committed, with a few changes.

Status: Fixed » Closed (fixed)

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