Support for more image types (PDF, TIFF, EPS, etc.)

this won't happen in any place but HEAD.

patch doesn't apply to D7.

applied the three patches but still gives me a "Uploaded file is not a valid image. Only JPG, PNG and GIF files are allowed." error.

I believe that imageapi is heading for core:
http://lists.drupal.org/pipermail/development/2009-March/032407.html
, so I wonder if it would be worth seeing if this patch would have a good home at: #416254: Add equivalent of image_get_info() at the toolkit level

But I'm not 100% sure exactly what is going on with image stuff in D7.

Work on porting this and the other patches to imageapi is now underway at #416254: Add equivalent of image_get_info() at the toolkit level

Here is a revised version of the patch. This is basically the same as the first version, except that
* it now should apply to HEAD
* the image conversion function is removed, as this can be achieved using the imagecache actions module
* much of image_get_info() goes into image_gd_get_info()
* there is no expansion of image_get_info() to tiff images using the gd toolkit

The previous patch failed because image_get_info() returned $details (the $filesize) for a non-image. In the patch attached here, the logic has been changed slightly so that $details are only returned if image_toolkit_get_info() returns something non-FALSE.

Hmmm.. I'm confused. The patch passed testing on 11th June, but the testing bot failed to install HEAD on the 12th. I'm submitting the same patch again to see if this is a one-off problem.

On the whole I like the idea of this patch but since getimagesize() isn't GD dependent I don't know if it should really go into that toolkit.

Also:

+ * Drupal supports GIF, JPG and PNG file formats, and may support 
+ * additional formats if toolkits modules are installed.

How about "and may support others depending on which toolkits are installed." ?

I see what you mean about getimagesize(), but there doesn't seem to be an alternative that is in gd proper, and I don't see how it can go in image.inc, as getimagesize() won't deal with PDF files, which is part of the rationale for this patch. So I think it should probably stay in image.gd.inc.

I think your suggestion for the docs is neat - it will go in the next iteration, but I'll wait for more comments before rolling.

HEAD is broken : http://testing.drupal.org/node/35

I'm tagging this with images and ImageCacheinCore as it is related to #371374: Add ImageCache UI Core.

Following. Great work egfrith! This would be a great addition and I'm glad to see it being added to HEAD.

totally keep forgetting to review this but i'm crashing out now so it'll have to be post imagecache.

Thanks quicksketch :-)

@drewish: no worries - I think it makes perfect sense to wait until the mega imagecache-in-core patch you, quicksketch and others are working intensively on is committed to core. I haven't had time to look at that patch in detail, but I suspect it may require changes in this patch.

I'm posting a new version of this patch. There is almost no change in the basic functionality, except that image_load() automatically returns FALSE if the image->info is FALSE.

However, I have added an "image convert" effect. This allows the code to be tested with conversion of PDF, EPS and TIFF files. The image convert effect is modified from the code presently in imagecache_coloractions.module in D6.

I've also made the doc change suggested by drewish at #15.

I'm going to post a patch to imageapi module that allows this code to be tested with imagemagick.

The corresponding patch to imageapi is at: #269329: ImageMagick support for more image types.

I'm going to be on holiday and away from computers and the internet for the next week and a bit.

Fixing erroneous change of status due the test bot working whilst I was working on a post. I don't have time to fix this before I go...

I have now discovered what caused the image_toolkit test failures: the dummy test toolkit in image_test.module needs a "get_info" operation added. Even with this added, testLoad() in image.test needs to add 'get_info' to its array of operations that it asserts were called. As this patch changes a test, I'm not sure what the testing bot will make of it, but I'll give it a go.

This patch should fix the remaining testEffects test that is broken by increasing core's number of image effects from 6 to 7. Semantically this is perhaps a bit odd, as get_info is not really an "image effect" - but perhaps if we think of "image effects" as "operations", it is OK.

Wow, it's been a little while since I've looked at this patch. Do you think file type conversion is really a necessary core feature? I'd think such behavior would be slightly edge-case. I'm just afraid that such functionality would be potentially confusing and need additional documentation and tests. I'd be really happy to see this split and the basic toolkit *_get_info() functions added separate from the image effect functionality.

