Opened 12 years ago
Closed 12 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)
Change History (32)
@
12 years ago
Solution 2: Animated GIF's and all other images will not be handled by imagick but GD instead
#3
@
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.
#5
@
12 years ago
We use setIteratorIndex() as a fix for when resizing gif animation.
So what happens if you don't call it?
#6
@
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:
↓ 9
↓ 12
@
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
@
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
#9
in reply to:
↑ 7
@
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
#11
follow-up:
↓ 13
@
12 years ago
this should help out the second problem better. Instead of callable checking on the version.
#12
in reply to:
↑ 7
@
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
@
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
@
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.
@
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
@
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' ).
#16
@
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:
↓ 18
@
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
@
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.
@
12 years ago
Reject if setIteratorIndex and mime type isn't jpeg, and check in supports_mime_type. Split Imagick checks for clarity.
#20
@
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:
↓ 23
@
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//
.
#23
in reply to:
↑ 21
@
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.
- ImagickPixel now checked once.
- Imagick::queryFormats() checked properly again
- Inline comments style change as requested.
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