As part of the effort to make it possible for imageapi to upload PDF and TIFF images (#400110: Ability to upload .tif file), drewish has stated that:
i'd be interested in adding functionality to imageapi to determine if a file is an image... basically replicating image_get_info() at the toolkit level so that imagemagic could determine that pdf's are valid images, etc. then imagecache would have something more intelligent to base it's resizing off of.
I'm opening this feature request to coordinate work on this.
It looks like wrunt has already implemented this, but in patches to image.inc, image.imagemagick.inc and image.module:
#269327: Support for more image types (PDF, TIFF, EPS, etc.)
#269329: ImageMagick support for more image types
#269337: Support for more image types (PDF, TIFF, EPS, etc.)
Having had a quick look at the imageapi code and these patches, I think a lot of the work could be used here. I'm going to put a note on these issues pointing them to this one.
If this work is done it might also address #366373: PDF support ( i.e. convert PDF to JPG support)
Comment | File | Size | Author |
---|---|---|---|
#24 | imageapi_416254-24.patch | 15.32 KB | egfrith |
#19 | imageapi_416254-19.patch | 11.91 KB | egfrith |
#18 | imageapi_416254-18.patch | 11.18 KB | egfrith |
#17 | imageapi_416254-17.patch | 11.31 KB | egfrith |
#11 | imageapi_416254-11.patch | 11.44 KB | egfrith |
Comments
Comment #1
quicksketchegfrith, you might be interested to know that ImageAPI is already in D7 core: #373613: Create "Image" Objects, Operate on Images By Resource. If this does get implemented we want to make sure core gets it also.
Comment #2
drewish CreditAttribution: drewish commentedquicksketch, i'd started to add this functionality to the core patch but decided it was a bit too much to take on. i think it might be good to get a working version in contrib and then look at pushing it into core.
Comment #3
quicksketchThis sounds like a great idea overall. FileField/ImageField in D6 currently don't leverage ImageAPI at all if it's available, this would make another great situation where such such support should be added.
Comment #4
egfrith CreditAttribution: egfrith commentedI'm working on this at the moment, and posting the patch so far, much of which comes from the patches mentioned earlier. This is not really suitable for detailed review; more so that the direction of travel is evident.
Also, it would be useful to have a comment on exactly what the get_info() function should be called - I'm assuming imageapi_get_info().
And should imageapi_image_open() invoke the correct imageapi_$toolkit_image_get_info() command instead of calling image_get_info().
Comment #5
egfrith CreditAttribution: egfrith commentedComment #6
egfrith CreditAttribution: egfrith commentedHere is a patch that I think is worth reviewing, and a description of what it does. I have taken some of the caching stuff out that was in previous iterations of the patch. Perhaps this should go back in, but for now, here is a fairly minimal patch:
imageapi.module
imageapi_gd.module
imageapi_imagemagick.module
* new function _imageapi_imagemagick_exec() takes some of the functionality of _imageapi_imagemagick_convert_exec() and is called by _imageapi_imagemagick_convert_exec() and _imageapi_imagemagick_identify_exec() to run convert and identify binaries respectively
Comment #7
egfrith CreditAttribution: egfrith commentedComment #8
drewish CreditAttribution: drewish commentedthanks for taking this up. i noticed a couple of problems with the current patch:
- imageapi_*image_get_info() needs PHPDocs for their parameters.
- imageapi_image_get_info() calls imageapi_image_open() but never closes the file which will lead to a memory leak in resource handles. This might be kind of tricky since we want to close the resource without doing a save.
- Couldn't imageapi_gd_image_get_info() just call image_info() rather than duplicating its code?
Comment #9
egfrith CreditAttribution: egfrith commentedThanks for your review. Re your points:
1. I'm a bit confused about this as imageapi_*image_get_info() do have PHPDocs for their parameters in the patch. However, none of the other functions in imageapi_*.module do - perhaps I should remove the PHPDocs? I've not changed anything in the attached patch.
2. Yes, this is a bit messy. I was trying to avoid duplicating code that is already in imageapi_image_open(). In the attached revised patch I do duplicate some of the code, but this avoids invoking the imageapi_*_image_open() functions.
3. I don't see why imageapi_gd_image_get_info() couldn't just call image_info() rather than duplicating its code. However, I suppose the reason I had duplicated the code was the thought that in drupal 7 imageapi is going to replace the current image functions. However I am a bit hazy about this, and in any case this is a patch for drupal 6 at present, so I can change this if you think it should definitely be changed.
Comment #10
drewish CreditAttribution: drewish commentedSo take a look at:
And show me where the @param tags for $file and $toolkit are ;)
Couldn't imageapi_gd_image_get_info() just end up like:
Comment #11
egfrith CreditAttribution: egfrith commentedThanks for the clarification. I've fixed the PHP docs now in imageapi.module now so that they have @params. (I note that image_get_info() doens't have any @param in it's documentation though..)
I've kept and corrected the PHP docs in imageapi_gd.module and imageapi_imagemagick.module, even though none of the other functions in these files have PHP docs.
I've made the change you suggested to imageapi_gd_image_get_info().
Finally, I've tried to address the slight duplication of code in the previous version of the patch by giving imageapi_image_get_info() an extra argument ($return_image_object) that allows it to return an object, and then using imageapi_image_get_info() to create the object in imageapi_image_open(). I think this is a bit more elegant than the previous patch.
Oh, and I've also made sure that the file_size is returned in the imagemagick version of imageapi_image_get_info(). It wasn't in the previous patch.
Comment #12
drewish CreditAttribution: drewish commentedYou're totally right on image_get_info() so I opened up #438810: image_get_info() missing @param doc and has bad parameter name.
I'm not certain about the other changes to imageapi_image_get_info()... I'm a bit tired right now so I can't really offer any constructive criticism but it feels like it like it's tangling it up so that open doesn't really open and get info doesn't just get info. I'll try to come up with something more substantial tomorrow.
Comment #13
egfrith CreditAttribution: egfrith commentedGood to have got the bug in core fixed. I suppose in the next iteration of the patch we should use $filepath instead of $file.
I agree about what you're saying about tangling things up. After having slept on the current patch, I really don't like the idea of a function which returns different things depending on a parameter. It seems to me that either we go with something like the previous version (at comment #9) , where there was a bit of duplicated code, or we create a function that just returns an image object without the info.
This could then be called both by imageapi_image_open() and imageapi_image_get_info(), and imageapi_image_get_info() could be just return the info given a $filepath and $toolkit.
Perhaps there is a better name for this function? e.g. imageapi_image_create_object() or _imageapi_image_create_object() or _imageapi_image_pre_open()?
Comment #14
egfrith CreditAttribution: egfrith commentedI'm wondering if it would make sense to move this issue to core, re-roll a version of this patch against core, and then, if and when the patch is accepted into core, move it back here? I suppose the rationale is that it would be nice to get this in core, and more eyes and brains might be able to come up with an elegant solution to the code organisation problems encountered here.
Comment #15
drewish CreditAttribution: drewish commentedthat seems reasonable, though it would be good to keep a patch against the D7 imagemagick toolkit so we can test it.
Comment #16
egfrith CreditAttribution: egfrith commentedI have now posted a patch against core at #269337: Support for more image types (PDF, TIFF, EPS, etc.). I'm working on a complementary patch to version 7.x of imageapi. This would just modify imageapi_imagemagick.module, and be very similar to the imagemagick part of the patch here. However, I'm not sure how to test it - would the core simpletests work, or do they set the toolkit to gd automatically?
Comment #17
egfrith CreditAttribution: egfrith commentedHere is a more straightforward patch against 6,x. It is less contorted than the last patch on this thread, and based on the patch on the patch to core at #269337: Support for more image types (PDF, TIFF, EPS, etc.). The key difference is that imageapi_gd_image_get_info() and imageapi_imagemagick_image_get_info() both return file_size in their array, whereas in the core version the equivalent functions don't, file_size being determined in the top-level function. This is because of using image_get_info() in this patch, which returns file_size by default, so it would seem wasteful to call @filesize in imageapi_get_info() too.
I am still aiming to get the patch for imageapi.imagemagick.inc for 7.x ready, but wanted to get things working in 6.x first.
Comment #18
egfrith CreditAttribution: egfrith commentedOoops. Last patch wasn't against that latest 6.x dev version - this one is.
Comment #19
egfrith CreditAttribution: egfrith commentedAttaching a patch which combines the patch here with an updated version of the patch at #375218: Changing file type with imagemagick. This is for the convenience of people at #366373: PDF support ( i.e. convert PDF to JPG support) rather than for review. At some point I will try to update the corresponding patch for imagemagick in D7 at #269329: ImageMagick support for more image types.
Comment #20
PolDude, your patch works perfectly good.
I made a imagebank for a customer and they sometimes upload tif pictures.
Thanks to your patch, I can now make a preview of those files!!! THANKS !
All I hope is that it get soon included in a stable version and maybe the ability to change the filename according to the extension!
Keep on the good work!
Comment #21
drewish CreditAttribution: drewish commentedOn the whole this looks really good. Mostly nit-picky stuff so this should be pretty easy to get committed.
There's a double space before standalone:
I'd love a comment in imageapi_imagemagick_image_close() explaining why you're looking at the type.
imageapi_imagemagick_image_get_info() could use better PHPDoc comments explaining that it's using the identify binary. Also "Imagemagick supports a number of file formats." isn't really that helpful. It could also use some inline comments explaining what's going on with it. I'm guessing "%w %h %m" is width, height, mimetype but please spell that out and if possible provide a link to the docs where they're defined.
I know that _imageapi_imagemagick_convert() and _imageapi_imagemagick_convert_exec() don't currently have PHPDocs for their parameters but since you're adding $type we should go ahead and fill those out. Same goes for _imageapi_imagemagick_identify_exec() and _imageapi_imagemagick_exec().
Comment #22
egfrith CreditAttribution: egfrith commentedI agree with all the points - nit-picking is good! I've done some work on the docs, but they're not finished yet. I hope to have a new patch ready some time early next week.
Comment #23
drewish CreditAttribution: drewish commentedgreat, this should be a pretty quick commit once that's done.
Comment #24
egfrith CreditAttribution: egfrith commentedIn this patch I think I've addressed all the points raised at #21, and documented a few extra functions too. I also changed some instances of $dst and $dest to $destination.
Comment #25
Rj-dupe-1 CreditAttribution: Rj-dupe-1 commentedWhat happened to the review? It seems it didn't end up being a quick commit after all...