Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#35973 closed defect (bug) (fixed)

Imagick HHVM compatibility

Reported by: duckdagobert's profile DuckDagobert Owned by: kirasong's profile kirasong
Milestone: 4.5 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords:
Focuses: Cc:

Description (last modified by SergeyBiryukov)

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.

Attachments (3)

35973.patch (608 bytes) - added by markoheijnen 8 years ago.
35973.2.patch (658 bytes) - added by markoheijnen 8 years ago.
35973.3.patch (602 bytes) - added by kirasong 8 years ago.
Don't use HHVM when loading image file from URL.

Download all attachments as: .zip

Change History (31)

#1 @SergeyBiryukov
8 years ago

  • Description modified (diff)

#2 @markoheijnen
8 years ago

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

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

#3 @DuckDagobert
8 years ago

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
8 years ago

#4 @markoheijnen
8 years ago

  • Keywords has-patch added

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.

#5 @markoheijnen
8 years ago

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

#6 @kirasong
8 years ago

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.

#7 @markoheijnen
8 years ago

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

#8 @kirasong
8 years ago

  • Milestone changed from Awaiting Review to 4.5
  • Summary changed from Imagick basic mistakes & progressive to Imagick HHVM compatibility

#9 @kirasong
8 years ago

@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.

#10 @kirasong
8 years ago

  • Owner set to mikeschroder
  • Status changed from new to assigned

#11 @kirasong
8 years ago

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

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


8 years ago

#13 @kirasong
8 years ago

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

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.

#14 @kirasong
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

This ticket was mentioned in Slack in #core-images by mike. View the logs.


8 years ago

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


8 years ago

@kirasong
8 years ago

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

#17 @kirasong
8 years ago

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().

This ticket was mentioned in Slack in #core-images by mike. View the logs.


8 years ago

#19 @markoheijnen
8 years ago

Looks good

#21 @kirasong
8 years ago

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.

#22 @kirasong
8 years ago

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.

#23 @kirasong
8 years ago

  • Keywords needs-patch added; has-patch removed

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


8 years ago

This ticket was mentioned in Slack in #core by mike. View the logs.


8 years ago

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


8 years ago

#27 @kirasong
8 years ago

  • Version changed from 4.4.2 to 3.5

#28 @kirasong
8 years ago

  • Keywords needs-patch removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Opened #36311 to track test_image_preserves_alpha_on_rotate() test failure.

Closing this out, since HHVM Imagick detection is fixed.

Note: See TracTickets for help on using tickets.