WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#22308 closed defect (bug) (fixed)

Require at least PHP Imagick 2.1.1 (Imagick::queryFormats() called statically)

Reported by: mpvanwinkle77 Owned by: ryan
Milestone: 3.5 Priority: normal
Severity: major Version: 3.5
Component: Media Keywords: has-patch
Focuses: Cc:
PR Number:

Description

Media Uploader throws error on server where Imagick is enabled:

"PHP Fatal error: Non-static method Imagick::queryformats() cannot be called statically, assuming $this from incompatible context in /../..wp-includes/class-wp-image-editor-imagick.php on line 153"

Attachments (6)

class-wp-image-editor-imagick.php.diff (623 bytes) - added by mpvanwinkle77 7 years ago.
22308-better-test.diff (806 bytes) - added by markoheijnen 7 years ago.
Check callable in test
22308-better-test.2.diff (548 bytes) - added by mikeschroder 7 years ago.
Refreshed Marko's Patch per the revert.
22308-better-test.3.diff (743 bytes) - added by mikeschroder 7 years ago.
Now with comments!
22308.version-check.diff (1.3 KB) - added by mikeschroder 7 years ago.
22308.diff (1.3 KB) - added by nacin 7 years ago.

Download all attachments as: .zip

Change History (30)

#1 @dotben
7 years ago

  • Cc dotben added

#2 @SergeyBiryukov
7 years ago

  • Component changed from General to Media
  • Milestone changed from Awaiting Review to 3.5

#3 @mpvanwinkle77
7 years ago

FYI, installing Imagick module v3.0 ( apt-get install php5-imagick ) resolves the issue as well ... but the patch is probably good for backwards compatibility. And if the new media functions require a certain version of the imagick extension then the WP_Image_Editor_Imagick::test() should be updated. Thoughts?

#4 @scribu
7 years ago

  • Cc scribu added

#5 @boogah
7 years ago

  • Cc boogah added

#6 @markoheijnen
7 years ago

It wasn't that we do some kind of versioning. The problem relies in documentation and changes that are made in the imagick library. There isn't a clear documentation for that.

Testing of the library is something I work on but that is something that will happen in the next week or so.

#7 @mpvanwinkle77
7 years ago

"The problem relies in documentation and changes that are made in the imagick library. There isn't a clear documentation for that. "

Agreed, I could find very little documentation myself.

#8 @nacin
7 years ago

You can continue to call a static method non-statically, so this switch at least avoids a fatal error. If we need to do version-checking once we know more, we can.

#9 @ryan
7 years ago

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

In 22421:

Call Imagick::queryformats() non-statically to preserve back compat with older versions of Imagick.

Props mpvanwinkle77
fixes #22308
see #6821

#10 @markoheijnen
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

We need to revert it and use a version check since supports_mime_type() need to be static

#11 follow-up: @nacin
7 years ago

http://comments.gmane.org/gmane.comp.php.pecl.cvs/8475

I think we can use is_callable( 'Imagick', 'queryFormats' ). However, if it is not callable in static context, what do we do? We would have to instantiate a bogus Imagick object (we could easily load in a tiny file in wp-includes/images if we had to), or use a hard-coded array of the basic types (png, jpg, gif) that we know will be supported, or we can actually have test() fail and have older versions of Imagick fail.

@markoheijnen
7 years ago

Check callable in test

#12 @markoheijnen
7 years ago

Something like this? I was planning to test it with at least Imagick version 2.2 but installing that seems harder then I thought.

#13 @markoheijnen
7 years ago

We can think about a version check with phpversion( 'imagick' ) but I'm unsure about that.

#14 @nacin
7 years ago

In 22549:

Revert [22421]. see #6821. see #22308. see #22419.

#15 @nacin
7 years ago

#22419 was marked as a duplicate.

#16 in reply to: ↑ 11 @mikeschroder
7 years ago

  • Cc mike.schroder@… added

Replying to nacin:

http://comments.gmane.org/gmane.comp.php.pecl.cvs/8475

I think we can use is_callable( 'Imagick', 'queryFormats' ). However, if it is not callable in static context, what do we do? We would have to instantiate a bogus Imagick object (we could easily load in a tiny file in wp-includes/images if we had to), or use a hard-coded array of the basic types (png, jpg, gif) that we know will be supported, or we can actually have test() fail and have older versions of Imagick fail.

Since it was fixed over four years ago, I'm personally fine with just requiring "new" versions of Imagick ( >= 2.2.0 ) with the is_callable check. It would definitely be preferred to avoid instantiating an object with an image just to check whether the format is supported.

@mpvanwinkle77 Which version of Imagick and provider are you currently using? Just checking to see, since we wouldn't want to miss a large base that has the older version.

#17 @nacin
7 years ago

Yep, we can probably toss the is_callable() check into test(), and just not use older versions of IM.

@mikeschroder
7 years ago

Refreshed Marko's Patch per the revert.

#18 @mikeschroder
7 years ago

Agreed. To double-check, did some testing with Imagick 2.1.1 and it seems to work as expected.

Refreshed markoheijnen's previous patch above, so that it applies cleanly after the revert.

#19 @nacin
7 years ago

  • Summary changed from [3.5-beta2-22315] Media Uploader throws error on server where Imagick is enabled: to Require at least PHP Imagick 2.1.1 (Imagick::queryFormats() called statically)

@mikeschroder
7 years ago

Now with comments!

#20 @nacin
7 years ago

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

In 22581:

Require Imagick 2.1.1 or greater, based on whether the queryFormats() method can be called statically.

props markoheijnen, DH-Shredder. fixes #22308.

#21 @mikeschroder
7 years ago

  • Keywords needs-patch added; has-patch needs-testing dev-feedback removed
  • Resolution fixed deleted
  • Severity changed from normal to major
  • Status changed from closed to reopened

Looks like this is still a problem:
http://wordpress.org/support/topic/fatal-error-with-imagemagick-and-media-editor

At the very least, we should have been running:
! is_callable( array( 'Imagick', 'queryFormats' ) ) or ! is_callable( 'Imagick::queryFormats' )

However, it looks like, per the notes, neither of these are sufficient to tell whether or not it's statically callable:
http://www.php.net/manual/en/function.is-callable.php

A direct version check or possibly ReflectionClass are two potential solutions.
Although the safest at this point would probably be to do a version check to 2.1.1 or greater.

If no one gets to it before me, I'll compile up 2.1.0 in the morning (when I'm slightly more awake) to test this further and write up a patch.

#22 @mikeschroder
7 years ago

  • Keywords has-patch added; needs-patch removed

In 22308.version-check.diff​ Add a direct version check.

Went back and checked through the code again, it does look like we need 2.2.0 or greater, indeed.

To test, installed both 2.1.1 and 2.2.0.

This patch was properly causing the editor to fall back to GD under 2.1.1 and using Imagick in 2.2.0.

If we want to get rid of more checks later, we can -- but this patch assumes better safe than sorry, in particular because no matter the version, the user can compile it with an old version of ImageMagick and get rid of some of the functions we need.

@nacin
7 years ago

#23 @mikeschroder
7 years ago

I'm fine with 22308.diff.

Tested; looks good.

#24 @nacin
7 years ago

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

In 22904:

Explicitly check for version 2.2.0 or higher of the Imagick PHP module, to ensure we can call queryFormats() statically. props DH-Shredder, fixes #22308.

Note: See TracTickets for help on using tickets.