WordPress.org

Make WordPress Core

#22543 closed defect (bug) (fixed)

ImageMagick: Call to undefined method Imagick::setIteratorIndex()

Reported by: ocean90 Owned by:
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: has-patch commit
Focuses: Cc:

Attachments (7)

22543-1.diff (673 bytes) - added by markoheijnen 17 months ago.
Solution 1: Animated GIF's still will be manipulate by imagick
22543-2.diff (614 bytes) - added by markoheijnen 17 months ago.
Solution 2: Animated GIF's and all other images will not be handled by imagick but GD instead
22543-3.diff (627 bytes) - added by markoheijnen 17 months ago.
Check on version
22543-4.diff (753 bytes) - added by markoheijnen 17 months ago.
Add check if class exists and check if setIteratorIndex is callable when the mime_type is for GIF or not given
22543-5.diff (2.0 KB) - added by DH-Shredder 17 months ago.
Initial draft after audit -- check all the functions.
22543-6.diff (2.0 KB) - added by DH-Shredder 17 months ago.
Reject if setIteratorIndex and mime type isn't jpeg, and check in supports_mime_type. Split Imagick checks for clarity.
22543-7.diff (2.0 KB) - added by DH-Shredder 17 months ago.
Make comments use ; Only check ImagePixel once, check queryFormats() again.

Download all attachments as: .zip

Change History (32)

comment:1 richardmtl17 months ago

  • Cc richardmtl added

comment:2 markoheijnen17 months ago

There are two ways of solving this. Will create both patches as we speak.

We use setIteratorIndex() as a fix for when resizing gif animation. Do we still want to use Imagick or do we then go to GD. I would say the first

markoheijnen17 months ago

Solution 1: Animated GIF's still will be manipulate by imagick

markoheijnen17 months ago

Solution 2: Animated GIF's and all other images will not be handled by imagick but GD instead

comment:3 markoheijnen17 months ago

Two solution to handle this case. The version of ImageMagick is really old so I would love to see solution 1 that get's committed.

The weird thing is that the version of the TS was release in 2006 but CentOS ( no clue if he is using it) was even updating this version last year: http://pkgs.org/centos-5-rhel-5/centos-rhel-x86_64/ImageMagick-6.2.8.0-12.el5.x86_64.rpm.html. No clue why.

comment:4 markoheijnen17 months ago

  • Keywords has-patch added

comment:5 scribu17 months ago

We use setIteratorIndex() as a fix for when resizing gif animation.

So what happens if you don't call it?

comment:6 markoheijnen17 months ago

The function set's the frame. If I'm correct it would select the last frame instead of the first. GD also selects the first one and thats why it's there.

comment:7 follow-ups: nacin17 months ago

  • Severity changed from normal to blocker

As this is the second time we have gotten bit with an undefined method, this needs a full audit.

comment:8 markoheijnen17 months ago

22543-2.diff​ is the way to go for this. When you have that old version of Imagick then we just should skip it and use GD instead

Version 0, edited 17 months ago by markoheijnen (next)

comment:9 in reply to: ↑ 7 ocean9017 months ago

Replying to nacin:

As this is the second time we have gotten bit with an undefined method, this needs a full audit.

Another one: http://wordpress.org/support/topic/fatal-error-class-imagick-not-found?replies=2

comment:10 markoheijnen17 months ago

Saw that one but that is just weird.

markoheijnen17 months ago

Check on version

comment:11 follow-up: markoheijnen17 months ago

this should help out the second problem better. Instead of callable checking on the version.

comment:12 in reply to: ↑ 7 DH-Shredder17 months ago

  • Cc mike.schroder@… added

Replying to nacin:

As this is the second time we have gotten bit with an undefined method, this needs a full audit.

Completely agreed, and will work on this.
It's complicated by the lack of documentation on both the Imagemagick (library) and Imagick (PHP module) fronts, but should be doable.
To the Commit Logs!

markoheijnen: That's good, but we still want to be sure we're blocking on the earliest version of Imagemagick that we need as well. An audit will help that. I think we're close, though, as we don't use very many obscure functions.

comment:13 in reply to: ↑ 11 nacin17 months ago

Replying to markoheijnen:

this should help out the second problem better. Instead of callable checking on the version.

I don't understand why we'd want to do a version check — we should aim to do feature detection whenever possible. In this case, it is possible.

comment:14 nacin17 months ago

If test() handled mime types, we'd be able to exclude IM due to the lack of setIteratorIndex() but only for GIF images. A 2006 version of IM is still going to result in better JPG quality than a 2012 version of GD.

markoheijnen17 months ago

Add check if class exists and check if setIteratorIndex is callable when the mime_type is for GIF or not given

comment:15 markoheijnen17 months ago

The feature detection should be possible but for the second problem we are already doing so. So I added class_exists('Imagick') for that. Maybe extension_loaded() can now be removed.

Second stuff you are right so a check if the mime_type is given or not and when needed check is_callable( $this->image, 'setIteratorIndex' ).

DH-Shredder17 months ago

Initial draft after audit -- check all the functions.

comment:16 DH-Shredder17 months ago

Attached initial pass after audit. Will run some more tests when I'm a bit more awake in the morning.

comment:17 follow-up: dfavor17 months ago

It appears there should also be a check for MIME type image/png, as png images can be animated also.

comment:18 in reply to: ↑ 17 DH-Shredder17 months ago

Replying to dfavor:

It appears there should also be a check for MIME type image/png, as png images can be animated also.

This is true, but there are an infinite amount of potential types with more than one frame that ImageMagick can support.

The options are either, block for the ones we know will break and are readily supported by WordPress (which is your suggestion), or detect multi-frame files in a different way.

Seeing if there's a better way to handle the check.
The trick here is using functions that actually exist on prehistoric versions of ImageMagick.

comment:19 ryan17 months ago

In 22849:

More feature detection in WP_Image_Editor_Imagick::test().
Check existence of setIteratorIndex().

Props DH-Shredder, markoheijnen
see #22543

DH-Shredder17 months ago

Reject if setIteratorIndex and mime type isn't jpeg, and check in supports_mime_type. Split Imagick checks for clarity.

comment:20 DH-Shredder17 months ago

In 22543-6.diff​:

  • Move mime check to supports_mime_type()
  • Reject if setIteratorIndex isn't available, and the mime is anything other than image/jpeg.
  • Split Imagick checks in test() for clarity and bail early if the extension isn't loaded.


comment:21 follow-up: nacin17 months ago

  • Keywords commit added

Looks good. Two small notes:

  • ImagickPixel is checked twice.
  • Don't use /** for inline comments, it should be /*. In this case, for just two lines, consider starting both with //.

comment:22 nacin17 months ago

  • Severity changed from blocker to normal

DH-Shredder17 months ago

Make comments use ; Only check ImagePixel once, check queryFormats() again.

comment:23 in reply to: ↑ 21 DH-Shredder17 months ago

Replying to nacin:

Looks good. Two small notes:

  • ImagickPixel is checked twice.
  • Don't use /** for inline comments, it should be /*. In this case, for just two lines, consider starting both with //.

Thanks, I see what I did there.

22543-7.diff​:

  • ImagickPixel now checked once.
  • Imagick::queryFormats() checked properly again
  • Inline comments style change as requested.

comment:24 ryan17 months ago

In 22864:

Clean up WP_Image_Editor_Imagick::test(). Props DH-Shredder. see #22543

comment:25 nacin17 months ago

  • Resolution set to fixed
  • Status changed from new to closed

Think this is done. Someone re-open if I am incorrect.

Note: See TracTickets for help on using tickets.