WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 4 months ago

#42663 new enhancement

Imagick support for stream wrappers

Reported by: calin Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch 2nd-opinion
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 3 years ago.
imagick.2.diff (2.1 KB) - added by calin 3 years ago.
imagick.3.diff (2.5 KB) - added by calin 3 years ago.
imagick.4.diff (12.9 KB) - added by calin 19 months ago.
imagick.5.diff (12.9 KB) - added by calin 19 months ago.
Fix testing names

Download all attachments as: .zip

Change History (21)

@calin
3 years ago

#1 @joemcgill
3 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 @mikeschroder
3 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
3 years ago

#3 @calin
3 years ago

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

@calin
3 years ago

#4 @calin
3 years ago

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

#5 follow-up: @calin
2 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
2 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 @mikeschroder
2 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
19 months 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
19 months ago

@calin
19 months ago

Fix testing names

#9 @calin
19 months 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
19 months ago

  • Version trunk deleted

#11 @pputzer
13 months ago

  • Keywords reporter-feedback removed

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


13 months ago

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


12 months ago

#14 follow-up: @mikeschroder
12 months 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
12 months 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 months 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 months ago by jimyaghi (previous) (diff)
Note: See TracTickets for help on using tickets.