[wp-trac] [WordPress Trac] #42663: Imagick support for stream wrappers
WordPress Trac
noreply at wordpress.org
Tue Oct 20 11:03:56 UTC 2020
#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
More information about the wp-trac
mailing list