Add support for changing destination image format

A magic colon is not really clean. Do you have any other approaches or ideas?

I can only guess we need to use a non-standard ->ops value or extension.

+++ b/imagemagick.module
@@ -372,6 +372,15 @@ function _imagemagick_convert($source, $dest, $args) {
   drupal_alter('imagemagick_arguments', $args, $context);

I also wonder whether such kind of an adjustment shouldn't be done via hook_imagemagick_arguments_alter() ?

For the drupal_alter hooks to be useful for image format conversion via either method described above, the _imagemagick_convert (or possibly image_imagemagick_save) functions would still need to be patched to use the possibly altered value of $context->destination instead of $destination after the drupal_alter calls.

I was under the impression that this is already possible and especially the 'destination' context variable would be used.

Attached patch allows that. However, we need to resolve the @todo I've added.

+++ b/imagemagick.module
@@ -359,28 +359,35 @@ function image_imagemagick_get_info(stdClass $image) {
+    'destination_original' => $destination_original,
...
-  return file_exists($dest);
+  // @todo Fails if $destination is altered into e.g. png:myimage.jpg :(
+  return file_exists($destination);

We could test for $destination_original instead. However, that would prevent people from using ImageMagick as a pure API module.

An alternative would be to add yet another 'destination_filename' variable to $context that would allow developers to change 'destination', but leave 'destination_filename' intact, or in case they are purposively changing the filename, they'd also have to change 'destination_filename' to make the file_exists() test pass.

If there is only the [extension]:[filename] syntax for the format conversion (without further edge-cases), then a 'destination_format' or just simply 'format' would work. This would have to be documented. Do you have ImageMagick API docs pointers for this?

Sorry, I meant in the official vendor API docs of ImageMagick. I didn't know of the extension: prefix for format conversion.

In that case, I'd say let's go with a new 'destination_format' parameter in $context, defaulting to an empty string, and if filled in by an alter hook with an extension, the extension, plus a colon, is prefixed to the $destination variable.

That way, an alter hook should be able to mimic and use the entire image save possibilities of ImageMagick by adjusting $context, even including the "ext:-" variant.

The code as well as the alter hook API phpDoc should contain pointers to http://www.imagemagick.org/Usage/files/#save

wow! Amazing!!! This patch is of excellent quality! :)

Seriously, where have you been all the time? I'd totally like to see more patches from you! :)

I'm curious if it might not be better to make the implementations of hook_imagemagick_arguments responsible for doing the prepending of the image format to the destination and returning the "format:destination_path" as the 'destination_format' of $context so a simpler test that doesn't muck with the $destination could be used within ImageMagick.

That wouldn't be a good idea. If you happen to know hook_form_alter() in D6, and if you ever tried to add a CSS class to a form element, or even better, if you ever tried to adjust the query string in $form_state['redirect'], then you basically know the problem: You've no effin' idea of whether the value is already in the format you're trying to alter it to, or whether a previous alter hook changed it into the format already.

In other words: If alter hook #1 changes 'destination' into "png:file.jpg", then alter hook #2 would change it to "jpg:png:file.jpg", unless you force all alter hooks to implement whack code to check what's actually contained in 'destination' first.

Or, to frame it as a more general question, should the implementation of a hook should be responsible for making sure altered values are sane before returning them, or is the module that invokes the hook responsible? [...] Should there be a test in ImageMagick for the validity of the destination, or should it be up to each implementation of a hook to be sure the destination it returns is valid?

That's a slightly different question than the above. In almost all cases, the common design pattern in Drupal is that alter hooks (actually any kind of function or callback) is expected to return valid data. If we'd introduce validation of returned values, then Drupal's codebase would likely be twice as large as it is today.

There are only very few exceptions to this in Drupal core, where super-essential data is affected. Don't have examples right now. But in general: No, the code that implements a hook is responsible for returning valid data.

So...

given your excellent patch, I only have very minor remarks:

+++ b/imagemagick.api.php
@@ -2,6 +2,44 @@
+function hook_imagemagick_load_alter(stdClass $image) {
...
+function hook_imagemagick_save_alter(stdClass $image, $context = array()) {

Yay! Thanks for those! Didn't know that the .api.php was still empty ;)

+++ b/imagemagick.api.php
@@ -2,6 +2,44 @@
+ *   - source_original: A string file URI of the original image.
+ *   - destination_original: A string file URI of the derivative image.
+ *   - source: A string filesystem path of the original image.
+ *   - destination: A string filesystem path of the derivative image.
+ *   - destination_format: A string format of the derivative image.

+++ b/imagemagick.module
@@ -490,6 +499,29 @@ function _imagemagick_convert_exec($command_args, &$output = NULL, &$error = NUL
+ * Destination paths must be prepended with a format in order for 'convert'
+ * to write the destination file in a different format than the source file.
+ *
+ * @see http://www.imagemagick.org/script/command-line-processing.php#output
+ * @see http://www.imagemagick.org/Usage/files/#save

For better readability, the context parameters should be ordered as in the actual PHP code; i.e., "source*" and "destination*" grouped together.

Also, the essential pointers and documentation about the 'destination_format' context parameter should be contained in the phpDoc of the alter hook. Developers expect to find this info in the API documentation in .api.php, not somewhere deeply hidden in the .module file ;)

+++ b/imagemagick.api.php
@@ -2,6 +2,44 @@
\ No newline at end of file

This shouldn't happen. ;)

+++ b/imagemagick.module
@@ -359,28 +359,37 @@ function image_imagemagick_get_info(stdClass $image) {
+  $command = escapeshellarg($source) . ' ' . implode(' ', $args) . ' ' . escapeshellarg(_imagemagick_destination($destination, $context['destination_format']));

@@ -490,6 +499,29 @@ function _imagemagick_convert_exec($command_args, &$output = NULL, &$error = NUL
+function _imagemagick_destination($destination, $format = NULL) {

Don't think we need a separate helper function for that. Let's do this directly within _imagemagick_convert().

Thanks for reporting, reviewing, and testing! Committed attached patch. (a couple of adjustments)

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Hi, I'm gathering from the above 'fixed' that we can now determine the output format (and file extension?) of an image processed with ImageMagick. How do I implement this? I'm using the module Raw ImageMagick to insert command options and currently trying "-format png" to convert JPG to PNG (so I can use transparency on uploaded images). How can I get this to work? The outputted files are still JPG and have no transparency. Thanks.

I'm using the latest Dev 7 version.. or will this only be 'officially' released?

@mgmhunt: Is it possible that you're mistaken about the purpose of the -format option? According to the details, it's supposed to be used with identify, not convert.

Yep very possible. The convert documentation states "The convert command recognizes these options. "... and then lists "-format string output formatted image characteristics" as an option. Which then leads to "%o output filename" or "%e filename extension or suffix". Which appears to only allow the printing of these attributes.

The ImageMagick documentation gives the example of "convert rose.jpg rose.png" as the method to convert the image format. But how can I do this with the Raw ImageMagick module? ie as a command line. As it seems to indicate ImageMagick uses the extension of the output file name to determine this. (which the module specifies not to do "Do not add an input or output file.")

+++ b/imagemagick.module
@@ -359,28 +359,47 @@ function image_imagemagick_get_info(stdClass $image) {

@see http://www.imagemagick.org/script/command-line-processing.php#output
@see hook_imagemagick_arguments_alter()

Thanks for the pointer. I'm still not sure what I need to type into the Raw ImageMagick module to effect the variables at "$destination_format .= ':' . $destination;". As this is called through multiple image styles, it must concat the arguments of all effects and then send them to this function. Would an argument of *.png in the Raw IM module coming last be OK? How do I reference the $destination_format via the effectmodule? Or is this outside the scope of the project and I should either override this programmatically or go to the Raw IM module forum?

Image style effects cannot access or alter the command arguments directly. They need to implement hook_imagemagick_arguments_alter() and (smartly) figure out whether they need to adjust the 'destination_format' in the alter hook.

Ideally, the IM Raw module project maintainers should be made aware of this issue to implement the proper logic into that module. So, yes, ideally post to the IM Raw module's issue queue (but search for existing issues first).

Thanks for your time. I'll progress this one on the other side! Cheers.

Addendum:

Have found a bit more info here http://drupal.org/node/628146#comment-4740296 "Some versions of Firefox and Safari do not view images whose format has been converted with the "Change File Format" action properly." and here http://drupal.org/node/219261#comment-4583488 "This is something that's come up in the im_raw issue a couple of times too, people not understanding why they can't 'change' format - they most likely have, but didn't see a changed file-extension."

So I guess there are issue with the Image Cache area...which is core in Drupal 7. Couldn't see anything in Drupal 7 image issue queue. http://drupal.org/project/issues/drupal?text=&status=Open&priorities=All...

Hello Sun,

i have installed the dev version of Imagemagick. I am looking for a solution to convert my files in .jpg format.
Apparently, i need to create a hook in my template.php.

I have created this hook :
function hook_imagemagick_arguments_alter($args, $context = array()) {
$context['jpg'];
}
or
function hook_imagemagick_arguments_alter($args, $context = array()) {
$context['.jpg'];
}

But, it's not functionnal.
Can you give an example of a hook with jpg format ?

Thanks a lot.