Make WordPress Core

Opened 12 years ago

Closed 12 years ago

#22543 closed defect (bug) (fixed)

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

Reported by: ocean90's profile 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 12 years ago.
Solution 1: Animated GIF's still will be manipulate by imagick
22543-2.diff (614 bytes) - added by markoheijnen 12 years 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 12 years ago.
Check on version
22543-4.diff (753 bytes) - added by markoheijnen 12 years 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 kirasong 12 years ago.
Initial draft after audit -- check all the functions.
22543-6.diff (2.0 KB) - added by kirasong 12 years 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 kirasong 12 years ago.
Make comments use ; Only check ImagePixel once, check queryFormats() again.

Download all attachments as: .zip

Change History (32)

#1 @richardmtl
12 years ago

  • Cc richardmtl added

#2 @markoheijnen
12 years 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

@markoheijnen
12 years ago

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

@markoheijnen
12 years ago

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

#3 @markoheijnen
12 years 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.

#4 @markoheijnen
12 years ago

  • Keywords has-patch added

#5 @scribu
12 years ago

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

So what happens if you don't call it?

#6 @markoheijnen
12 years 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.

#7 follow-ups: @nacin
12 years 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.

#8 @markoheijnen
12 years 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

Last edited 12 years ago by markoheijnen (previous) (diff)

#9 in reply to: ↑ 7 @ocean90
12 years 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

#10 @markoheijnen
12 years ago

Saw that one but that is just weird.

@markoheijnen
12 years ago

Check on version

#11 follow-up: @markoheijnen
12 years ago

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

#12 in reply to: ↑ 7 @kirasong
12 years 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.

#13 in reply to: ↑ 11 @nacin
12 years 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.

#14 @nacin
12 years 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.

@markoheijnen
12 years ago

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

#15 @markoheijnen
12 years 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' ).

@kirasong
12 years ago

Initial draft after audit -- check all the functions.

#16 @kirasong
12 years ago

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

#17 follow-up: @dfavor
12 years ago

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

#18 in reply to: ↑ 17 @kirasong
12 years 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.

#19 @ryan
12 years ago

In 22849:

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

Props DH-Shredder, markoheijnen
see #22543

@kirasong
12 years ago

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

#20 @kirasong
12 years 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.


#21 follow-up: @nacin
12 years 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 //.

#22 @nacin
12 years ago

  • Severity changed from blocker to normal

@kirasong
12 years ago

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

#23 in reply to: ↑ 21 @kirasong
12 years 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.

#24 @ryan
12 years ago

In 22864:

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

#25 @nacin
12 years 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.