Make WordPress Core

Opened 2 years ago

Last modified 6 weeks ago

#62388 reopened defect (bug)

If image size exists with crop and without crop, only 1 will be created,

Reported by: kkmuffme Owned by:
Priority: normal Milestone:
Component: Media Version:
Severity: major Keywords: has-patch has-unit-tests
Cc: Focuses:

Description

Essentially https://core.trac.wordpress.org/ticket/61925 but if the same size exists with crop true and crop false, then whatever size was added later will overwrite a previous one, since the resulting file names are identical.
e.g. foo-450x450.png would be created first uncropped, then cropped resulting in only the cropped file being available.

So while the other ticket is purely an efficiency one, this is a bug since it's unexpected that the order in which the sizes are added matters and you could end up with cropped images when you added an uncropped size or vice versa

Change History (7)

#1 @abcd95
2 years ago

Hi, Thanks for bringing this up.

I propose a solution, The fix adds a -cropped suffix to cropped image filenames in the generate_filename() method of the WP_Image_Editor class. This ensures unique filenames for cropped and uncropped versions.

Please let me know if we can do better.

Thanks.

#2 @kkmuffme
2 years ago

  • Resolution → duplicate
  • Status new → closed

Duplicate of #61925.

Turns out WP is not cropping at all in the first place, but crop refers to enforcing either one or both provided dimensions https://core.trac.wordpress.org/ticket/62389#ticket

So this is a duplicate of the referenced issue then, but something one has to be aware of, since we can also skip the crop if the dimensions would scale/scaled dimensions match the crop requested dimensions

@abcd95 thanks for your reply. Next time check the issue at hand first, because your comment is not helpful or adding anything to the conversation.

#3 @desrosj
23 months ago

  • Milestone Awaiting Review

#4 @nickchomey
4 months ago

  • Keywords needs-patch added
  • Resolution duplicate
  • Status closed → reopened

As my comments in #62389 explain, this issue is actually valid, whereas the superseding #62389 is invalid and now closed.

If an image size has the same dimensions but different crop values (notably, its not just a true/false boolean, but bool|array where array can be

array( 'left', 'top' ) );

x_crop_position accepts ‘left’ ‘center’, or ‘right’.
y_crop_position accepts ‘top’, ‘center’, or ‘bottom’.

​https://developer.wordpress.org/reference/functions/add_image_size/#crop-mode

So, it needs to be determined how to treat different crops of same image size.

@abcd95's suggestion of appending -cropped seems directionally correct, but not sufficient due to possible combinations of the crop positions.

Seems to me that something like -crop-x_crop-y_crop (eg -crop-left-bottom) would be sufficient.

An alternative is creating a hash of those if we think that that string looks too cumbersome. I dont really like this idea though, as people might very well want to review the different crops.

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


4 months ago
#5

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

## Summary

  • add explicit crop-aware filename suffixes when multiple generated image sizes share the same target dimensions
  • keep normal sub-size filenames unchanged unless there is a duplicate-dimension collision risk
  • add a regression test covering duplicate cropped sizes generated during attachment metadata creation

## Testing

  • php -l src/wp-admin/includes/image.php
  • php -l src/wp-includes/class-wp-image-editor-gd.php
  • php -l src/wp-includes/class-wp-image-editor-imagick.php
  • php -l tests/phpunit/tests/media.php
  • php vendor/bin/phpcbf src/wp-admin/includes/image.php src/wp-includes/class-wp-image-editor-gd.php src/wp-includes/class-wp-image-editor-imagick.php tests/phpunit/tests/media.php
  • php vendor/bin/phpcs src/wp-admin/includes/image.php src/wp-includes/class-wp-image-editor-gd.php src/wp-includes/class-wp-image-editor-imagick.php tests/phpunit/tests/media.php
  • php vendor/bin/phpunit tests/phpunit/tests/media.php --filter test_wp_generate_attachment_metadata_adds_unique_suffixes_for_duplicate_cropped_sizes *(fails locally: test DB host mysql is unavailable in this session)*

#6 @arkaprabhachowdhury
7 weeks ago

Opened [PR #12232](​https://github.com/WordPress/wordpress-develop/pull/12232) to address this ticket. It adds crop-aware filename suffixes when generated image sizes share target dimensions, preserves normal filenames otherwise, and adds regression coverage for duplicate cropped sizes.

I picked this up because the ticket describes a media-filename collision edge case that remains relevant in current WordPress. The change is scoped to the ticket and submitted here for code review.

Last edited 7 weeks ago by arkaprabhachowdhury (previous) (diff)

#7 @ncl8p
6 weeks ago

to fix this issue, WordPress needs to stop giving different versions of an image the exact same filename.

When two image sizes ask for the same dimensions (like 450 by 450 pixels) but use different cropping rules, WordPress should check for that overlap before creating the files. If it spots a conflict, it should add a label to the end of the filename based on how the image is cropped—such as adding -cropped or -crop-left-top.

For normal images that don't share dimensions with another size, the filenames will stay exactly as they are so existing plugins and themes don't break.

Note: See TracTickets for help on using tickets.