WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

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

Download all attachments as: .zip

Change History (32)

#1 @richardmtl
3 years ago

  • Cc richardmtl added

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

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

@markoheijnen
3 years ago

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

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

  • Keywords has-patch added

#5 @scribu
3 years ago

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

So what happens if you don't call it?

#6 @markoheijnen
3 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
3 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
3 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

Version 0, edited 3 years ago by markoheijnen (next)

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

Saw that one but that is just weird.

@markoheijnen
3 years ago

Check on version

#11 follow-up: @markoheijnen
3 years ago

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

#12 in reply to: ↑ 7 @mikeschroder
3 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
3 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
3 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
3 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
3 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' ).

@mikeschroder
3 years ago

Initial draft after audit -- check all the functions.

#16 @mikeschroder
3 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
3 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 @mikeschroder
3 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
3 years ago

In 22849:

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

Props DH-Shredder, markoheijnen
see #22543

@mikeschroder
3 years ago

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

#20 @mikeschroder
3 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
3 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
3 years ago

  • Severity changed from blocker to normal

@mikeschroder
3 years ago

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

#23 in reply to: ↑ 21 @mikeschroder
3 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
3 years ago

In 22864:

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

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