Thanks for the review! I don't think the file type conversion is really a necessary core feature, in the sense that it could be provided by a contrib module. I also take the points about potential confusion and the need for testing.

I suppose the reason for including the code was (a) to allow the code to be tested on conversion from PDF and (b) because in D6 the conversion code doesn't quite seem to sit happily in any of the imagecache_actions modules. At present, it is in imagecache_coloractions, which doesn't seem very logical. However, these are not strong reasons for including things in core.

So here is a new patch, with the file type conversion code stripped out.

Thanks, this looks great now. I've been working on porting ImageField to D7 and I realized it would be convenient to know what image types a toolkit supports without having to test it out on an image. It seems this would be helpful for this situation also, rather than hard-coding the list of "jpg", "gif", and "png". Some installations of GD actually support things like SVG and some don't support gif. Then we can make ImageField (or just "Image" in D7) work with all the image types that are supported by the current toolkit.

At the same time, this also isn't necessary for this particular patch, but if you're feeling ambitious it might be a good enhancement.

Ambition has won over my worry that this patch won't make it in time for the code freeze :-)

In the new version, there is a new api function image_get_toolkit_extensions($toolkit) and an implementation for gd, image_gd_get_toolkit_extensions(). This function is called by image_gd_get_info().

I suppose that the top-level api function could be called by image_get_info() rather than image_gd_get_info?

Also, I've not been able to find any docs for gd_info() that refer to SVG, so I haven't included it yet.

@egfrith, this is a great idea. Thanks for keeping it moving. I reviewed the patch in #33 and found these issues, and some general questions/ideas. Let me know if you have any questions.

+ * Retrieve the extensions supported by a toolkit

Needs a period at the end

+ * Drupal supports GIF, JPG and PNG file formats, and may support

Has extra whitespace at the end of the line.

- *    'mime_type' - MIME type ('image/jpeg', 'image/gif', 'image/png').
+ *    'mime_type' - MIME type, e.g. 'image/jpeg', 'image/gif', 'image/png'.

This change probably isn't needed.

I think maybe image_get_info() should take an image object as it's only argument. Then this could be removed:

+    $image = new stdClass();
+    $image->source = $filepath;
+    $image->toolkit = $toolkit;

And this

+ $image->info = image_get_info($file, $toolkit);

could be changed to

$image->info = image_get_info($image) or even just image_get_info($image) if we passed by reference.

Either way, the above line has extra whitespace at the end.

+ * One of the image toolkit operations: 'get_info', 'load', 'save',

Also has extra whitespace at the end.

+ * Retrieve the extensions supported by the toolkit

Needs a period at the end, and might want to change to "Retrieve the extensions supported by the GD toolkit." or something similar.

+  if (isset($info['JPEG Support']) && $info['JPEG Support']) {
+    $extensions[2] = 'jpg';
+  }
+  if (isset($info['JPG Support']) && $info['JPG Support']) {
+    $extensions[2] = 'jpg';
+  }

I'm not sure if the isset() is needed in these if statements, I believe gd_info() will return an array where each key is set to either TRUE/FALSE

