Consolidate a couple of filters to the WP_Image_Editor base class

[markoheijnen]

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

[markoheijnen]

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.

[DrewAPicture]

Thanks for the patch Marko.

[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

[DrewAPicture]

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.

[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.

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.

[DrewAPicture]

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.

[markoheijnen]

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.

[kirasong]

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.

[markoheijnen]

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.

[markoheijnen]

#26177 was marked as a duplicate.

[markoheijnen]

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.

[markoheijnen]

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

[markoheijnen]

#26177 was marked as a duplicate.

[kirasong]

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.

[DrewAPicture]

#26177 was marked as a duplicate.

[kirasong]

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.

[kirasong]

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

[kirasong]

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.

[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.

[kirasong]

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.

[kirasong]

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.

[markoheijnen]

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

[nacin]

In 26645:

Improvements to image quality handling in the image editor classes.

props markoheijnen, DH-Shredder.
fixes #25721.

[DrewAPicture]

In 26648:

Hook docs fixes following [26645].

See #25721.

[DrewAPicture]

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.