Make WordPress Core

Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#42663 closed defect (bug) (fixed)

Imagick support for stream wrappers

Reported by: calin's profile calin Owned by: kirasong's profile kirasong
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-unit-tests needs-patch has-dev-note
Focuses: Cc:

Description

The current implementation of imagick backend doesn't support stream wrappers. In order to support them readImageFile and writeImageFile methods must be used instead of readImage and writeImage. The methods were added in ImageMagick module 2.3.0.

This patch uses stream wrapper compatible functions if they are available.

Attachments (5)

imagick.diff (2.1 KB) - added by calin 6 years ago.
imagick.2.diff (2.1 KB) - added by calin 6 years ago.
imagick.3.diff (2.5 KB) - added by calin 6 years ago.
imagick.4.diff (12.9 KB) - added by calin 5 years ago.
imagick.5.diff (12.9 KB) - added by calin 5 years ago.
Fix testing names

Download all attachments as: .zip

Change History (47)

@calin
6 years ago

#1 @joemcgill
6 years ago

  • Keywords has-patch 2nd-opinion added

Hi @calin,

Thanks for the suggestion and for supplying a patch. This seems like a good idea to me.

cc: @mikeschroder for a second opinion.

Related: #28077.

#2 @kirasong
6 years ago

Thanks for the patch, @calin!

I'll need to dig into this more to understand what cases we're accounting for right now and not.

As a bit of quick info to add to the ticket, I remember that initially make_image() and the bits within were created to work around this particular use case, but handling streams better is something I'd definitely support.

@calin
6 years ago

#3 @calin
6 years ago

I've updated the patch to use static properties since WP_Image_Editor::test is a static method.

@calin
6 years ago

#4 @calin
6 years ago

I've updated the patch so that wp core tests pass.

#5 follow-up: @calin
6 years ago

Hi, @mikeschroder, @joemcgill, is there anything I can improve on this patch? Do you need more info so that it can be merged?

#6 @pputzer
6 years ago

Can I help? Being able to reliably use stream wrappers with WP_Image_Editor without having to check for the GD implementation would be a huge benefit.

#7 in reply to: ↑ 5 @kirasong
6 years ago

  • Keywords reporter-feedback added

Hey @calin!
Sorry for the wait on further comments, and thanks again for the patches.

Yes, I've got a couple of questions:

  • Would it be possible to handle the writing part of this within make_image(), since this is much of the use-case for it? It's fine to extend make_image() for Imagick like GD does if that is necessary. If that's not preferred, any reasoning would be great.
  • Could you please walk me through the reasoning for $imagick_filename and its use?

Thanks much!

#8 @calin
5 years ago

Hi @mikeschroder,

I'm going to attempt another patch using the following approach: if the path is a stream wrapper, copy the file locally, apply changes and copy back. I think imagick does the same behinde the scenes, but the implementation seams flakey (see: https://github.com/mkoppanen/imagick/issues/225). This should also enable support for stream wrappers for older versions of imagick as well.

Will upload the patch+tests in the following days.

Btw. is there a deadline for including this in 5.1?

@calin
5 years ago

@calin
5 years ago

Fix testing names

#9 @calin
5 years ago

@mikeschroder, I've updated the patch and also added tests.

For the testing I've also added a dummy, in-memory stream wrapper with just enough implementation for image editor testing. The added tests are also enabled for GD, which supports stream wrappers out of the box.

#10 @pento
5 years ago

  • Version trunk deleted

#11 @pputzer
5 years ago

  • Keywords reporter-feedback removed

This ticket was mentioned in Slack in #core-media by pepe. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-media by pepe. View the logs.


5 years ago

#14 follow-up: @kirasong
5 years ago

Hi @calin!

I'm sorry it's taken so long to review this. Thank you *so* much for the patch!

It was brought up in today's media component meeting, and I have two thoughts on the approach:

  • I'm wondering if there's a way to do this without saving a file, since if folks are intending to use streams, I wonder if they might have filesystem concerns.
  • I think it's better to avoid changing the signature of pdf_setup() if it's possible.

#15 in reply to: ↑ 14 @pputzer
5 years ago

Replying to mikeschroder:

  • I'm wondering if there's a way to do this without saving a file, since if folks are intending to use streams, I wonder if they might have filesystem concerns.
  • I think it's better to avoid changing the signature of pdf_setup() if it's possible.

Since @calin is not around at the moment, I'll try to come up with a patch that conforms to these requirements (reusing the tests etc.). I've looked at the code and the history of the writeImageFile method and I think it is now safe to add these as a hard requirement. ImageMagick introduced them 2007 and the imagick extension added support 2009.

#16 @jimyaghi
4 years ago

Hey guys i notice the base class WP_Image_Editor assumes a stream is always output to stdout and thus it always captures the image using ob_start() and friends, then opens a file to write to on the stream.

However, only the WP_Image_Editor_GD class has this behaviour because the callbacks imagegif()/imagepng()/imgejpeg() default to stdout when no filename is specified. The WP_Image_Editor_Imagick class does NOT do this and thus does not work to write to streams.

This can be easily fixed.

WP_Image_Editor_GD uses the following override method to remove the filename argument for streams.

<?php
        /**
         * Either calls editor's save function or handles file as a stream.
         *
         * @since 3.5.0
         *
         * @param string|stream $filename
         * @param callable $function
         * @param array $arguments
         * @return bool
         */
        protected function make_image( $filename, $function, $arguments ) {
                if ( wp_is_stream( $filename ) ) {
                        $arguments[1] = null;
                }

                return parent::make_image( $filename, $function, $arguments );
        }