if ($info['JPEG Support'] || $inf0['JPG Support']) {

Though, I'm not sure about the 'JPG Support', the gd_info() documentation doesn't mention anything about that being a possible return value, only 'JPEG Support'. http://php.net/gd_info

Is there a reason for keying the $extensions array like this + $extensions[1] = 'gif'; why not just $extensions[] = 'gif'

+ *    'width'     - Width, in pixels.
+ *    'height'    - Height, in pixels.

These two line should not have the extra whitespace between the key and the value.

+    $details = array('width'     => $data[0],
+                     'height'    => $data[1],
+                     'extension' => $extension,
+                     'mime_type' => $data['mime']);

Should be reformatted so that each key/value pair is on a new line and indented 2 spaces. The final pair should also have a trailing comma and the extra whitespace should be removed. e.g.

$details = array(
  'width' => $data[0],
  'height' => $data[1],
);

Thinking about this a little more. I think

+ $extensions[1] = 'gif'; should be $extensions['gif'] = 'gif';

This would make it really easy to do things like

 $extensions = image_get_toolkit_extensions('gd');
if (isset($extensions['gif'])) {
  // My GIF support dependent code here ...
} 

@egfrith, this is a great idea. Thanks for keeping it moving. I reviewed the patch in #33 and found these issues, and some general questions/ideas. Let me know if you have any questions.

Thanks for the detailed review!

+ * Retrieve the extensions supported by a toolkit

Needs a period at the end

+ * Drupal supports GIF, JPG and PNG file formats, and may support

Has extra whitespace at the end of the line.

-   'mime_type' - MIME type ('image/jpeg', 'image/gif', 'image/png').
+ *    'mime_type' - MIME type, e.g. 'image/jpeg', 'image/gif', 'image/png'.

This change probably isn't needed.

I've fixed the punctuation and white space and reverted the change.

I think maybe image_get_info() should take an image object as it's only argument.

I agree, this sounds like a logical idea. It was touched on during some earlier work on this patch at #416254: Add equivalent of image_get_info() at the toolkit level. However, it turns out there is a problem: When an image is created with image_load() a resouce handle is created. At present there is no way of closing the resource without doing a save (via image_save()). If the image isn't closed, there could be a memory leak in resource handles.

To fix this in a principled way, we could perhaps write functions such as image_create() and image_destroy() which would create and destroy image objects without opening any resources or writing to file. However, this would be a more radical change, so I won't make any changes to the patch without comments from others.

image_get_info($image) if we passed by reference.

Either way, the above line has extra whitespace at the end.

+ * One of the image toolkit operations: 'get_info', 'load', 'save',

Also has extra whitespace at the end.

+ * Retrieve the extensions supported by the toolkit

Needs a period at the end, and might want to change to "Retrieve the extensions supported by the GD toolkit." or something similar.

All fixed

+ if (isset($info['JPEG Support']) && $info['JPEG Support']) {
+    $extensions[2] = 'jpg';
+ ; }
+  if (isset($info['JPG Support']) && $info['JPG Support']) {
+    $extensions[2] = 'jpg';
+  }

I'm not sure if the isset() is needed in these if statements, I believe gd_info() will return an array where each key is set to either TRUE/FALSE

if ($info['JPEG Support'] || $inf0['JPG Support']) {

Though, I'm not sure about the 'JPG Support', the gd_info() documentation doesn't mention anything about that being a possible return value, only 'JPEG Support'. http://php.net/gd_info

On that page there is: "Note: Previous to PHP 5.3.0, the JPEG Support attribute was named JPG Support." This is why the isset() statements are needed. I suppose an alternative would be to check for the version of PHP; I don't have a strong preference so I'll leave the status quo.

Is there a reason for keying the $extensions array like this +    $extensions[1] = 'gif'; why not just $extensions[] = 'gif'

The reason (at present) is that the numbers correspond to IMAGETYPE_GIF, IMAGETYPE_PNG and IMAGETYPE_JPEG, as returned by getimagesize(). See http://uk3.php.net/manual/en/function.exif-imagetype.php.

+ *   'width'     - Width, in pixels.
+ *   'height'    - Height, in pixels.

These two line should not have the extra whitespace between the key and the value.

+    $details = array('width'     => $data[0],
...

Should be reformatted so that each key/value pair is on a new line and indented 2 spaces. The final pair should also have a trailing comma and the extra whitespace should be removed. e.g....

These are both now fixed.

Thinking about this a little more. I think

+    $extensions[1] = 'gif'; should be $extensions['gif'] = 'gif';

I agree, this does look neater. However, it would put a little more complexity into image_gd_get_info(), as it would have to map $data[2] to 'gif' or 'png' - however the php function image_type_to_mime_type() might be helpful here. Alternatively, it should be possible now to do:

 $extensions[IMAGETYPE_GIF] = 'gif';

More generally, this has got me wondering whether the extension is useful element at all - it would appear to duplicate the mime_type element, and I haven't seen it actually used in any of the core image files - but on the other hand quicksketch clearly is using it for something!
On the other hand, maybe image_get_toolkit_mime_types() would be just as good as image_get_toolkit_extensions() ?

My suspicion is that quicksketch wants to use it in imagefield, perhaps for doing file validation. Being able to inform the admin what image types their server supports when they are creating a new image field or something like that.

It seems to me that this line is already doing mapping that would work fine with $extensions[IMAGETYPE_GIF] = 'gif';

$extension = array_key_exists($data[2], $extensions) ? $extensions[$data[2]] : '';

Since $data[2] = IMAGETYPE_GIF

I could go either way with the image_create(), image_destory() stuff. Potentially useful functions incase we come across something like this again, but image_get_info() works fine using the current method as well.

To clarify the need for a list of supported extensions, here's the use-case. Right now ImageField assumes a restriction of just JPG, GIF, and PNG images, regardless of what the capabilities of the current toolkit are. When configuring an Image field, the administrator is asked for a list of "Allowed extensions", which defaults to jpg, jpeg, gif, and png. The validation routine on that field then does not allow any other extensions to be entered, or if other extensions are already present on the field (such as if the admin changed the widget to "Image" when it was just a "File" previously), then all extensions except png, gif, jpg, and jpeg are stripped from the list when the widget is displayed on the node form.

So basically I want to broaden this support so that Image fields can support things like TIFF as long as the toolkit supports TIFF so we can generate a thumbnail to be displayed on the node edit form. Once this list can be retrieved from the toolkit, it can be array_intersected with the list of extensions the administrator has entered.

As for why we need $image->extension, I'm not sure we really do need that at all. It probably shouldn't be filtered down based on image_gd_get_toolkit_extensions() though, since if an image can have the extension jpeg or jpg and still be an image/jpeg.

That brings up an important point that both "jpeg" and "jpg" should be in the list of supported extensions if JPG support is available, since GD will support either file name.

Hmm... it seems to me that the most principled way of dealing this would be for:

* each toolkit only to return which mime_types they support in functions such as image_gd_get_toolkit_mime_types()

* convert this list of mime types into a list of extensions using a new function, perhaps called file_mime_types_to_extensions(), which would utilise file_default_mimetype_mapping() in a similar way to how file_get_mimetype() does currently, but to map an array of mime_types to an array of extensions.

* remove the extensions field in image_get_info() - if extensions for an image are required, they can be found using file_mime_types_to_extensions()

How does that sound?

My reservation is that it's maybe using a sledgehammer to crack a nut, but on the other hand, file_default_mimetype_mapping() does contain multiple extensions such as jpg, jpeg and tif and tiff.

If the idea is good, should we create a new issue for the new file function - I'm getting a bit concerned that the scope is creeping...

egfrith, I think you're right, let's just go back to the hard-coded lists for now, I didn't think this would require such extensive changes at it is now requiring. Sorry for the troubles. :-(

No worries! Here's a new version of the patch, which has the image_toolkit_get_extensions functions stripped out, and the hardcoded extensions array put back, albeit indexed by IMAGETYPE_GIF &c. Might this patch be RTBC now?

Re the idea about creating file_mime_types_to_extensions(), would this be worth pursing in another patch? I don't suppose it would get in for D7, but perhaps for D8?

This looks good, could we confirm that constants like IMAGETYPE_GIF are available even when GD is not installed? IMAGETYPE_* constants are listed specifically on the GD PHP documentation page: http://us3.php.net/image.constants.

Also I think we should change:

Drupal supports GIF, JPG and PNG file formats, and may support...

to:

Drupal supports GIF, JPG and PNG file formats when used with the GD toolkit, and may support...

From the docs, it looks as though IMAGETYPE_* won't be available when GD isn't installed. But I don't think this is a problem, as they are only referred to within image.gd.inc. The constants don't make it to the output of image_get_info(). I think using them makes the code a little clearer. However, we can go back to the original.

I'd be happy to make the doc change you suggest - I'll do that this evening.

Subscribe.

egfrith, the mime type and extension are both useful since it's not a 1:1 mapping. i think user picture uses the extension to construct the user's picture path, e.g. $user->uid . $info['extension'].

Thanks for the clarification drewish.

The attached patch fixes the changes to the docs proposed by quicksketch at #43. Everything else, including returning the extension, remains the same.

these changes look really good but i'm a tiny bit concerned about using the IMAGETYPE_* constants as keys for the extensions. by keying them it seems to imply an additional constraint on the API that's not documented and that other toolkits are going to have to mimic. we can either document this, drop their use, or define the constants if GD isn't installed...

The way the code in image_gd_get_info() is written, we have to key the extensions array, because the third element in the array returned by getimagesize() is IMAGETYPE_GIF (=1), IMAGETYPE_JPEG (= 2) or IMAGETYPE_PNG (=3), depending on the type of image, and this is then used to select the element of $extensions. If $extensions were not keyed, the wrong element would be returned.

None of the keys of $extensions are returned by image_gd_get_info(), so I don' think using the constants implies an additional constraint on the API. We don't need to define IMAGETYPE_*, as they are only used in image.gd.inc. If they are not defined, neither is getimagesize().

In the current version of image_get_info(), the $extensions array is also keyed, but just by '1', '2' and '3' rather than the "logical" constant names.

If the problem is keyed arrays, we could write image_gd_get_info() in a different way, e.g.:

switch ($data[2]) {
    case  IMAGETYPE_GIF:
        $extension = 'gif';
        break;
    case  IMAGETYPE_JPEG:
        $extension = 'jpg';
        break;
    case  IMAGETYPE_PNG:
        $extension = 'png';
        break;
  }
   

Ooops - I was wrong about getimagesize(): it does not require the GD library, being a part of standard PHP. However, the IMAGETYPE_* constants are also a part of standard PHP, being defined in the same file as getimagesize():

http://svn.php.net/viewvc/php/php-src/branches/PHP_5_2/ext/standard/imag...

So we don't need to define the constants for use elsewhere in drupal. Given that's the case, I think that it would be fine to use them. However, to clarify that the constants are not defined in the drupal source code, perhaps we can add a comment like this, just above the definition of $extensions?

// IMAGETYPE_GIF, IMAGETYPE_JPEG and IMAGETYPE_PNG are constants defined by PHP;
// see http://www.php.net/image.constants

That said - and given the impending code freeze - I'm happy to go back to '1', '2' and '3' as keys. Having had a scan through the drupal source code there aren't very many occasions on which PHP constants are used. I've only been able to find constants such as T_CURLY_OPEN used in registry.inc, so perhaps using PHP constants is a bit non-standard and hence potentially confusing.

Apart from the issue of the IMAGETYPE_* constants, drewish, quicksketch and and eojthebrave seem to be OK with this patch. I'm therefore replacing the IMAGEYPES_* constants with '1', '2' and '3', as currently stands in drupal HEAD, and marking the patch RTBC. If this isn't an appropriate course of action, please switch the status back!

Looks good to me! Seems like I shouldn't have raised such a fuss and we could have been okay with #36 ;-)

+ * Image tookit's get_info operation.

Egfrith, if you want to re-roll a new patch to fix the spelling mistake here awesome. If not, I don't see why whoever commits this can't just fix it at that time.

Thanks for sticking with this one, awesome work.

Bah gum, you have an eye for detail eojthebrave! Here's a new version with the typo fixed in the new code - though there is still a typo in the existing code... perhaps a separate issue?

+++ includes/image.inc	25 Aug 2009 20:42:32 -0000
@@ -101,10 +101,14 @@ function image_toolkit_invoke($method, s
+ *   An optional, image toolkit name to override the default.

No need for this comma.

+++ includes/image.inc	25 Aug 2009 20:42:32 -0000
@@ -114,23 +118,24 @@ function image_toolkit_invoke($method, s
+  if (!$toolkit) {
+    $toolkit = image_get_toolkit();
+  }
+  if ($toolkit) {

How is $toolkit *not* set by now, when we just set it if it was not set? Is that second if necessary?

+++ modules/system/image.gd.inc	25 Aug 2009 20:42:33 -0000
@@ -321,5 +321,38 @@ function image_gd_create_tmp(stdClass $i
+ *    'width' - Width, in pixels.
+ *    'height' - Height, in pixels.
+ *    'extension' - Commonly used file extension for the image.
+ *    'mime_type' - MIME type ('image/jpeg', 'image/gif', 'image/png').

These should be in the format of:

 - thingy: Definition

See hook_theme()'s docs in system.api.php as an example.

+++ modules/system/image.gd.inc	25 Aug 2009 20:42:33 -0000
@@ -321,5 +321,38 @@ function image_gd_create_tmp(stdClass $i
+  $data = @getimagesize($image->source);

Why the suppression operator? This is normally a bad practice, so if we need it here, let's comment why.

Fix these up, and I can commit this tomorrow or else at the ice cream sprint. :)

Beer-o-mania starts in 3 days! Don't drink and patch.

Incorporating above feedback:

No need for this comma.

Removed.

How is $toolkit *not* set by now, when we just set it if it was not set? Is that second if necessary?

This happens if no default toolkit exists (which will return FALSE). We do the same thing in the image_load() function.

These should be in the format of:
- thingy: Definition

Fixed.

+ $data = @getimagesize($image->source);
Why the suppression operator? This is normally a bad practice, so if we need it here, let's comment why.

We were just copying code that was already in existence (D6 has this same thing in place). However I found that file.inc is not doing this sort of thing for its calls to filesize(), which is inconsistent with image.inc (which calls @filesize()). So I removed the suppression operators here too.

re @getimagesize($image->source)
Could it be for a file called something.jpg that isn't a jpeg, or is corrupted? suffix != mime-type. Methinks getimagesize complains if that happens. Has the integrity of the file been checked yet?

Thanks for the review and the fixes.

According to

http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/user/user.module?r...

committed (with very little discussion by modern drupal standards!) in #9958: User pictures/avatars broken? and referenced in #6449: Invalid img tag results from submitting user info without updating user picture, the reason is:

Suppressing PHP errors from getimagesize() using @. drupal_set_message() is used to report these errors already and in a much prettier way.

(This code got from user.module to image.inc in #16358: Core image handling).

Does this still stand up as a reason? I'm not an expert on how drupal catches PHP errors.

I've had a look at the PHP code behind getimagesize() at:

http://svn.php.net/viewvc/php/php-src/branches/PHP_5_2/ext/standard/imag...

In php_getimagetype() (called by getimagesize()) there is one E_WARNING ("PNG file corrupted by ASCII conversion"). I'm fairly sure we couldn't catch it before calling getimagesize() by any other means. There are also a number of E_NOTICEs ("Read Error!") which I don't know if we could catch.

getimagesize() doesn't convey this information in its output. So if we suppress the errors we potentially suppress useful debugging information, but we do probably break E_ALL compliance. I don't know what the right answer is!

I've tested the patch without the suppression operator with a few images, some with the correct ending and some without. I've not managed to induce and E_WARNINGs or E_NOTICEs. So I think that, in addition to the above reasoning, dropping the suppression operator is fine. I've also noticed that the reformatting of the array attributes still needed to be done in one of the get_info() functions, so here is a new version of the patch with all the formatting (hopefully) corrected.

As I think all the issues have been addressed, I'm marking RTBC. Hope you're enjoying lots of ice cream!

Committed to HEAD! Thanks a lot! :)

Thank-you very much - I've an inordinate level of satisfaction at this relatively miniscule contribution to D7 :-)

The TIF/PDF/EPS feature is GENIUS!

With a print publishing background (from a long, long time ago) I've ALWAYS struggled with the concept of uploading a compressed image (jpg), to have it cropped/scaled/modified and then "re-compressed" for display.

Now people can begin with TIFs for store product master images, and everything can be rendered beautifully by imagecache (or whatever it will be in 7) on the fly. It's tough using re-compressed photos for stores needing high quality closeups/previews of products. It's also a really cool idea for a professional image gallery too.

Thanks!

for more info about svg support, see #238678: Support for pluggable image formats (or at least adding SVG!)