Prevent image request errors from bubbling up to the entire page.

Yes v2 does it. I might look through the code to see if i can help.

Unfortunately this is a real showstopper for me.

I just looked at the code and this is simpler than I thought :-)

Maybe there should be the choice of returning false or some default url if the image retrieval fails. The default or error url could be a value passed in.

Different topic and not trying to find fault but I think it should probably implement a lock around the download much like in imagecache.module. I'm not sure what happens if we get multiple requests for the image at the same time.

I haven't studied how to do patches but I will and see if I can do something useful.

Thanks

I think I'm making this more complicated that it needs to be although I still don't understand something.

I suddenly realized that there's a difference between this:

<?php $image_url = imagecache_external_generate_path($url, $preset); ?>
<img src="<?php print $image_url;?>" alt="image description" />

and

<img src="<?php print imagecache_external_generate_path($url, $preset); ?>" alt="image description" />

I tried the first version by blindly copying and pasting from the instructions and of course drupal_not_found() in imagecache_external_fetch 404s the whole page.

I expected the second version to solve the problem because I think it's all in a separate request made by the browser. But... it still seems to 404 the whole page if I give it a bad url. I don't understand that. Investigating further.

Edit: D'oh! I was confusing myself. The second version still does the processing in the main request. I'm trying to figure out how ImageCache does it because I think it essentially deals with the same problem if somehow the preset fails.

Thanks for the quick work! I'll try it tomorrow and report.

A funny thing. It took me a while to figure out how ImageCache avoids the problem because it throws http errors if the preset doesn't exist etc but it doesn't mess up the whole page. I see now that it's because the real work in ImageCache happens when the special image url is requested, not when the image url and tag is generated. Any errors only affect the image request.

I started to suggest that this might be a better way to do the next version of ImageCache External and then I noticed that's exactly how v1 worked. In some ways it feels like a cleaner way to go but I can understand the point about performance.

I still worry a little about the lack of locking around the download but I can't quite see how it could be implemented in v2. With v1 I think it would have been easy to do it the same way as ImageCache and do another request if the download is in progress but that's not an option with the main page request. I guess that's a topic for another thread.

Cheers
Ross

The patch works nicely, thanks.

I guess some people are never happy but... I think it would be nice to have the choice of either using the default url or simply returning false if the external image cannot be downloaded. A very simple way to do that could be a $use_default, parameter passed into imagecache_external_generate_path.

In some cases it would be useful for the template to know if we have a real image.

<?php $url = imagecache_external_generate_path($url, $preset, false); ?>
<?php if ($url): ?>
<img src="<?php print $url; ?>" />
<?php endif; ?>

As for locking. I think there is a fairly easy solution. file_save_data will rename by default so we should use the result of that to generate the final url rather than assuming it doesn't change. It just means that there will be two copies in the external and ImageCache caches if two users collide.

I take back some of my previous comments about v1 maybe being a better way to go. I didn't realize it was doing a Drupal bootstrap for each image request. I spent some time thinking about it and looking through the code and I think I understand some of the issues better now. I think the v2 method is a good compromise. It's more complicated than I first thought.

I'm sure you went though this too but I was thinking that it would be nice if it could work like ImageCache directly where PHP and Drupal is not involved at all once the image has been downloaded. The actual request would trigger the download and a 404 would be returned if the image doesn't exist. It would work nicely with simple external urls but it gets messy when you try to map a long complicated urlencoded url to a directory in the file system. V2 almost achieves the same thing. We still unfortunately hit the file system every time in imagecache_external_generate_path but I can't see a way to avoid that. Of course it all becomes a bit of a moot point with anonymous users and the page cache.

Thanks
Ross

Thanks for comments. I'll look at doing a patch but it might be a week or two before I get to it. I'd like to do something about the locking too. I don't think we should ignore that.

For the purpose of perhaps a pointless discussion, I'll clarify the point I was making in case it causes someone to have a bright idea. I'm just thinking out loud.

I think I understand how v1 and v2 work and I understand what you're saying. However, it's still not quite equivalent to how ImageCache works. It would be nice if worked like ImageCache but it might be a bit impractical and not worth the extra complexity. I think v2 is a good compromise.

With ImageCache, when you do this

print theme('imagecache' .... )

that does nothing except generate and print a string. It doesn't touch the image in any way. Then when the browser requests the image url, the rewrite rules cause the image to be served directly if it exists otherwise Drupal is bootstrapped to generate, serve and save the image. Mod-rewrite or whatever does the test, not PHP. If the image exists then PHP has absolutely no part in serving it.

imagecache_external_generate_path is different in that it does more than return a string. It checks for the image and downloads it if it doesn't exist. Since it is typically called whenever the image tag is on the page, that file exists test happens on every request long after the image has been downloaded.

The problem with v1 is that the image never exists directly at that url so Drupal is bootstrapped every time. I know v2 was written to avoid that.

A nice alternative would be to do something that is kind of the best of both v1 and v2. The practicality of it depends on how complex the external URL is.

For an external url like this:

www.somedomain.com/part1/part2/someimage.jpg

we would generate a image src url of the following or even hardcode it because all we need at this point is a string.

/sites/default/files/external/presetname/www.somedomain.com/part1/part2/someimage.jpg

then Drupal would start as expected if the image does not exist. We'd make /sites/default/files/external call our module but instead of doing what v1 does, we should generate the final image and place it at that location in the file system rather than keeping the external image in /externals. I guess that means downloading, calling ImageCache and moving the final image.

Subsequent requests will then be served the image directly without needing the file exists test in imagecache_external_generate_path. PHP would not be involved and imagecache_external_generate_path would need to do nothing more than string manipulation.

I think that would work well but would get messy with a complex url. e.g.,

www.somedomain.com/part1/part2/someimage?x=1&y=2&z=3

You'd need to encode it to something that could be mapped to a directory and file. Urlencode of only the query string gets close.

/sites/default/files/external/presetname/www.somedomain.com/part1/part2/someimage%3Fx%3D1%26y%3D2%26z%3D3

I don't know about % in a file path. It might need some modified encoding. Maybe an ampersand would map to a slash to avoid any one part of the path getting unreasonably long. You'd probably need .jpg or whatever on the end even if the external url didn't have it to make sure we serve the correct content type.

Well... that's what I was thinking. Probably not worth it for the sake of eliminating that one "does it exist" test so v2 is probably a good compromise but... then again if you have a page with a number of external images then maybe it would be worth it on a busy site.

Cheers
Ross