[wp-trac] [WordPress Trac] #42663: Imagick support for stream wrappers

[WordPress Trac]

#42663: Imagick support for stream wrappers
-------------------------------------------------+-------------------------
 Reporter:  calin                                |       Owner:
                                                 |  mikeschroder
     Type:  enhancement                          |      Status:  assigned
 Priority:  normal                               |   Milestone:  5.6
Component:  Media                                |     Version:
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch 2nd-opinion has-unit-      |     Focuses:
  tests                                          |
-------------------------------------------------+-------------------------

Comment (by mikeschroder):

 > 1. should I add support for very old versions of the imagick extension
 as @calin's patch did?  Pros: works with old PHP; Cons: adds some code
 bloat.

 I did a bit of research here (both on @calin and your proposed Imagick
 functions to use).
 After that, I’m not clear which versions of the Imagick extension we’d be
 dropping if we go with the approach in the PR, as the new function,
 `readImageBlob`, seems to be available from 2.0.0.

 A couple of datapoints to help the discussion:
 - Right now, [https://github.com/WordPress/wordpress-
 develop/blob/master/src/wp-includes/class-wp-image-editor-imagick.php#L35
 2.2.0 is required].

 - This might not be representative, but as a spot check, looks like the
 majority of [https://make.wordpress.org/hosting/test-results/ recent test
 reporters] that support Imagick have 3.4.4.

 I'm guessing I'm missing something here -- Could you please explain what
 you've found so far?


 > 2. should I try and call `make_image` from `_save` like @calin's patch
 did.  Pros: ??; Cons: convoluted code.

 `make_image()` was added to the class specifically to separate file stream
 concerns, so that's the reason I gave the earlier feedback.

 As an example, [https://github.com/WordPress/wordpress-
 develop/blob/master/src/wp-includes/class-wp-image-editor-gd.php#L508 this
 is how GD handles it].

 Would you mind going into a little detail on why you think it's a better
 not to use it in this case?


 > 3. should I add a dummy stream wrapper for testing like @calin's patch
 did instead of testing with `file://` URLs?  Pros: more likely to pick up
 future bugs if the code relies on features like seekable streams; Cons:
 bunch of extra code in the tests for the fake stream.

 I think that the more tests the better if it will improve coverage, since
 this is a less commonly used feature that has had a series of bugs. But of
 course, any tests you have the time to include are appreciated.

 Thanks again!

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/42663#comment:21>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform