_imageapi_imagemagick_convert_exec() currently considers that anything that convert outputs in stderr is an error that should be reported (in red) to any users, including anonymous.

That's wrong for two reasons:

  • convert outputs warning to stderr in some conditions, for example when parsing non-totally-compliant images. Those warnings don't result in the conversion failing, and they should not be displayed
  • displaying error messages to every visitors, without a way to remove them, is a bad practice
CommentFileSizeAuthor
#8 imageapi-imagemagick_1.diff1.21 KBmikejoconnor
#5 imageapi-imagemagick.diff1.21 KBmikejoconnor
#4 imageapi-imagemagick.diff944 bytesmikejoconnor
#3 803760-fixup.patch842 bytesdamien tournoud
#1 803760-error-display.patch1.64 KBdamien tournoud

Comments

damien tournoud’s picture

damien tournoud commented
Status: Active » Needs review
StatusFileSize
new 803760-error-display.patch1.64 KB

Here is what I suggest:

  • Rely solely on the return code to decide if stderr is an error or not
  • If we identified an imagemagick error, throw a PHP error. This error will be then caught by Drupal error handler, that will log it to watchdog and eventually display it to the user if configured to do so.
drewish’s picture

drewish commented
Status: Needs review » Fixed

Thanks, committed to HEAD and DRUPAL-6--1.

damien tournoud’s picture

damien tournoud commented
Status: Fixed » Needs review
StatusFileSize
new 803760-fixup.patch842 bytes

I got the condition upside down in this patch, sorry (last minute cleanups...).

mikejoconnor’s picture

mikejoconnor commented
StatusFileSize
new imageapi-imagemagick.diff944 bytes

DamZ:

That patch looks good, and solves the issue. I've attached an alternative, which does two things.

  1. It checks to make sure we have an error to report
  2. It adds the error code to the message
mikejoconnor’s picture

mikejoconnor commented
StatusFileSize
new imageapi-imagemagick.diff1.21 KB

Here is another patch, which cleans this up a bit more.

We always report the error code, however we add some logic to print the error message, if it exists.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

Tested, looks good to me.

drewish’s picture

drewish commented
Status: Reviewed & tested by the community » Needs work 
+        trigger_error(t("ImageMagick reported error code !code,\nMessage:\n!error", array('!code' => $return_code, '!error' => $errors)), E_USER_ERROR);

Seems like there should either be a period or Message should be lowercase.

mikejoconnor’s picture

mikejoconnor commented
Status: Needs work » Needs review
StatusFileSize
new imageapi-imagemagick_1.diff1.21 KB

I just realized this didn't make it in. Here's an update. replaced the , with a .

kaare’s picture

kaare
him/his
Primary language Norwegian Bokmål
Location Tromsø
commented

Yup. this patch works for me.

drewish’s picture

drewish commented
Status: Needs review » Fixed

Thanks, committed to HEAD and DRUPAL-6--1.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.