Add equivalent of image_get_info() at the toolkit level

I'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().

Here 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

• New API function imageapi_image_get_info() to act as replacement for image_get_info(). This function extracts info using the selected toolkit.
• This function is used both externally (in which case its first argument should be a file string) and internally (in imageapi_image_open() and imageapi_image_close()), in which case its first argument should be an image object. If a file string is given to imageapi_image_get_info() it gets the information by calling imageapi_image_open(), which calls imageapi_image_get_info() with an image argument. This is perhaps a bit messy, but I can't see how to get round it, apart from creating a separate internal function (e.g. imageapi_image_get_info_image()) which would take the $image arg.
• Calls to image_get_info() replaced by imageapi_image_get_info()

imageapi_gd.module

• imageapi_gd_image_get_info() basically copy of image_get_info()

imageapi_imagemagick.module

• imageapi_imagemagick_image_get_info() requires the imagemagick "identify" binary
• "-append" arguemnt added for multilayer images
• "[0]" appended to source file given to convert binary so that multipage PDF files can be converted
  * 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

Thanks 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.

Thanks 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.

Good 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.

function imageapi_image_pre_open($filepath, $toolkit = FALSE) {
  if (!$toolkit) {
    $toolkit = imageapi_default_toolkit();
  }
  if ($toolkit) {
    $image = new stdClass();
    $image->source = $filepath;
    $image->toolkit = $toolkit;
  }
  return $image; 
}

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()?

I'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.

I 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?

Here 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.

Ooops. Last patch wasn't against that latest 6.x dev version - this one is.

Attaching 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.

I 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.

In 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.