Perhaps we can emulate it by doing something like the following in WP_Image_Editor_Imagick:

<?php
       protected function make_image( $filename, $function, $arguments ) {
                if ( wp_is_stream( $filename ) ) {
                        $function = [$this, 'to_stdout'];
                        $arguments = [$this->image->getImageBlob()];
                }

                return parent::make_image( $filename, $function, $arguments );
        }
        function to_stdout($blob) {
             echo $blob;
             return true;
        }

This way the behaviour is consistent in both classes.

Alternatively, we can skip the extra method and do a condition in the _save().

Currently we have:

<?php
$this->make_image( $filename, array( $image, 'writeImage' ), array( $filename ) );

It would become:

<?php
if(is_stream($filename)) {
    $this->make_image( $filename, array( $this, 'to_stdout' ), array( $image->getImageBlob() ) );
} else {
    $this->make_image( $filename, array( $image, 'writeImage' ), array( $filename ) );
}

~j

Last edited 4 years ago by jimyaghi (previous) (diff)

#17 @kirasong
4 years ago

#50630 was marked as a duplicate.

#18 @kirasong
4 years ago

  • Milestone changed from Awaiting Review to 5.6
  • Owner set to mikeschroder
  • Status changed from new to assigned

This ticket was mentioned in PR #521 on WordPress/wordpress-develop by p00ya.


4 years ago
#19

  • Keywords has-unit-tests added

Support streams from the imagick image editor. Fixes problems with using realpath and imagick::readImage with streams.

Trac ticket: https://core.trac.wordpress.org/ticket/42663

#20 @p00ya
4 years ago

Open questions regarding my github PR:

  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.
  2. should I try and call make_image from _save like @calin's patch did. Pros: ??; Cons: convoluted code.
  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.

#21 follow-up: @kirasong
4 years ago

  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:

  • This might not be representative, but as a spot check, looks like the majority of 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?

  1. 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, 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?

  1. 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!

#22 in reply to: ↑ 21 @p00ya
3 years ago

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

Replying to 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:

  • This might not be representative, but as a spot check, looks like the majority of 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 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.

  1. 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, 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.

  1. 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!).

#23 @p00ya
3 years ago

Please take another look at https://github.com/WordPress/wordpress-develop/pull/521 (updated to use a custom stream wrapper instead of file:// URLs). I borrowed some of calin's code.

#24 @kirasong
3 years ago

@p00ya Thanks so much for all of the research, notes, and the new patch. Taking a look today.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 years ago

#27 @kirasong
3 years ago

Sorry for the slow reply.

I ran this by the media channel / maintainers.

I think if it’s possible, it’d be best to maintain compatibility at least with make_image().

Some notes from @joemcgill starting here:
https://wordpress.slack.com/archives/C02SX62S6/p1601996983144800

If you’d like to take a pass at that, I’d be grateful. If not, I’ve also been thinking about the best way to do so and can put it together this week before beta.

Thanks again for the help!

#28 @p00ya
3 years ago

I can spend some time this Friday updating the PR to shuffle more logic into make_image, assuming that's not too tight for beta cuts.

I'll note that @joemcgill's messages didn't really justify the use of make_image for me. There are some more points I could make in addition to the long post above (responding to the concerns about compatibility and separation of concerns), but I'm also happy to leave it at "respectfully disagree" and move the logic for Imagick in there anyway. The maintainers are welcome to ping me on Slack to discuss further though.

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


3 years ago

#30 @kirasong
3 years ago

Around time-frame, that sounds great, thank you!

Also, definitely do feel free to explain the points if you'd like -- I think everyone involved (myself included) is looking for the best way to do things, and changing minds/decisions is 💯 fine.

I'm personally on the fence about this, which is why I ended up reaching out for another opinion. I agree the current design could be better, and with supporting so many installs, sometimes finding that middle-ground is tricky.

#31 @p00ya
3 years ago

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:

$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:


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.

#32 @kirasong
3 years ago

Just a quick note to say I haven't forgotten about this for those following.

Had a migraine that kept me from it today, but I'll make a call on this, approach, and 5.6 tomorrow / before the beta.

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


3 years ago

#34 @kirasong
3 years ago

  • Keywords needs-patch added; has-patch 2nd-opinion removed
  • Type changed from enhancement to defect (bug)

@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.

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!

#35 @kirasong
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 49230:

Media: Support Stream Wrappers In WP_Image_Editor_Imagick

Since WP_Image_Editor's introduction, stream wrappers have functioned in WP_Image_Editor_GD, but haven't been properly supported in WP_Image_Editor_Imagick.

  • Detects stream wrappers and uses file_put_contents() along with Imagick::read/getImageBlob() for handling when necessary.
  • Introduces private method, WP_Image_Editor_Imagick::write_image to handle detection and proper saving.
  • Introduces WP_Test_Stream class for testing stream wrappers, along with new tests for Imagick's stream handling and a stream filename test.

Adds requirement for Imagick::readImageBlob(), available in Imagick >= 2.0.0, which aligns with the current requirement of Imagick >= 2.2.0.

Props p00ya, calin, joemcgill, pputzer, jimyaghi, mikeschroder.
Fixes #42663.

This ticket was mentioned in Slack in #core by mike. View the logs.


3 years ago

#37 @kirasong
3 years ago

  • Keywords needs-dev-note added

#39 @p00ya
3 years ago

FYI I've tested image uploads (and thumbnailing) with 5.6-beta1 and Google's cloud storage plugin 0.1.6. LGTM.

This ticket was mentioned in Slack in #core by daisyo. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.