I found that putting multiple theme('imagecache', ..)'s on a page consistently causes one or more image generations to fail, logged as a misguiding "directory is not writable" error.
example: in a cck node's .tpl.php:
print theme('imagecache', 'HistoricalMarkerImageThumb', $field_bigimage[0]['filepath']);
print theme('imagecache', 'HistoricalMarkerImageMed', $field_bigimage[0]['filepath']);
print theme('imagecache', 'HistoricalMarkerImageLarge', $field_bigimage[0]['filepath']);Of this, typically the first call produces ..Thumb and ..Large, but not ..Med. An error is logged for ..Med. Reload of this page works fine (b/c two are already pre-gen).
I believe this is caused by multiple processes (each concurrent browser request for img-src="..." on the page) trying to access the same tmp file. It appears that an attempt at handling this was put in place in imagecache.module:~104, the test for !is_file($tmpdestination), but that completely bypasses the entire image creation routine! And since $generated = TRUE is set in both cases (if/else), the module continues along its merry way to call the missing image, adding a fistful of errors to the log.
- First Recommendation:
- Make the tmp file unique:
change :80
$tmpdestination = file_directory_temp() . str_replace(dirname($path), '', $path);
to
$tmpdestination = file_directory_temp() . '/' . $preset['presetid'] . str_replace(dirname($path) . '/', '', $path);
which would make tempfile paths like /tmp/1origimagename.jpg, /tmp/2origimagename.jpg, /tmp/3origimagename.jpg - I opted for pathid over pathname for brevity but still unique
- :~104-107
if (!is_file($tmpdestination)) { $generated = TRUE; file_copy($source, $tmpdestination); - Remove $generated = TRUE;. Add curly brace to close this conditional block. Replace "else" portion (and removed closing curly brace) of this conditional block in :130-132 with $generated = TRUE;.
Second Recommendation:
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | imagecache_0.patch | 592 bytes | texas-bronius |
| #5 | imagecache.module_3.patch | 3.12 KB | texas-bronius |
| #2 | imagecache.module_2.patch | 3.12 KB | texas-bronius |
Comments
Comment #1
dopry commentedCould you roll a patch? prepending the presetname to the tmpfile seems like good solution.
http://www.drupal.org/diffandpatch
Comment #2
texas-bronius commentedRight on. I was just reading up about mismatchin' patchin'. I got caught up in trying to figure out how to work out the diff's in filenames, b/c this creates a new file called "imagecache.module.bak" (your preserved orig file's name)
See the patch, attached... please let me know how to improve my process: this patch will create a .bak file. :|
Comment #3
texas-bronius commentedn00b here.. I think the prev patch is not what we're looking for.. bear with me: i'll get it soon! :D
Comment #4
dopry commentedThe patch is close... I'll make you keep fixing it until you get it right. :)
I wouldn't change the if (!is_file($tmpdestination)) block, its a sort of locking mechanism.
This chunk of code prevents multiple apache requests from trying to work on the same file....
Say you get 5 requests for tmpdestination before it is generated and put in its final location....
You have 5 processes trying to manipulate the same file... You won't get the expected results...
While the case is unlikely for any low volume site... any high volumn site after a cache flush might experience bad behavior resulting in horribly mangled images... I'd rather an user see an image not found and things look ok the next page view, that I would like them to see horribly mangled images until I notice them and flush the cache, only to find another image has been horribly mangled...
The else case for that if block should be to deliver a 'placeholder' image for the preset. I haven't implemented that feature yet.
Comment #5
texas-bronius commentedOk. See attached patch. After having created the patch, I changed the filenames in --- and +++ of first two lines to "imagecache.module" manually. Is that correct?..
Comment #6
texas-bronius commentedI hate to turn this into a patch thread and mislead other journeymen as I have so fondly been misled in the past myself. My patch above does what it says including the breaking of your is_file check... Can we take this offline (gmail) until I learn how to patch?
Comment #7
dopry commentedfor generating patches, when I'm not using cvs I normally have a workflow like...
copy example.module to example.module.orig
$ cp example.module example.module.orig
edit example.module
$ vi example.module
generate a diff in unified format from example.module.orig to example.module into a patch file.
$ diff -u example.module.orig example.module > example.patch
hope that helps.
Comment #8
texas-bronius commentedThe attached patch is against the original 4.7.x-1.x and supercedes all mispatches of mine above. Per dopry's last chastisement (kidding) above, this fixes only Recommendation 1 above, making a unique tmpdestination filename by prepending filename with presentname (not presetid).
thanks for your help d.
Comment #9
texas-bronius commentedfyi: when I zap my test files:
rm -fdr files/imagecacheand reload, it works. But my Drupal log still reports
The directory sites/example.com is not writable, because it does not have the correct permissions set
Comment #10
dopry commentedThat last error log you mention is mostly harmless.. It'll just fill your log table after a couple eons.
Commited to the 4.7.-1.x branch.
Comment #11
(not verified) commented