WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#25721 closed enhancement (fixed)

Consolidate a couple of filters to the WP_Image_Editor base class

Reported by: markoheijnen Owned by: nacin
Milestone: 3.8 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: has-patch dev-feedback
Focuses: Cc:

Description

This is part hook docs and part a small rewrite of the code to move the filters to WP_Image_Editor.

Moving the following filters to WP_Image_Editor:

  • image_memory_limit
  • jpeg_quality
  • Not sure about image_make_intermediate_size

Will also straighten when quality is set. It's different between GD and Imagick

Attachments (5)

25721.diff (5.6 KB) - added by markoheijnen 8 years ago.
25721.2.diff (5.7 KB) - added by markoheijnen 8 years ago.
25721.3.2.diff (6.0 KB) - added by markoheijnen 8 years ago.
25721.3.diff (5.8 KB) - added by markoheijnen 8 years ago.
25721.4.diff (6.9 KB) - added by mikeschroder 8 years ago.
Enforce proper ranges all around. Standardize method signatures. Return WP_Error where appropriate.

Download all attachments as: .zip

Change History (31)

#1 @kpdesign
8 years ago

  • Summary changed from Hooks Docs: wp-includes/class-wp-image-editor-gd and wp-includes/class-wp-image-editor-imagick.php to Hooks Docs: wp-includes/class-wp-image-editor-gd.php and wp-includes/class-wp-image-editor-imagick.php

@markoheijnen
8 years ago

#2 @markoheijnen
8 years ago

First try to change the structure. I didn't move the filter 'image_make_intermediate_size'. Also not sure why someone wants to use this filter.

#3 @markoheijnen
8 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

#4 @DrewAPicture
8 years ago

  • Owner set to DrewAPicture
  • Status changed from new to reviewing

Thanks for the patch Marko.

#5 follow-up: @markoheijnen
8 years ago

I forgot to mention. This patch can only be reviewed on inline-docs. Commit should only be done if the code changes are good to commit

#6 in reply to: ↑ 5 @DrewAPicture
8 years ago

Replying to markoheijnen:

I forgot to mention. This patch can only be reviewed on inline-docs. Commit should only be done if the code changes are good to commit

Functional changes to need to happen in a separate ticket. And realistically, it's easier to keep track of progress if we only do inline docs for a single file at once.

#7 follow-up: @markoheijnen
8 years ago

Maybe, I guess same ticket but separate patches would be better. You don't code something without docs and then create a separate ticket for the docs.

I do disagree that this should be a single file at once. In general you are right. Both editors are coded the same. Unless you want to commit a file that just says that all filters are duplicated of the other file without them having the docs yet.

#8 in reply to: ↑ 7 @DrewAPicture
8 years ago

  • Component changed from Inline Docs to Media
  • Owner DrewAPicture deleted
  • Summary changed from Hooks Docs: wp-includes/class-wp-image-editor-gd.php and wp-includes/class-wp-image-editor-imagick.php to Consolidate a couple of filters to the WP_Image_Editor base class
  • Type changed from defect (bug) to enhancement

Replying to markoheijnen:

Maybe, I guess same ticket but separate patches would be better. You don't code something without docs and then create a separate ticket for the docs.

Put another way, if the primary goal of the ticket is to do anything other than just document hooks, it's not a Hook Docs ticket. :)

Here's some feedback on the docs in 25721.diff:

  • image_make_intermediate_size - We need to pick which of these as the primary and mark the second one as a duplicate
  • Short descriptions and parameter descriptions should each end with a period.

#9 @markoheijnen
8 years ago

You are right about that. I thought about that later. Thanks for the feedback and yes I should have make one of the filter as duplicate.

@markoheijnen
8 years ago

#10 @mikeschroder
8 years ago

markoheijnen: I'm unsure about creating a whole new method for setting the max memory, since really Imagick doesn't require that call at all. Would prefer removing it, since the Imagick memory usage is outside of the PHP memory_limit, rather than encourage other editors to increase the amount of RAM requested as well.

Last edited 8 years ago by mikeschroder (previous) (diff)

#11 @markoheijnen
8 years ago

I would love to see this committed this week.

I agree with Shredder that we really don't need the memory limit in Imagick so we probably don't need it abstracted. That said it's not that bad to set it only on image resize then on every page load. That said if someone really wants to set it it will not be that hard to do so.

#12 @markoheijnen
8 years ago

#26177 was marked as a duplicate.

@markoheijnen
8 years ago

#13 @markoheijnen
8 years ago

