WordPress.org

Make WordPress Core

Opened 20 months ago

Last modified 6 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 reporter-feedback
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 20 months ago.
imagick.2.diff (2.1 KB) - added by calin 20 months ago.
imagick.3.diff (2.5 KB) - added by calin 20 months ago.
imagick.4.diff (12.9 KB) - added by calin 6 months ago.
imagick.5.diff (12.9 KB) - added by calin 6 months ago.
Fix testing names

Download all attachments as: .zip

Change History (15)

@calin
20 months ago

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

#3 @calin
20 months ago

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

@calin
20 months ago

#4 @calin
20 months ago

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

#5 follow-up: @calin
17 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
12 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
10 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
6 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
6 months ago

@calin
6 months ago

Fix testing names

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

  • Version trunk deleted
Note: See TracTickets for help on using tickets.