Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
When theme_image() is asked to get an image's size, so it can add width and height info to an <img> tag, it calls PHP's native getimagesize() function to get that information. Sometimes getimagesize() fails -- I've seen it happen with jpegs that have lots of EXIF data, for example. In those cases, theme_image() should degrade gracefully, and just spit out an <img> tag without the width and height information. At the moment, it willfully refuses to output anything at all. Patch forthcoming.
Comment | File | Size | Author |
---|---|---|---|
#21 | 561708_image_gd_inc_2.patch | 486 bytes | gleroux02 |
#20 | 561708_image_gd_inc.patch | 485 bytes | gleroux02 |
#18 | 561708_theme_image.patch | 1.01 KB | Island Usurper |
#12 | image.gd_.inc_.3.patch | 550 bytes | jurgenhaas |
#10 | image.gd_.inc_.2.patch | 505 bytes | jurgenhaas |
Comments
Comment #1
ksenzeeI've also removed the is_file() call, since we're now using stream wrappers, which is_file() knows nothing about.
Comment #2
Dries CreditAttribution: Dries commentedNot 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.
Comment #3
ksenzeeI 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.
The benefit of specifying the image size is that the browser reserves the right size chunk of real estate for your image as the page loads. If you have a lot of images, and you don't specify their size, things move around on the page during load. That leads to the annoying situation where you're trying to read the content before page load is finished, and suddenly it jumps down on the page after some big header image finishes loading. Specifying the image size takes care of that. I think removing it would be a regression. (Maybe we could get some input on this from real themers.)
So yes, when you're calling theme_image(), asking it to specify the image size is a plus. It improves the user experience. But it's not a dealbreaker. I can't think of a situation where, if you can't get the image size, you'd prefer theme_image() to return nothing -- which is the current behavior. To me, specifying $getsize in theme_image() means "Please specify the image size, if you can get it. If not, no biggie." Maybe we could change the parameter name to $get_size_if_you_can_please. :)
Comment #4
Dries CreditAttribution: Dries commentedIt 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?
Comment #5
ksenzeeOh, I see what you mean. No, having the parameter means that if you don't care about the image size -- maybe you already know what it is, or it's small enough you're not worried about screen redraws -- you can skip a somewhat expensive call to getimagesize().
Comment #6
c960657 CreditAttribution: c960657 commentedgetimagesize() 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'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:
Comment #7
jurgenhaasI'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
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.
Comment #8
jurgenhaasJust 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.
Comment #10
jurgenhaasForgot to convert line-endings
Comment #12
jurgenhaasAnother try, sorry this is my first patch so I need to find out how it works properly.
Comment #14
jurgenhaasI 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?
Comment #15
eojthebrave@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.
Comment #16
sun.core CreditAttribution: sun.core commentedNot critical. Please read and understand Priority levels of Issues, thanks.
Comment #17
willmoy CreditAttribution: willmoy commentedSub. Just run across this with some perfectly ordinary png files, will try to have a look at the patch next week.
Comment #18
Island Usurper CreditAttribution: Island Usurper commentedI'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.
Comment #19
gleroux02 CreditAttribution: gleroux02 commentedI 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.
Comment #20
gleroux02 CreditAttribution: gleroux02 commentedAlright 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.
Comment #21
gleroux02 CreditAttribution: gleroux02 commentedSorry about that I missed a ) at the end there. New patch added.
Comment #22
das-peter CreditAttribution: das-peter commentedHi,
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
Comment #23
catchWorks for me.
Comment #24
catchTagging with major, random images failing to generate thumbnails is fairly serious.
Comment #25
eojthebraveThis 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?
Comment #26
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #27
MustangGB CreditAttribution: MustangGB commentedComment #28
MustangGB CreditAttribution: MustangGB commentedTag update