Talked with nacin about 25721.2.diff​ and decided that the memory limit for now can stay in the editor classes. I also changed set_quality method that you always need to pass the quality and that on load the quality will be set by $this->quality. This also uses the new filter 'wp_editor_set_quality' for every image manipulation.

#14 @markoheijnen
8 years ago

Totally forgot to mention that some of the documentation was copy/paste from #26177

#15 @markoheijnen
8 years ago

#26177 was marked as a duplicate.

#16 @mikeschroder
8 years ago

I chatted with @markoheijnen, and it seems like we should fix the signature issue here before this goes in.

The filter jpeg_quality accepts 0-100, and set set_quality accepts 1-100.

Clearly, this means that there is an existing bug here, but we shouldn't fix it by creating a different bug instead.

I should have time to work on this today, and will post an additional patch.

#17 @DrewAPicture
8 years ago

#26177 was marked as a duplicate.

#18 @mikeschroder
8 years ago

Just wanted to update with progress -- I'm still working on the above.

It looks like, indeed, the values accepted by both existing editors are integers, which means that set_quality accepts one fewer value than jpeg_quality, and we can't just do maths to create the appropriate equivalent percentage.

Of note, also, GD creates size-identical images between 0 and 1.

There are a few options here.

  • Consolidate filter still, and pass only 1-100 back to editors
  • Consolidate filter still, but change set_quality's signature to 0-100, and crush 0 within Imagick (this seems less desirable only because the meaning of 0 is questionable, when it refers to a quality percentage).
  • Do not consolidate filter, crush within set_quality, and handle jpeg_quality appropriately within each editor

Clearly, if they were defined as floats, this would be easier to translate; it's just a bit weirder when we're talking about ints.

For now, I'm going with the first option, since GD outputs (from all of my tests so far) the same image with 0 and 1, minus a header change noting the change in quality.

@mikeschroder
8 years ago

Enforce proper ranges all around. Standardize method signatures. Return WP_Error where appropriate.

#19 @mikeschroder
8 years ago

Attached 25721.4.diff.

This includes previous patches, and also does the following:

  • Enforce proper quality ranges
  • Standardize set_quality() method signatures between base and Imagick.
  • Return WP_Error where appropriate.
  • Update Inline docs for correctness

One point of contention here is the behaviour when running set_quality() by itself.

Previously, in Imagick (but not base), if you called it without an argument, it would set $this->quality as the quality, and return as to whether that succeeded. Because it was in there previously, for backcompat, I've standardized on that behavior.

However, since Imagick is no longer relying on the behavior, the other option would be to make the argument required.

#20 follow-up: @markoheijnen
8 years ago

I would recommend to just make the argument for set_quality() required.

I also would remove the weird check in set_quality for JPEG. To set the quality from 0 to 1 to protect backwards compatibility doesn't make a lot sense. No one will do so since images will get really crappy.

#21 in reply to: ↑ 20 @mikeschroder
8 years ago

Replying to markoheijnen:

I would recommend to just make the argument for set_quality() required.

I also would remove the weird check in set_quality for JPEG. To set the quality from 0 to 1 to protect backwards compatibility doesn't make a lot sense. No one will do so since images will get really crappy.

Both of these were backcompat decisions -- in particular the second, though.

Because we're now enforcing proper values, if someone passes to jpeg_quality a 0 quality, it will WP_Error [edit: if we get rid of that check]. I agree that it's not particularly pretty, but don't see why we should break image manipulation entirely for anyone who was doing that previously.

Last edited 8 years ago by mikeschroder (previous) (diff)

#22 @mikeschroder
8 years ago

Just an addition on here -- I'd say that we could skip this for 3.8, but it does make the wp_editor_set_quality filter actually work like it should for Imagick, and that's a bug worth fixing.

#23 @markoheijnen
8 years ago

This shouldn't be skipped. The fix for setting the quality is to important.

#24 @nacin
8 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from reviewing to closed

In 26645:

Improvements to image quality handling in the image editor classes.

props markoheijnen, DH-Shredder.
fixes #25721.

#25 @DrewAPicture
8 years ago

In 26648:

Hook docs fixes following [26645].

See #25721.

#26 @DrewAPicture
8 years ago

In 26650:

Two more hook docs fixes.

image_memory_limit filter:

  • Go with int|string on the limit, and simply notate '256M' as an acceptable string value.

wp_editor_set_quality filter:

  • Add a missing parameter description for the mime type.

See #25721.

Note: See TracTickets for help on using tickets.