WordPress.org
Get WordPress

Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#38832 closed defect (bug) (fixed)

WP_Image_Editor can no longer edit PDF pages other than the first one

Reported by: kirasong's profile kirasong Owned by: kirasong's profile kirasong
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Media Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

After [39187], WordPress force-loads only the first page of a PDF.

As an unintended consequence, if developers were using/extending WP_Image_Editor_Imagick to output other pages, this will be broken in the current state, and would require them to override load() to work around it.

We should do at least one of the following:

  • Find a way to pass desired pages to the editor
  • Create a workaround for those cases
  • Educate developers on the change

Attachments (2)

38832.diff​ (1.5 KB) - added by kirasong 8 years ago.
Workaround. Would allow authors to override the PDF behavior.
38832.2.diff​ (3.6 KB) - added by joemcgill 8 years ago.

Download all attachments as: .zip

Change History (12)

This ticket was mentioned in ​Slack in #core-media by mike. ​View the logs.
8 years ago

#2 @kirasong
8 years ago

  • Owner set to mikeschroder
  • Status changed from new to assigned

#3 @kirasong
8 years ago

Chatted with @markoheijnen a bit about this. He suggested we put the small amount of PDF logic that was added into a method that can be overridden, which would allow both changing how pages are handled and DPI.

Would probably be helpful to know how common the current use case is, since the above would still potentially break plugins that are extending the editor for PDF reasons.

I’d like to avoid a filter if we can, although it is definitely the "quick" way to fix it.

@kirasong
8 years ago

Workaround. Would allow authors to override the PDF behavior.

#4 @kirasong
8 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

I'm not very happy with this approach, but 38832.diff​ allows developers to override the PDF loading behavior to re-enable multi-page loads or change the DPI.

This approach would not solve the backcompat issue, but would allow working around it.

Separately, ping'd @ipstenu to see if it's possible to search the plugin repo for extends WP_Image_Editor_Imagick -- if anyone else has a downloaded version, a search would be appreciated to make it easier to dig for plugin authors that have extended the Imagick editor for PDF reasons.

@joemcgill
8 years ago

#6 @joemcgill
8 years ago

38832.2.diff​ Takes a bit of a different approach here, reverting some of the PDF specific code in WP_Image_Editor_Imagick::load() in favor of an approach that would allow someone to set resolution and page ranges on the editor instance before running the load() method. The changes to wp_generate_attachment_metadata() for generating thumbnails for PDFs shows how this could be used.

This was a pretty quick proof of concept, so testing and feedback would be appreciated.

#7 @kirasong
8 years ago

Hmm. Took a look; thanks for putting it together! Interesting workaround -- hadn’t thought about running _choose separately.

Two (related) notes:

  • If we can, I’d prefer to avoid adding additional public methods on only one type of WP_Image_Editor, since it could lead to more third party code that will only work with the Imagick editor (but would fail if another editor is selected).
  • I’m not a huge fan of update_file being public, since the WP_Image_Editor model is supposed to surround a single input file, and this would not have the effect expected by developers if run from outside of a WP_Image_Editor.

#8 @joemcgill
8 years ago

Thanks Mike. Those echo my main concerns with this approach as well, but was looking for a way to do this that:

a) Didn't introduce a new filterable value into the logic chain, and
b) Left PDF implementation details out of the load() method to ensure backcompat with anything extending the WP_Image_Editor_Imagick class.

The only other way I could see us getting around this is to put an action hook or filter between the instantiation of the Imagick object on $this->image and the $this->image->readImage() call.

This ticket was mentioned in ​Slack in #core-media by joemcgill. ​View the logs.
8 years ago

#10 @kirasong
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 39303:

Media: Allow override of PDF setup for multiple pages or DPI.

After [39187], WordPress started loading only the first page of a PDF.

This is appropriate for performance, but made it impossible to
write plugins that read other pages without overriding load().

Introduces WP_Image_Editor_Imagick->pdf_setup(), to allow an override
to change WordPress' rendering DPI defaults or which pages are loaded.

Fixes #38832. See #38522, #31050.
Props markoheijnen, joemcgill, mikeschroder.

Note: See TracTickets for help on using tickets.