This module's imagemagick version detection suffers a similar bug to the imageapi bug located here:

The function _image_imagemagick_check_path() does not check if convert is usable, only if php can access it. PHP's open_basedir value can prevent php from reaching it, yet leave it available for use as a bash command.

I've attached a patch to adds the same version checking I suggested in the imageapi module.

To some degree, I don't like duplicating this check. I'd prefer to simply call the imageapi function that preforms the same thing, but I understand if you aren't interested in going that direction with the project.

CommentFileSizeAuthor
#15 970326-imagemagick-detection-bug-2.patch1.58 KBlance.gliser
#10 970326-imagemagick-detection-bug-2.patch1.58 KBlance.gliser
#7 970326-imagemagick-detection-bug.patch1.58 KBlance.gliser
#2 image_im_advanced-imagemagick-version-detection.patch1.09 KBlance.gliser
imagemagick-version-detection.patch656 byteslance.gliser

Comments

lance.gliser’s picture

lance.gliser commented

Sorry, forgot to give the imageapi issue link: http://drupal.org/node/608270

lance.gliser’s picture

lance.gliser commented
StatusFileSize
new image_im_advanced-imagemagick-version-detection.patch1.09 KB

Further along in my install, finding that the contributed image_im_advanced module needs a patch as well, as it tries to open the convert file to check for existence as well. This patch changes it to rely on the previously patched function in the image module.

Status: Needs review » Needs work

The last submitted patch, image_im_advanced-imagemagick-version-detection.patch, failed testing.

joachim’s picture

joachim commented

Two patches appear to be different.

Could you submit one patch which covers all the changes related to this bug report please?

Also, note that I don't have ImageMagick installed -- this patch will need other users to review it.

lance.gliser’s picture

lance.gliser commented
Version: 6.x-1.x-dev » 6.x-1.0
StatusFileSize
new 970326-imagemagick-detection-bug.patch1.58 KB

I grabbed the latest recommended release and rerolled the patch to include both file's changes in one patch. Previous patches included my project root, my apologies. This patch is rooted in the image module.

lance.gliser’s picture

lance.gliser commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 970326-imagemagick-detection-bug.patch, failed testing.

lance.gliser’s picture

lance.gliser commented
StatusFileSize
new 970326-imagemagick-detection-bug-2.patch1.58 KB

Fah, improper line endings. My apologies. Resubmitting

lance.gliser’s picture

lance.gliser commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 970326-imagemagick-detection-bug-2.patch, failed testing.

lance.gliser’s picture

lance.gliser commented

Someone might have to help me out. I'm not sure I understand what drupal QA is upset about. The error message they give for the patch is:
[10:25:15] Invoking operation [check]...
[10:25:15] Encountered error on [check], details:
array (
'@filename' => '970326-imagemagick-detection-bug-2.patch',
)

joachim’s picture

joachim commented

The details for #10 say:

Detect invalid patch format
Ensure the patch only contains unix-style line endings

lance.gliser’s picture

lance.gliser commented
Status: Needs work » Needs review
StatusFileSize
new 970326-imagemagick-detection-bug-2.patch1.58 KB

I found my SVN was converting to platform correct line endings. Might have been the cause. I ran through the Eclipse for windows setup again, and resaved the file. If this doesn't create a proper line endings, I'm at a loss.

Status: Needs review » Needs work

The last submitted patch, 970326-imagemagick-detection-bug-2.patch, failed testing.

joachim’s picture

joachim commented

~/Sites/_sandbox/image-DRUPAL-6--1 joachim$ patch -p0 < 970326-imagemagick-detection-bug-2_0.patch
(Stripping trailing CRs from patch.)
patching file image.imagemagick.inc
(Stripping trailing CRs from patch.)
patching file contrib/image_im_advanced/image_im_advanced.install

It's still the line endings I'm afraid...

Also:

+++ image.imagemagick.inc	(working copy)
@@ -52,6 +52,13 @@
+  // Try it out and return without errors on success

Needs a full stop here.

Though more details on what 'it' is would be good too!

+++ image.imagemagick.inc	(working copy)
@@ -52,6 +52,13 @@
+  $handle = popen($path . " -version", 'r');

Would single quotes do here?

Powered by Dreditor.

lance.gliser’s picture

lance.gliser commented
Status: Needs work » Needs review

Dreditor looks like a fantastic tool. Thanks for the heads up. Something's wrong with my patch creation through eclipse. I'll have to keep trying to sort it out, and Dreditor might make that easier.

The single quote you asked about isn't my code. The developer that wrote the bit I'm trying to patch in for you gets all the credit (linked at the top of this issue, to another issue where he proposes it for imageapi.module). I see no reason it couldn't be unified.

I'm going to hand this one off to you, or anyone else what wants to copy the patch with proper line endings. Going to deal with my own eclipse issues for now.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Closed (duplicate)

Thanks for taking the time to report this issue, and especially for providing a patch!

However, marking as duplicate of #613814: Convert not found when ImageMagick is in the path. You can follow up on that issue to track its status instead. If any information from this issue is missing in the other issue, please make sure you provide it over there.