Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#48304 closed defect (bug) (fixed)

Big Image: Revisit 'threshold' suffix on file names

Reported by: kraftbj's profile kraftbj Owned by: azaozz's profile azaozz
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.3
Component: Media Keywords: has-patch
Focuses: Cc:

Description

After #46076, when uploading a big image (over 2560px by default), there is special processing to create a new "full" size image that acts as the largest size that should usually be used by WordPress.

The naming convention employed by 46076 for this reduced image is the value of the threshold alone, e.g. image-2560.png for a file of image.png uploaded.

WordPress, previously, used both the height and width for all created sizes, e.g. image-2560x2000.png.

Whenever a plugin/service is looking for the original image, it can commonly look to strip the suffix. For this new size, the suffix style has changed resulting in cases where it may not be able to realize it is not the original image.

Question: Are we open to not having a special naming convention for the threshold-limited file and let WordPress give it the standard image-2560x2000.jpg file name?

Attaching a patch if we want to use the more traditional naming scheme and marking as 5.3 to ensure review before the new functionality ships as stable.

cc: @azaozz

Attachments (2)

48304.diff (902 bytes) - added by kraftbj 5 years ago.
48304.2.diff (886 bytes) - added by azaozz 5 years ago.

Download all attachments as: .zip

Change History (18)

@kraftbj
5 years ago

#1 @kirasong
5 years ago

I agree it'd be better to have the naming be more consistent, if it's possible.

@azaozz is there a technical reason I'm forgetting/missing for choosing to name the size this way?

#2 follow-up: @kraftbj
5 years ago

I'll add if it is intentional to use a different format than an usual resize (maybe we don't want to make it that easy to use the original?), instead of -####.ext, maybe something like -wpthres-####.ext etc to make it more obvious where the file name came from while providing a pseudo-namespace.

Last edited 5 years ago by kraftbj (previous) (diff)

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


5 years ago

#4 in reply to: ↑ 2 @azaozz
5 years ago

Replying to kraftbj:

I'll add if it is intentional to use a different format than an usual resize (maybe we don't want to make it that easy to use the original?)...

This is exactly the intention. Similarly to #48302, the scaled image is intended to replace the actual "attachment image" in all use cases, same as if the user has scaled the image before uploading it. The original image is kept in order to make better quality sub-sizes in the future.

...instead of -####.ext, maybe something like -wpthres-####.ext etc to make it more obvious where the file name came from while providing a pseudo-namespace.

I'm open to changing the file name but it should probably be more descriptive and at the same time quite short. Having the "threshold" value in the name makes sense. Can possibly add wp- in front or perhaps scaled-?

On the other hand appending just the scale size seems to serve that purpose too?

Last edited 5 years ago by azaozz (previous) (diff)

#5 @azaozz
5 years ago

  • Milestone changed from 5.3 to 5.4

Unfortunately this ticket was opened only few hours before 5.3-RC1. Going to move it to the 5.4 milestone for now but it can still be considered for 5.3 if a compelling user case exists.

#6 follow-up: @kraftbj
5 years ago

After discussing this a bit, the technical use case I have is less compelling. The compelling situation I have now that I would suggest 5.3 consideration is for support, developers, etc to be able to more easily identified that an image was scaled or threshold-bound by WP.

For support techs, seeing image-2560.png won't be as obvious as image-scaled-2560.png or something like that for when users/customers have questions about image sizing.

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


5 years ago

@azaozz
5 years ago

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

Replying to kraftbj:

For support techs, seeing image-2560.png won't be as obvious as image-scaled-2560.png or something like that for when users/customers have questions about image sizing.

Yes, makes sense. As this is just a name change, thinking it's acceptable to do during RC. Would still need another committer to sign off.

In 48304.2.diff: append -scaled plus the threshold size to scaled images file names.

#9 @azaozz
5 years ago

  • Keywords 2nd-opinion removed
  • Milestone changed from 5.4 to 5.3

Moving back to 5.3 for consideration.

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


5 years ago

#11 follow-ups: @chrisvanpatten
5 years ago

For whatever it's worth, I want to note that to me the simpler format here is a major advantage of this approach as it allows us to construct the URL to the "threshold" version from any size, without knowing exact dimensions. We were previously filtering core in gross/scary ways to get this behavior, and it's been amazing to be able to revert this code and use a simpler approach in core.

Frankly, I'd like to see this change rolled out across the board; using the image size name as the suffix instead of exact dimensions. I'd be very disappointed to see this go away.

Alternately, it would be nice to see this be filterable; as noted above we were doing very gross/scary hacks to be able to customize the saved file name. If there were a filter in place to be able to easily filter the generated file name, that would more-than alleviate my concerns.

Last edited 5 years ago by chrisvanpatten (previous) (diff)

#12 in reply to: ↑ 11 @desrosj
5 years ago

Replying to chrisvanpatten:

For whatever it's worth, I want to note that to me the simpler format here is a major advantage of this approach as it allows us to construct the URL to the "threshold" version from any size, without knowing exact dimensions. We were previously filtering core in gross/scary ways to get this behavior, and it's been amazing to be able to revert this code and use a simpler approach in core.

Frankly, I'd like to see this change rolled out across the board; using the image size name as the suffix instead of exact dimensions. I'd be very disappointed to see this go away.

Not saying you're use case is unreasonable @chrisvanpatten, but wanted to play devil's advocate. Couldn't custom code just use Core's big_image_size_threshold filter to construct the image name? Something like:

$size = apply_filters( 'big_image_size_threshold', 2560 );

$size_name = 'scaled-' . (int) $size;

Also, if the size is not found in the scaled image's name and a site adds a filter to big_image_size_threshold to change the scaled value, it could become difficult to know which images have been scaled to the correct dimensions and which have not.

Alternately, it would be nice to see this be filterable;

A filter did come to mind here when looking through the changes.

As far as 48304.2.diff goes, I would be +1 to this to make it easier to identified images that were scaled or threshold-bound by WP.

#13 in reply to: ↑ 11 @azaozz
5 years ago

Replying to chrisvanpatten:

I want to note that to me the simpler format here is a major advantage of this approach as it allows us to construct the URL to the "threshold" version from any size, without knowing exact dimensions.
...
Frankly, I'd like to see this change rolled out across the board; using the image size name as the suffix instead of exact dimensions. I'd be very disappointed to see this go away.

As @desrosj points out above 48304.2.diff doesn't change that. It is to just append another (static) string before the threshold value to make these scaled images easier to recognize when looking at the uploads directory directly.

#14 @azaozz
5 years ago

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

In 46565:

Media/Upload: When the users upload big images and WordPress creates a scaled image to use as the largest size, append scaled- to the file names of the scaled images to make them easier to recognize.

Props kraftbj, azaozz.
Fixes #48304.

#15 @chrisvanpatten
5 years ago

A filter did come to mind here when looking through the changes.

To me this is the ultimate problem: right now, $editor->generate_filename is not filterable, and I really feel that it should be. Otherwise, you have to resort to ugly hacks where you watch for core's generating of the files and need to rename the file in place. It would make it radically easier to apply this type of file structure (filename-size.ext) across the board, rather than rely on the unpredictable image names that contain the image sizes.

I'm still pretty disappointed about this, but ultimately recognise why it was made. Ultimately it just means I have to re-enable my ugly hack code to rename files again :)

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


5 years ago

Note: See TracTickets for help on using tickets.