Make WordPress Core

Opened 18 months ago

Closed 17 months ago

Last modified 16 months ago

#57370 closed defect (bug) (fixed)

WP_Image_Editor_Imagick creates unexpected 'thumbnail' image size when original image is 150x150

Reported by: danielbachhuber's profile danielbachhuber Owned by: danielbachhuber's profile danielbachhuber
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Discovered with https://github.com/wp-cli/media-command/pull/173

If you have brew install imagemagick and pecl install imagick, the issue reproduces with pretty easily with WP-CLI:

$ wp site empty --uploads --yes && wp db reset --yes && wp core install && wp option update uploads_use_yearmonth_folders 0
Success: The site at 'https://vanilla.test' was emptied.
Success: Database reset.
Success: WordPress installed successfully.
Success: Updated 'uploads_use_yearmonth_folders' option.
$ wget http://wp-cli.org/behat-data/white-150-square.jpg
$ wp media import white-150-square.jpg --title="My imported small attachment"
Imported file 'white-150-square.jpg' as attachment ID 4.
Success: Imported 1 of 1 items.
$ ls wp-content/uploads
white-150-square-150x150.jpg   white-150-square.jpg

Only white-150-square.jpg should be present, not white-150-square-150x150.jpg.

The underlying problem is that WP_Image_Editor_Imagick->resize() returns true (https://github.com/WordPress/wordpress-develop/blob/e993bc5f739af0ac589049c1f102788f57a1983d/src/wp-includes/class-wp-image-editor-imagick.php#L270-L273), and then make_subsize() proceeds to save a new image anyway (https://github.com/WordPress/wordpress-develop/blob/e993bc5f739af0ac589049c1f102788f57a1983d/src/wp-includes/class-wp-image-editor-imagick.php#L506-L516)

The behavior doesn't happen with WP_Image_Editor_GD because it has some is_gd_image() check (https://github.com/WordPress/wordpress-develop/blob/e993bc5f739af0ac589049c1f102788f57a1983d/src/wp-includes/class-wp-image-editor-gd.php#L178-L185)

Change History (18)

This ticket was mentioned in PR #3796 on WordPress/wordpress-develop by danielbachhuber.


18 months ago
#1

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

#2 @danielbachhuber
18 months ago

https://github.com/WordPress/wordpress-develop/pull/3796 is one approach at a fix, although I'm not 100% confident it's the most appropriate approach.

@NekoJonez commented on PR #3796:


18 months ago
#3

Is that new error translate-able?

danielbachhuber commented on PR #3796:


18 months ago
#4

Is that new error translate-able?

@NekoJonez It's wrapped in __() so it should be? Is there something specific you're uncertain about?

@NekoJonez commented on PR #3796:


18 months ago
#5

Is that new error translate-able?

@NekoJonez It's wrapped in __() so it should be? Is there something specific you're uncertain about?

Maybe I am confusing with textdomains and such.

#6 @wojtekn
18 months ago

@danielbachhuber good catch!

I tried to validate it on the wp-env environment, but I can't reproduce the current behavior that we consider incorrect:

  1. I open SSH for the container:
wp-env run cli bash
  1. I ensure everything is clean:
wp site empty --uploads --yes && wp option update uploads_use_yearmonth_folders 0
Success: The site at 'http://localhost:8888' was emptied.
Success: Value passed for 'uploads_use_yearmonth_folders' option is unchanged.
  1. I download the test image:
wget http://wp-cli.org/behat-data/white-150-square.jpg
'white-150-square.jpg' saved
  1. I import the file:
wp media import white-150-square.jpg --title="My imported small attachment"
Imported file 'white-150-square.jpg' as attachment ID 1.
Success: Imported 1 of 1 items.
  1. I see no additional thumbnail created:
ls wp-content/uploads
white-150-square.jpg

wp-env uses Imagick:

$ php -i | grep imag
/usr/local/etc/php/conf.d/docker-php-ext-imagick.ini,
imagick
imagick module => enabled
imagick module version => 3.6.0
imagick classes => Imagick, ImagickDraw, ImagickPixel, ImagickPixelIterator, ImagickKernel
Imagick compiled with ImageMagick version => ImageMagick 7.1.0-50 beta Q16-HDRI aarch64 20489 https://imagemagick.org

Maybe it happens only for ImageMagick 6.x?

#7 @danielbachhuber
18 months ago

@wojtekn I'm using 7.1.0-55:

$ brew info imagemagick
==> imagemagick: stable 7.1.0-55 (bottled), HEAD
Tools and libraries to manipulate images in many formats
https://imagemagick.org/index.php
/opt/homebrew/Cellar/imagemagick/7.1.0-55 (807 files, 31.0MB) *
  Poured from bottle on 2022-12-21 at 10:47:11
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/imagemagick.rb
License: ImageMagick
==> Dependencies
Build: pkg-config ✔
Required: freetype ✔, ghostscript ✔, jpeg-turbo ✔, libheif ✔, liblqr ✔, libpng ✔, libraw ✔, libtiff ✔, libtool ✔, little-cms2 ✔, openexr ✔, openjpeg ✔, webp ✔, xz ✔, libomp ✔
==> Options
--HEAD
	Install HEAD version
==> Analytics
install: 184,160 (30 days), 470,977 (90 days), 1,972,966 (365 days)
install-on-request: 162,236 (30 days), 418,097 (90 days), 1,745,374 (365 days)
build-error: 19 (30 days)

#8 @wojtekn
17 months ago

@danielbachhuber I reproduced the issue with PHP and MariaDB installed by homebrew:

  • PHP 8.0.26
  • mariadb 10.10.2
  • imagick 3.7.0 with ImageMagick 7.1.0-57 (Beta) Q16-HDRI aarch64 20701

Now when I run wp media import white-150-square.jpeg --title="My imported small attachment" I get two files in the wp-content/uploads directory:

  • white-150-square-150x150.jpeg
  • white-150-square.jpeg

When I apply your patch, I get only one file. It looks like it fixes the issue.

I'm still unsure for which environment setup this issue pops out, but it seems that with your fix, we may have consistent behavior across environments.

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


17 months ago

#10 @antpb
17 months ago

  • Milestone changed from Awaiting Review to 6.2

Moving this into 6.2 to investigate. Worth noting that I think this issue already in 6.2 is related: #48485

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


17 months ago

#12 @danielbachhuber
17 months ago

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

In 55278:

Media: Bail early if image is already the requested size.

In WP_Image_Editor_Imagick, bail early in make_subsize() if the image is already the requested size. Previously, make_subsize() would create another copy of the file. WP_Image_Editor_GD doesn't have the same problem.

Props wojtekn, danielbachhuber.
Fixes #57370.

#14 @danielbachhuber
17 months ago

In 55280:

Media: Add test file missed in [55278].

See #57370.

#16 @danielbachhuber
16 months ago

In 55300:

Media: Use strict comparison in make_subsize().

Follow up from [55278].

Props bueltge, desrosj, mukesh27.
See #57370.

#18 @hellofromTonya
16 months ago

In 55467:

Tests: Improve Tests_Media::test_wp_generate_attachment_metadata_doesnt_generate_sizes_for_150_square_image().

Changes:

  • from assertEquals() to assertSame(). Why? To ensure both the return value and data type match the expected results.
  • the expected height and width from string to integer data types. Why integer? getimagesize() (within wp_getimagesize()) will return an integer for both height and weight.
  • adds the ticket annotation.
  • adds assertion failure messages. Why? To denote which assertion failed, which aids in debugging efforts.

Follow-up to [55278].

Props costdev, peterwilsoncc, mukesh27, ankitmaru, hellofromTonya.
See #56800, #57370.

Note: See TracTickets for help on using tickets.