[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 p00ya):

 Sorry for the delay in replying (replies below required a bit of fact-
 checking).

 Replying to [comment:21 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?

 I agree with your analysis.  readimageblob has been around since imagick
 2.0.0
 [https://github.com/Imagick/imagick/blob/d0f8197c32b2075adb2806d2c413efb091ef63de/imagick.c
 this commit].  When I asked if I should add compatibility I was only
 looking at differences with calin's patch, without comparing the imagick
 history and the min version already enforced by WordPress.  Nothing to be
 done here.

 >
 >
 > > 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?

 Sure (apologies if this comes across as blunt - it's just my opinion).

 `WP_Image_Editor::make_image($filename, $function, $arguments)`'s contract
 is something like:
  If $filename is a stream URL, call $function($arguments...).  $function
 should output an image, and `make_image` will take care of capturing it in
 a buffer and then writing it to $filename.

 This is an unfortunate API design: make_image is doing two things:
 abstracting files vs streams, and also doing the output buffering stuff.
 In the imagick case, the output buffering is unnecessary because we can
 get the bytes directly with `getImageBlob` and then naturally write them
 with PHP's `file_put_contents`.

 Comparing the code complexity:
 * calling `file_put_contents` is simpler than the `fopen`/`fwrite` logic
 in `make_image` for writing to a file/stream
 * setting up a callback for `make_image` is a bunch of boilerplate
 * calling `getImageBlob` is simpler than writing an image to output just
 so that it can be `ob_get_contents()` and put back in a string.
 * all this so that the image editor can move (not even eliminate, as
 `WP_Image_Editor_GD` demonstrates!) a single branch depending on whether
 the output is a stream URL.

 I am also suspicious about extra copies using `make_image` but I haven't
 done any profiling to demonstrate this.  While the extra copies are
 perhaps fanciful, memory usage being a concern is not: image operations
 are the only time I hit container limits (memory) on a cloud WordPress
 install.  Ideally we'd actually push the stream write into the imagick
 extension (where it only needs to buffer a block at a time), but as noted
 it's a bit buggy in this regard (eventually this might get fixed in
 imagick or ImageMagick proper).  In that case, we'd definitely want to
 skip `make_image`.

 If `make_image` had hooks then maybe there would be more of a case for
 unifying on it (maybe a plugin could use the hook to set ACLs or such, and
 we would want that to work regardless of image editor and is_streamness).
 But there are no hooks, so to me it looks like `make_image` is an awkward
 solution looking for a problem, and PHP/imagick provide a superior
 solution OOTB 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.

 I'll add the better tests this weekend (though I won't object to the
 existing PR being merged in the meantime!).

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