WordPress.org

Make WordPress Core

Opened 23 months ago

Last modified 7 weeks 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:
PR Number:

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 23 months ago.
imagick.2.diff (2.1 KB) - added by calin 23 months ago.
imagick.3.diff (2.5 KB) - added by calin 23 months ago.
imagick.4.diff (12.9 KB) - added by calin 9 months ago.
imagick.5.diff (12.9 KB) - added by calin 9 months ago.
Fix testing names

Download all attachments as: .zip

Change History (20)

@calin
23 months ago

#1 @joemcgill
23 months 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
23 months 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
23 months ago

#3 @calin
23 months ago

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

@calin
23 months ago

#4 @calin
23 months ago

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

#5 follow-up: @calin
20 months 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
15 months 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
13 months 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
9 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
9 months ago

@calin
9 months ago

Fix testing names

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

  • Version trunk deleted

#11 @pputzer
3 months ago

  • Keywords reporter-feedback removed

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


3 months ago

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


3 months ago

#14 follow-up: @mikeschroder
3 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
7 weeks 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.

Note: See TracTickets for help on using tickets.