WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 3 months ago

Last modified 5 weeks ago

#50630 closed defect (bug) (duplicate)

imagick image editor does not support streams

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

Description

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 https://wordpress.org/plugins/gcs/ 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 5 months ago.
Patch for #50630

Download all attachments as: .zip

Change History (14)

@p00ya
5 months ago

Patch for #50630

#1 @p00ya
5 months 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.


3 months ago

#3 @SergeyBiryukov
3 months 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.


3 months ago

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


3 months ago

#6 @hellofromTonya
3 months 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.


3 months 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: https://core.trac.wordpress.org/ticket/50630

#8 @p00ya
3 months 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 @mikeschroder
3 months 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
3 months 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.


3 months ago

#12 @mikeschroder
3 months 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.