Convert not found when ImageMagick is in the path

Thanks for the patch :)

I don't have a system I can test imagemagick stuff on, so it's going to have to wait for someone who can test this.

Looks reasonable to a quick read, but a couple of points:
- is there a reason for your renaming _image_imagemagick_check_path() to remove the underscore? By convention the initial underscore that means it's a module-internal function. Renaming it in this patch only adds noise as far as I can tell.
- I'd appreciate a comment above this line: if (strcmp(dirname($path), ".") == 0) to say what you are testing. PHP file and string functions hurt my brain :(

I don't see a call to image_imagemagick_check_path() either with or without underscore in image_im_advanced_requirements() or anywhere in the file image_im_advanced.install.

Private functions in PHP are a convention, not a rule of the language. And to be honest, I don't know if 'private' means in the same module, or whether we can say within the image package counts as private. Logically I suppose we should mean just within the module, so your change removing the underscore is correct -- if I could find an external call to it!

Ok...
so you're changing what used to be an internal function to something that's sort of hook-like in that an invoke function will treat is as an implementation....
This sounds like a big change to me, but I've no idea what this really means as I've never used imagemagick.
So if a couple of people confirm this is good, I'll commit :)

We are about to roll a new release, so if this patch will be slightly revised ASAP, then it's going to have a good chance to be included.

+++ image.imagemagick.inc	25 Oct 2009 23:31:37 -0000
@@ -25,7 +25,7 @@ function image_imagemagick_settings() {
+    '#description' => t('Specify the path to the ImageMagic <kbd>convert</kbd> binary. For example: <kbd>/usr/bin/convert</kbd> or <kbd>C:\Program Files\ImageMagick-6.3.4-Q16\convert.exe</kbd> or simply <kbd>convert</kbd> if ImageMagic is in the path.'),

Apparently, many instances of "ImageMagic" that should be "ImageMagick" in here ;)

+++ image.imagemagick.inc	25 Oct 2009 23:31:37 -0000
@@ -39,7 +39,7 @@ function image_imagemagick_settings() {
-    $valid = _image_imagemagick_check_path($form_element['values']['image_imagemagick_convert'], 'image_imagemagick_convert');
+    $valid = image_imagemagick_check_path($form_element['values']['image_imagemagick_convert'], 'image_imagemagick_convert');

The function name and therefore API change doesn't seem to be required - can you revert that, please?

+++ image.imagemagick.inc	25 Oct 2009 23:31:37 -0000
@@ -51,10 +51,24 @@ function _image_imagemagick_build_versio
+  if (strcmp(dirname($path), ".") == 0) {
+    if (exec($path . " -version") !== FALSE) {

Why don't we simply compare

dirname($path) == '.'

?

At least, I don't understand the strcmp().

Furthermore, it looks like the second/inner condition could be added to the first.

+++ image.imagemagick.inc	25 Oct 2009 23:31:37 -0000
@@ -51,10 +51,24 @@ function _image_imagemagick_build_versio
+  else {
+    if (file_exists($path)) {

Looks like this elseif could be written on one line.

+++ contrib/image_im_advanced/image_im_advanced.install	25 Oct 2009 23:31:37 -0000
@@ -28,7 +28,7 @@ function image_im_advanced_requirements(
-    if (!file_exists($convert_path)) {
+    if (!image_toolkit_invoke("check_path", array($convert_path))) {

Always use single quotes (') unless double quotes are technically required.

Powered by Dreditor.

Subscribing

Moving to ImageMagick project.

Not completely happy with this yet, but it works.

Thanks for reporting, reviewing, and testing! Committed attached patch to HEAD.

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