theme_image() chokes if getimagesize() fails

Not sure this makes sense. Passing an extra parameter doesn't seem to help because as a developer, I don't know if getimagesize() is going to fail.

Could the call to getimagesize() be removed completely, maybe? I know it was (and is) considered good practice to specify the image size, but that might be a thing of the past in today's world.

It seems like the parameters doesn't add much value. It doesn't really change the behavior. Already, if it can't find the image size, it skips it?

getimagesize() should be invoked with $path, not $url. Otherwise all images are fetched via the web server. This is a big performance hit compared to when the file is accessed directly in the filesystem.

I've also removed the is_file() call, since we're now using stream wrappers, which is_file() knows nothing about.

I'm not sure what you mean. is_file() does work with stream wrappers, as long as wrapper::url_stat() sets the proper file flag (0100000) in the 'mode' entry. I think the purpose of the is_file() is to prevent a HTTP request if theme_image() is called with an http:// URL. Instead you can use this approach:

  file_stream_wrapper_valid_scheme(file_uri_scheme($path))

I've had the same problem with getimagesize(): not in theme_image but in image_gd_get_info() which is called by image_style_generate() to create a image style derivative. This was driving me crazy but with some hints from above I was able to solve the problem, at least for my current specific scenario.

Instead of

$data = getimagesize($image->source);

I now call

$xy = str_replace('public:/', DRUPAL_ROOT .'/'. file_directory_path(), $image->source);
$data = getimagesize($xy);

and that works with all images where as before about 30% of the images couldn't be loaded.

The aove is a quick hack, so I'm sure the experts in here will be able to make some proper code out of it.

Just created a patch that solves this problem for getimagesize(), not sure if something similar was required elsewhere? I've also changed the assignment from "theme system" to "image system" as it seems more related to images than themes.

Forgot to convert line-endings

Another try, sorry this is my first patch so I need to find out how it works properly.

I need help with this one. The patch file structure is OK now, so I've overcome that hurdle. Now I'm getting error messages from the test itself and I looked into the image.test file but can't figure why my patch should be causing that problem. I do admit, that I'm not familiar with the simpletest system yet.

Can someone please give me some guidance on what to do next?

@jurgenhass, I think you need to replace this:

$data = getimagesize(str_replace('public:/', DRUPAL_ROOT .'/'. file_directory_path(), $image->source));

With this:

$data = getimagesize(drupal_realpath($image->source);

I think that should take care of the issues you're seeing. I think we should probably split this into a separate issue though. Open a new one and post a revised patch there.

Not critical. Please read and understand Priority levels of Issues, thanks.

Sub. Just run across this with some perfectly ordinary png files, will try to have a look at the patch next week.

I've rearranged theme_image() so that it always outputs an img tag so that the alt attribute can be used if it needs to be. I also took out the is_file() check because all it does is prevent getimagesize() from being used on http:// or https:// wrappers. Since public://, private://, and temporary:// all support stat(), I see no reason to bother with it. Any other stream wrappers that might get implemented ought to handle getimagesize() just fine as well.

I have applied this patch to drupal 7.0-alpha4 and it worked perfectly. The patch took without error and the images that were failing the getimagesize() test were able to be printed.

Alright I may of set RTBC a little early. The 561708_theme_image.patch worked fine on an image that was not using image styles. Unfortunately image.gd.inc still was having similar issues with the getimagesize function. Based on eojthebrave's comment I have attempted to create a patch to fix this in image.gd.inc.

Sorry about that I missed a ) at the end there. New patch added.

Hi,

had similar issue - adding drupal_realpath before use of getimagesize also solved it.
Debugging the stream wrapper implementation didn't lead me to a better solution...
btw: drupal_realpath works also with images over http://

Cheers,
Peter

Works for me.

Tagging with major, random images failing to generate thumbnails is fairly serious.

This is possibly related, and may actually solve the problem in this thread. #788102: jpg images do not display for image fields, jpg derivitaves not created or #844676: (Tests needed) Stream wrappers stream_seek() return is invalid

Anyone able to apply the patch from the second issue above and see if it fixes the issues encountered here?

Committed to CVS HEAD. Thanks.

Tag update