Make WordPress Core

Opened 7 years ago

Last modified 4 weeks ago

#48489 new defect (bug)

Big image size threshold should take into account registered image sizes.

Reported by: johnbillion Owned by:
Priority: normal Milestone: Future Release
Component: Media Version: 5.3
Severity: normal Keywords: good-first-bug has-patch has-unit-tests
Cc: Focuses:

Description

The "big image" upper size threshold is set to 2560. If an image size is registered that has a width or height larger than this, then the image will be unexpectedly cropped to 2560.

The value that gets passed through to the big_image_size_threshold filter should be set to the maximum value of either 2560 or the largest width or height from all registered image sizes.

Attachments (3)

48489.diff (1.9 KB ) - added by psrpinto 5 years ago.
48489.2.diff (3.9 KB ) - added by f26d 3 years ago.
48489.3.diff (4.1 KB ) - added by sachinrajcp123 3 months ago.
Added a patch for this ticket.

Download all attachments as: .zip

Change History (17)

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


7 years ago

#2 @pbiron
7 years ago

  • Milestone Future Release5.4

#3 @johnbillion
7 years ago

  • Focuses performance removed
  • Keywords needs-patch added; dev-feedback removed

#4 @johnbillion
7 years ago

  • Milestone 5.4Future Release

#5 @johnbillion
6 years ago

  • Keywords good-first-bug added

@psrpinto
5 years ago

#6 @psrpinto
5 years ago

I submitted a patch which implements the solution suggested in the ticket description:

The value that gets passed through to the big_image_size_threshold filter should be set to the maximum value of either 2560 or the largest width or height from all registered image sizes.

However, I noticed that many of the default themes (e.g. twentytwentyone, twentytwenty, twentynineteen) set the post_thumbnail_size's height to 9999, which results in the threshold now being 9999.

This means the majority of pictures will not be cropped, and I'm not sure that was the intention behind the proposed solution.

Do you think such a high threshold makes sense @johnbillion?

#7 @karpstrucking
5 years ago

  • Keywords has-patch added; needs-patch removed

#8 @adamsilverstein
3 years ago

  • Keywords needs-unit-tests added

However, I noticed that many of the default themes (e.g. twentytwentyone, twentytwenty, twentynineteen) set the post_thumbnail_size's height to 9999, which results in the threshold now being 9999.

@psrpinto Do we know why these themes use that value? I assume the goal is to crop only the horizontal dimension? is there another way to achieve that?

or maybe we can separate out the width/height treatment, or treat 9999 as a special case?

#9 @a4jp.com
3 years ago

I never knew that all images had a maximum size of 2560 when getting cropped T-T. This should be explained in /wp-admin/options-media.php if it's a set limit we can't change.

Would it make sense to allow a blank field to work if we don't want the height cropped instead of having to type 9999? I'd rather leave the field empty with nothing in it if I didn't want anything cropped instead of typing in a weird number like 9999. 9999 should crop at 9999 if images that big exist though (codewise).

This ticket was mentioned in Slack in #core-test by danieldayton33. View the logs.


3 years ago

@f26d
3 years ago

#11 @f26d
3 years ago

The 9999 height hack for the post-thumbnail size does complicate things… I think it makes sense to loop over just the widths of the image sizes to potentially bump up $threshold.

There doesn't seem to be a perfect solution to this, but in general (*waves hand*), it seems more common to resize images based on a width constraint than based on a height constraint. This makes me think that plenty of custom image sizes are likely to use identical width and height (or something like 9999 for the height, so that the height argument is essentially disregarded).

I've updated @psrpinto's diff with this suggested approach, as well as with some unit tests.

This is my first contribution to core, so please let me know if I'm missing something!

#12 @eclev91
2 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

You could get your threshold with a one-liner - max( array_merge( [ 2560 ], wp_list_pluck( wp_get_registered_image_subsizes(), 'width' ) ) )

Does it make sense to split this out into two different threshold values? One for height and one for width? This may require some additional considerations in the resize call that happens if you went that route.

As far as the core theme behavior, correct me if I'm wrong, but I only see that in twentytwenty, and I believe its behavior would be unchanged if you were to omit the height argument entirely. I could be wrong. A different ticket might suggest changing that in twentytwenty.

This ticket was mentioned in PR #12178 on WordPress/wordpress-develop by @arkaprabhachowdhury.


3 months ago
#13


This updates the default big_image_size_threshold calculation in wp_create_image_subsizes() so it accounts for registered image subsize widths before scaling large uploads. Instead of always starting from 2560, core now uses the larger of 2560 and the widest registered image size, while still ignoring height-only 9999 style definitions that are commonly used to mean “unbounded height.”
It also adds focused PHPUnit coverage to verify both behaviors: wide registered sizes prevent unnecessary -scaled generation, and height-only large values do not inflate the threshold.

Trac ticket: https://core.trac.wordpress.org/ticket/48489

## Use of AI Tools

@sachinrajcp123
3 months ago

Added a patch for this ticket.

#14 @arkaprabhachowdhury
4 weeks ago

Opened [PR #12178](https://github.com/WordPress/wordpress-develop/pull/12178) to address this ticket. … This updates the default big_image_size_threshold calculation in wp_create_image_subsizes() so it accounts for registered image subsize widths before scaling large uploads. Instead of always starting from 2560, core now uses the larger of 2560 and the widest registered image size, while still ignoring height-only 9999 style definitions that are commonly used to mean “unbounded height.” It also adds focused PHPUnit coverage to verify both behaviors: wide registered sizes prevent unnecessary -sc The PR includes focused regression coverage, and its validation details are recorded in the PR description.

I picked this up because the ticket describes a concrete core behavior or edge case that remains relevant in current WordPress. The proposed change is intentionally scoped to the ticket and is submitted here for code review, with the technical discussion remaining on this ticket.

Note: See TracTickets for help on using tickets.