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

 So I wrote something that went through `make_image`, slept on it, then
 decided not to anyway.  See recent commits on https://github.com/WordPress
 /wordpress-develop/pull/521 .

 TLDR: I've factored out the stream vs file logic into a new, **private**
 `_write_image` method, which takes care of the separation-of-concerns in a
 much cleaner way than using `make_image`.  Clients relying on `make_image`
 will retain the existing behaviour.

 Long post explaining my reasoning follows.

 I think it's a useful exercise to try and define exactly what
 `make_image`'s contract is.  I'll expand on the definition I gave before
 for `WP_Image_Editor`:

  If $filename is a file, then just call $function($arguments...), which
 should write an image to $filename. 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.

 Already we can see this is a bit awkward when considering separation of
 concerns: ''sometimes'' `make_image` offers to write to the destination,
 but other times $function must take care of writing to the destination.

 Now lets examine the contract for `WP_Image_Editor_GD`:

  If $filename is a file, then just call $function($arguments...), which
 should write an image to $filename.  If $filename is a stream URL, call
 $function($arguments...), but actually replace `$arguments[1]` with NULL.
 $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 obviously getting awkward.  Is `WP_Image_Editor_GD::make_image`
 even polymorphic when it subtly changes the contract from the parent by
 poking at the callback arguments?  This is also terrible from a
 readability perspective: the reader has to check the implementations of
 `WP_Image_Editor_GD::make_image`, `WP_Image_Editor::make_image` and
 `imagegif` to figure out WTF is going on in a call like:

 {{{#!php
 $this->make_image( $filename, 'imagegif', array( $image, $filename ) )
 }}}

 Now lets talk about compatibility.  `make_image` is protected, so the only
 clients we need to be concerned with are those in wordpress-core or third
 party plugins that are **subclassing** `WP_Image_Editor_Imagick`.  There's
 no subclasses in wordpress-core (and even if there were, I'd fix them).

 So if I were such a plugin developer, I'd probably be trying something
 like:

 {{{#!php

 class Bestest_Image_Editor extends WP_Image_Editor_Imagick {

 // ...

 if ( wp_is_stream ( $filename ) ) {
   // Thanks, make_image, for the handy output buffering.
   $this->make_image( $filename, array( $this, 'stream' ), array( ) );
 } else {
   // Why even bother with make_image here?
   $this->image->writeImage( $filename );
 }
 }}}

 This probably works today.  As a side-note, it demonstrates that nobody
 can trust `make_image` to truly abstract the file/URL distinction; the
 caller already needs to be concerned with passing different callbacks in,
 so `make_image` just becomes a utility for output buffering.  At this
 point, the arguments in my previous post apply.

 There's another possibility to consider for compatibility: clients are
 viewing `make_image` as some kind of template method design pattern, and
 overriding it.  If we don't call it, their important image-writing hook
 will not fire.  I think this scenario is very unlikely, and I'm also not
 sympathetic to these clients (since they're relying on undocumented
 implementation details of the `_save` method).

 So if we want to use `make_image`, we either need to override it like GD
 (which breaks polymorphic contracts and therefore code) or submit to
 output buffering (breaks separation-of-concerns anyway, and my previous
 concerns about readability and memory usage apply).

 So I'd still prefer to view `make_image` simply as an output buffering
 utility for image editor subclasses, and never even use it from
 `WP_Image_Editor_Imagick`.  It has an unfortunate API contract, but folks
 might already be relying on it and GD found a way to use it so whatevs.

 If it's just a protected helper function, lets not be obsessed with
 awkwardly calling it when there are much more direct calls.

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