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

[WordPress Trac]

#42663: Imagick support for stream wrappers
----------------------------------------+---------------------------
 Reporter:  calin                       |       Owner:  mikeschroder
     Type:  defect (bug)                |      Status:  assigned
 Priority:  normal                      |   Milestone:  5.6
Component:  Media                       |     Version:
 Severity:  normal                      |  Resolution:
 Keywords:  has-unit-tests needs-patch  |     Focuses:
----------------------------------------+---------------------------
Changes (by mikeschroder):

 * keywords:  has-patch 2nd-opinion has-unit-tests => has-unit-tests needs-
     patch
 * type:  enhancement => defect (bug)


Comment:

 @p00ya:

 Looked over this and chatted with @joemcgill about it as well.
 TL;DR: I think your rationale is good. Let's do this.

 He included a couple of suggestions I'll pass on here:
 - We might not need a `_` on the private method.\\
   I agree, as it's private in this case.

 - We could consider deprecating `make_image` (as falls in line with your
 recommendations), either now, or later.\\
   Let's start without that, and consider whether we should deprecate
 later, as we'd want to be very specific about where and for what it is
 being deprecated, and I think that's worth a discussion.

 - There's a warning in `file_put_contents` about the return value we
 should heed about the return value: https://www.php.net/manual/en/function
 .file-put-contents.php


 On the last note, going through the patch, I also noticed that
 `writeImage()` can throw an exception via Imagick on error. This does get
 caught properly in `_save()`, but should be either documented, or perhaps
 caught within the `write_image()` method directly, which would allow the
 signature to be a bit tighter.

 Putting together a pass so we can get an initial commit before beta. We
 can iterate on this a bit during beta if we find issues, and if you have
 further feedback about what ends up going in.

 Finally, I'm changing this from an enhancement to a bug, because it was
 intended to work. In my opinion, that doesn't change that it should land
 now, due to the complexity of the change, but at least it reflects the
 history of the issue a bit better.

 Thanks again for all your help!

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