Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
_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
Comment | File | Size | Author |
---|---|---|---|
#8 | imageapi-imagemagick_1.diff | 1.21 KB | mikejoconnor |
#5 | imageapi-imagemagick.diff | 1.21 KB | mikejoconnor |
#4 | imageapi-imagemagick.diff | 944 bytes | mikejoconnor |
#3 | 803760-fixup.patch | 842 bytes | Damien Tournoud |
#1 | 803760-error-display.patch | 1.64 KB | Damien Tournoud |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere is what I suggest:
stderr
is an error or notComment #2
drewish CreditAttribution: drewish commentedThanks, committed to HEAD and DRUPAL-6--1.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedI got the condition upside down in this patch, sorry (last minute cleanups...).
Comment #4
mikejoconnor CreditAttribution: mikejoconnor commentedDamZ:
That patch looks good, and solves the issue. I've attached an alternative, which does two things.
Comment #5
mikejoconnor CreditAttribution: mikejoconnor commentedHere 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.
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedTested, looks good to me.
Comment #7
drewish CreditAttribution: drewish commentedSeems like there should either be a period or Message should be lowercase.
Comment #8
mikejoconnor CreditAttribution: mikejoconnor commentedI just realized this didn't make it in. Here's an update. replaced the , with a .
Comment #9
kaareYup. this patch works for me.
Comment #10
drewish CreditAttribution: drewish commentedThanks, committed to HEAD and DRUPAL-6--1.