Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#50630 closed defect (bug) (duplicate)

imagick image editor does not support streams

Reported by: p00ya's profile p00ya Owned by:
Milestone: Priority: normal
Severity: normal Version: 5.5
Component: Media Keywords: has-patch has-unit-tests
Focuses: Cc:


The imagick image editor (in particular the load and save methods) do not work with streams.

The use case for having the image editor work with streams is when using plugins like that register a custom stream handler (in that case for "gs://" URLs) and add a filter to upload_dir so that uploads result in RPCs to external services.

There are a few reasons the original code didn't work:

  • realpath doesn't work with streams
  • Imagick::readImage and Imagick::writeImage expect filenames, not URLs

I have a working patch which I'll upload to this ticket.

Attachments (1)

50630.diff (4.0 KB) - added by p00ya 4 years ago.
Patch for #50630

Download all attachments as: .zip

Change History (14)

4 years ago

Patch for #50630

#1 @p00ya
4 years ago

Some notes on the patch:

The patch uses Imagick::getImageBlob and Imagick::readImageBlob. I did also investigate using readImageFile and writeImageFile, but these appear to misbehave with some stream handlers (probably a bug in the PHP extension?).

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

4 years ago

#3 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.6

Hi there, welcome to WordPress Trac! Thanks for the ticket.

Just noting that the patch looks good to me at a glance. Moving to the milestone so it doesn't get lost.

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

4 years ago

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

4 years ago

#6 @hellofromTonya
4 years ago

  • Keywords has-patch added

Tagging to denote the ticket has a patch that's ready for review.

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

4 years ago

  • Keywords has-unit-tests added

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

Trac ticket:

#8 @p00ya
4 years ago

Hi, just a note to any reviewers that they should look at the github PR rather than reviewing the patch I attached here on trac (which now has merge conflicts).

#9 @kirasong
4 years ago

Thanks so much for the patch and tests!
Agreed @SergeyBiryukov, let's see if we can fix this for 5.6.

Previously: #42663

Thinking maybe we should move this over there, so the discussion continues in the same place?

#10 @p00ya
4 years ago

I'm subbed on #42663 now if you want to move discussion there.

Let me know whether I should:

  1. point the PR to #42663
  2. add support for very old versions of the imagick extension as #42663's patch did
  3. try and call make_image from _save like #42663's patch did (I'm against this)
  4. add a dummy stream wrapper for testing like #42663's patch did (instead of testing with file:// URLs)

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

4 years ago

#12 @kirasong
4 years ago

  • Milestone 5.6 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Sounds good -- Yes, please point the PR over there, so we can discuss your notes/questions there.

Since the (other) ticket was started a while ago, I'm definitely open to a change in approach from what was commented there. Just want to be sure we take those concerns into account when choosing what goes in!

I'm going to mark this one as a duplicate, then assign #42663 to 5.6, like this one was, so it stays on track.

Note: See TracTickets for help on using tickets.