WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#29856 closed defect (bug) (fixed)

Setting default quality doesn't always work

Reported by: markoheijnen Owned by: SergeyBiryukov
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.0
Component: Media Keywords: has-patch
Focuses: Cc:
PR Number:

Description

Due to a misplaced if statement in [28879] the quality doesn't get set for non JPG images.
Setting quality for Imagick also failed. Added unit tests to check for default behaviour.

Attachments (4)

29856.diff (5.0 KB) - added by markoheijnen 5 years ago.
29856.2.diff (5.0 KB) - added by markoheijnen 5 years ago.
29856.3.diff (1.6 KB) - added by markoheijnen 5 years ago.
Move running filters to set_quality method.
29856.docs_test.diff (3.9 KB) - added by mikeschroder 5 years ago.
Tests and Docs to clarify filter precedence.

Download all attachments as: .zip

Change History (33)

@markoheijnen
5 years ago

This ticket was mentioned in IRC in #wordpress-dev by markoheijnen. View the logs.


5 years ago

#2 @johnbillion
5 years ago

Line 259 of class-wp-image-editor.php in 29856.diff is this:

if ( $quality == null ) {

Should that have a strict equality check?

@markoheijnen
5 years ago

#3 @markoheijnen
5 years ago

Yes, it does.

#4 @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 29834:

Fix setting default quality in WP_Image_Editor.

props markoheijnen.
fixes #29856 for trunk.

#5 @SergeyBiryukov
5 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#6 @ocean90
5 years ago

#29903 was marked as a duplicate.

#7 @tomauger
5 years ago

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

This patch broke image multiresizing on a live multisite (that was accidentally using Trunk rather than stable ooops).

Essentially, using WP_Image_Editor_GD, setting quality on load() set the quality of all image sizes to 1.

It should also be noted that the direct call to set_quality() bypasses all the quality filters. I'm not sure whether this is an issue, but it threw me for a loop as I was trying to log the quality setting and the filters were just not being called at all.

Interestingly, I am not seeing the issue on a Vanilla install with Twentythirteen and no plugins. However, I did disable all plugins and used Twentyten on the multisite and the issue occurred consistently. A mirror dev site using the same database and configuration, but the stable 4.0 instead of trunk was not experiencing the issue.

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


5 years ago

#9 @nacin
5 years ago

  • Keywords has-patch dev-feedback added; fixed-major needs-patch reporter-feedback removed

#10 @markoheijnen
5 years ago

I'm unsure why this would fail for you. set_quality() would fallback to the property default_quality because the value passed is null. So the quality would be 90 then. I do see the issue of filters not getting passed.

@markoheijnen
5 years ago

Move running filters to set_quality method.

#11 @markoheijnen
5 years ago

  • Keywords reporter-feedback added; dev-feedback removed

I added an additional patch that moves all the logic from get_quality() to set_quality(). This way the filters will run always.
So as a developer I expect that calling set_quality() will always go to the default value which I can filter with 'wp_editor_set_quality'.

#12 @tomauger
5 years ago

That's fantastic Marko - I'll test the patch and report back if there's anything weird, but I can't imagine there would be.

#13 @samhotchkiss
5 years ago

works for me

#14 @nacin
5 years ago

  • Milestone changed from 4.0.1 to 4.1

#15 @tomauger
5 years ago

Everything seems in order here.

#16 @DrewAPicture
5 years ago

@markoheijnen, @SergeyBiryukov: What's left here for 4.1? Is 29856.3.diff the agreed-upon solution?

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


5 years ago

#18 @SergeyBiryukov
5 years ago

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

In 30788:

Move the logic from WP_Image_Editor::get_quality() to WP_Image_Editor::set_quality(), so that 'wp_editor_set_quality' and 'jpeg_quality' filters run when setting the default value.

props markoheijnen.
fixes #29856.

@mikeschroder
5 years ago

Tests and Docs to clarify filter precedence.

#19 @mikeschroder
5 years ago

Was going through the unit tests for this, and found several edge cases that needed a bit of further testing.

While doing so, noticed that it's not clear in docs that if you instantiate an editor, then either quality filter is applied, it does not affect the quality of any further images saved by that editor (unless set_quality is directly called with a null parameter).

Would be great to get the docs changes in 4.1 if it's still an option. Unit tests, of course, also help, but can wait if it's too much for the milestone.

The attached patch contains both changes, but I can split them out if it's helpful.

#20 @SergeyBiryukov
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#21 @markoheijnen
5 years ago

The attached patch look good and is ready to go in. We could even explain more by mentioning that when set_quality is called with a null that it reverts back to the default value. Which is filterable.

#22 @SergeyBiryukov
5 years ago

  • Keywords commit added; reporter-feedback removed

#23 @johnbillion
5 years ago

In 30873:

Add tests which ensure the wp_editor_set_quality and jpeg_quality filters only apply if they are added before the corresponding WP_Image_Editor is instantiated.

Props DH-Shredder
See #29856

#24 @johnbillion
5 years ago

The inline docs for the wp_editor_set_quality and jpeg_quality filters still aren't great, but I can't come up with anything better.

#25 @johnbillion
5 years ago

In 30874:

Clarify the behaviour of the wp_editor_set_quality and jpeg_quality filters.

Props DH-Shredder
See #29856

#26 @johnbillion
5 years ago

  • Keywords fixed-major added

#27 @johnbillion
5 years ago

In 30875:

Add tests which ensure the wp_editor_set_quality and jpeg_quality filters only apply if they are added before the corresponding WP_Image_Editor is instantiated.

Merges [30873] to the 4.1 branch.

Props DH-Shredder
See #29856

#28 @johnbillion
5 years ago

In 30876:

Clarify the behaviour of the wp_editor_set_quality and jpeg_quality filters.

Merges [30874] to the 4.1 branch.

Props DH-Shredder
See #29856

#29 @johnbillion
5 years ago

  • Keywords commit fixed-major removed
  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.