Imagick HHVM compatibility

[DuckDagobert]

Hey,

I am using

Array
(
    [versionNumber] => 1655
    [versionString] => ImageMagick 6.7.7-10 2014-03-08 Q16 http://www.imagemagick.org
)

in /wp-includes/class-wp-image-editor-imagick.php line 57-75 you list the required methods WP needs in imagick to later check using array_diff.
The problem is that you use old capitalisation e.g. getimage but Imagick lately (since at least 1 year already) uses getImage, writeImage,... so this will cause that imagick is never used.

Array
(
    [3] => getimage
    [4] => writeimage
    [5] => getimageblob
    [6] => getimagegeometry
    [7] => getimageformat
    [8] => setimageformat
    [9] => setimagecompression
    [10] => setimagecompressionquality
    [11] => setimagepage
    [12] => scaleimage
    [13] => cropimage
    [14] => rotateimage
    [15] => flipimage
    [16] => flopimage
)

As I saw activity in here: #21668 only a couple hours ago by an admin after years of inactivity but dont know how to get into the slack conversation, I opened a ticket here. Also it would be nice if you would finally support progressive jpegs as stated in that ticket.

[markoheijnen]

What is your version of Imagick. With version 3.3.0 everything is still lowercase for me.

[DuckDagobert]

I am on HHVM and just updated to the latest version of Imagick yesterday (but capitalization was oon the version I had before too, which was around 7 months old) (​https://github.com/facebook/hhvm/tree/master/hphp/runtime/ext/imagick)

I also checked on stackoverflow, phpimagick,... and they all use Capitalized commands too.

[markoheijnen]

That makes a lot of sense. Thats not the original Imagick but adjusted version for HHVM. I guess this is the risk of using HHVM. The commands are capitalized as getImage but get_class_methods( 'Imagick' ) returns it as getimage.

[markoheijnen]

Btw, I added a patch and was wondering if you could test this one?

[kirasong]

It looks like 35973.patch​ is ~19ms (on shared) slower than current trunk, so we probably want to see if there's an alternate way to do this comparison.

[markoheijnen]

Thats not that bad for resizing images ;). 2nd patch would fix that.

[kirasong]

@DuckDagobert Renamed the ticket so that the primary issue you found (with HHVM) is clear, and this doesn't get missed in 4.5.

I would encourage continued discussion on progressive JPEGs on #21668.

[kirasong]

This is on my list today for deeper testing, and commit if it looks good.

[kirasong]

In 36916:

Media: Support Imagick in HHVM.

Removes case-sensitivity from Imagick feature detection within WP_Image_Editor_Imagick::test().
This allows correct detection of Imagick support within HHVM.

Props markoheijnen, DuckDagobert.
Fixes #35973.

[kirasong]

Hmm, while I saw it working during testing, this introduces two failing Imagick unit tests for HHVM on Travis:
​https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/114952704#L1184

Going to see what I can do to get unit tests running on HHVM on this side, but any digging into this is appreciated.

[kirasong]

Don't use HHVM when loading image file from URL.

[kirasong]

Imagick in HHVM fails when trying to load a file from a URL.

35973.3.patch​ works around this by failing test() when a URL is being loaded, causing WordPress to choose the "next best" editor (which is usually GD, which does load URLs in HHVM).

This fixes the test failure for Tests_Image_Functions::test_wp_crop_image_url().

[markoheijnen]

Looks good

[kirasong]

Reported upstream here: ​https://github.com/facebook/hhvm/issues/6908

[kirasong]

In 36996:

Media: Fall back to GD when loading URL in HHVM Imagick.

HHVM does not currently support loading URLs in its port of Imagick.
Fixes test_wp_crop_image_url() failure introduced in [36916].

See #35973.

[kirasong]

What's left: Still trying to reproduce the test_image_preserves_alpha_on_rotate() failure.

When testing with a local HHVM 3.6.6 (with the same build hash), it does not fail.

[kirasong]

Opened #36311 to track test_image_preserves_alpha_on_rotate() test failure.

Closing this out, since HHVM Imagick detection is fixed.