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)
#2
@
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.
#4
@
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
mysqlis unavailable in this session)*
#6
@
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.
#7
@
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.
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
Hi, Thanks for bringing this up.
I propose a solution, The fix adds a
-croppedsuffix to cropped image filenames in thegenerate_filename()method of theWP_Image_Editorclass. This ensures unique filenames for cropped and uncropped versions.Please let me know if we can do better.
Thanks.