The first commit to DRUPAL-5 after the DRUPAL-5--1-5 tag has introduced some fairly serious bugs in imagecache_cache(). There's a $tmpdestination variable being passed to getimagesize(), but that variable is never declared. Looking at the CVS archeology, it seems that this variable used to be heavily used in this function, but was mostly removed in the name of "cleaned up lock/tmp file handling". ;) The result is that the function tries to grab the filesize on a non-existent filename, which then results in a divide by zero:

warning: Division by zero in .../sites/all/modules/imagecache/imagecache.module on line 169.
warning: getimagesize(imagecache/thumbnail/files/images/foo.jpg) [function.getimagesize]: failed to open stream: No such file or directory in .../sites/all/modules/imagecache/imagecache.module on line 208.
warning: filesize() [function.filesize]: stat failed for imagecache/thumbnail/files/images/foo.jpg in .../sites/all/modules/imagecache/imagecache.module on line 209.

So, I fixed the code to use $tmp instead of $tmpdestination, but then I hit some other errors at the end of the function when you're trying to use $dst. That's still just a relative path, yet you're using it for functions like filesize() that need an absolute path. So, instead of wrapping all of those in file_create_path(), I just called that once to initialize a $final_path variable which we then use in all the right spots.

In my testing, the attached patch now makes DRUPAL-5 work without error when generating a new derivative.

Sorry for including it in the same patch, but this also fixes some tabs vs. spaces issues and using {} around if () for better code style, since it was right next to the lines I was fixing anyway. ;)

CommentFileSizeAuthor
imagecache-1.19.2.36-bugs.patch2.05 KBdww
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fighella’s picture

Awesome fix so far! Worked a treat. The issue was driving me insane! Thanks alot for the help! ;)

JD

paulnem’s picture

This issue seems to have made it into the 1.6 release without this patch, a number of users are now having the problem (further described at : http://drupal.org/node/277030 )

I've applied this patch to my 5.x-1.6 install, it appears to work well. Not sure if it needs to be updated at all for the newer version of imagecache.

Would be good to get some others testing this patch and get a new release done.

Just as an added note, I took this as an opportunity to try out thickbox 2.0 and imagecache 2.1. I've resolved the issues I had with upgrading to thickbox so I don't see a reason not to use imagecache 2.1 where THIS issue isn't present. I'll be upgrading my prod site in the next couple of weeks and suggest you do the same if possible.

dopry’s picture

Status: Needs review » Fixed

committed to DRUPAL-5

dww’s picture

Yay, thanks. :)

yan’s picture

I'm getting the following errors with 5.x-1.6, shouldn't the problem be fixed as stated above? Or am I getting something wrong? What does "committed to DRUPAL-5" mean? Should I use the dev version?

Division by zero in (...)/imagecache.module on line 169.
getimagesize(imagecache/<preset name>/files/images/<file name>) [<a href='function.getimagesize'>function.getimagesize</a>]: failed to open stream: No such file or directory in (...)/imagecache.module on line 208.
filesize() [<a href='function.filesize'>function.filesize</a>]: stat failed for imagecache/<preset name>/files/images/<file name> in (...)/imagecache.module on line 209.
paulnem’s picture

Indeed, either downgrade to 1.5, use the patch or get the dev version of the module.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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