Fix bug that would prevent uploaded images from being converted to the intended output format when having fallback formats enabled

[mukeshpanchal27]

Summary

Fixes #1634

[mukeshpanchal27]

There are currently 5 unit tests which are failing on trunk:

There were 5 failures:

1) Test_WebP_Uploads_Load::test_it_should_not_create_the_original_mime_type_for_jpeg_images with data set "webp" ('webp')

Failed asserting that '2024/11/leaves-17-1080x720.webp' ends with "leaves-17-1080x720-webp.webp".
/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-load.php:63

2) Test_WebP_Uploads_Load::test_it_should_not_create_the_original_mime_type_for_jpeg_images with data set "avif" ('avif')

Failed asserting that '2024/11/leaves-18-1080x720.avif' ends with "leaves-18-1080x720-avif.avif".
/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-load.php:63

3) Test_WebP_Uploads_Load::test_it_should_remove_the_generated_webp_images_when_the_attachment_is_deleted

Failed asserting that file "/var/www/html/wp-content/uploads/2024/11/leaves-28-1080x720-webp.webp" does not exist.
/var/www/html/wp-content/plugins/performance/vendor/yoast/phpunit-polyfills/src/Polyfills/AssertionRenames.php:113
/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-load.php:343

4) Test_WebP_Uploads_Load::test_it_should_remove_the_attached_webp_version_if_the_attachment_is_force_deleted

Failed asserting that file "/var/www/html/wp-content/uploads/2024/11/leaves-28-1080x720-webp.webp" does not exist.
/var/www/html/wp-content/plugins/performance/vendor/yoast/phpunit-polyfills/src/Polyfills/AssertionRenames.php:113
/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-load.php:368

5) Test_WebP_Uploads_Load::test_it_should_remove_full_size_images_when_no_size_image_exists

Failed asserting that file "/var/www/html/wp-content/uploads/2024/11/leaves-28-1080x720-webp.webp" does not exist.
/var/www/html/wp-content/plugins/performance/vendor/yoast/phpunit-polyfills/src/Polyfills/AssertionRenames.php:113
/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-load.php:391

The issue is in the core, it change the metadata for file key and our plugin use that key. Check WordPress/wordpress-develop#7733 (comment) for more details.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: adamsilverstein <[email protected]>
Co-authored-by: felixarntz <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: swissspidy <[email protected]>
Co-authored-by: azaozz <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[adamsilverstein]

Thanks!

[mukeshpanchal27]

@adamsilverstein Have you had chance to review the failed unit tests for trunk? Is it expected behaviour for trunk? Have you review WordPress/wordpress-develop#7733?

[felixarntz]

@mukeshpanchal27 I'm a bit concerned about simply removing the assertions. Why is that the right choice?

To me, these assertions failing means that something may be broken by the Core changes, that we either have to fix in the plugin or in Core. Or can you clarify why the expected behavior should have changed now?

[felixarntz]

On another note: It looks like the following tests are doing exactly the same:

• test_it_should_create_jpeg_and_webp_for_jpeg_images_if_opted_in
• test_it_should_create_jpeg_and_webp_for_jpeg_images_if_generate_webp_and_jpeg_set

One of them should probably be removed?

[felixarntz]

@mukeshpanchal27 @adamsilverstein I found the actual problem here: It's a bug that's been in the plugin for a long time, but so far would only have affected images above the "big image" threshold. We just don't have any tests with a large image, so we should definitely add one with an image above that threshold to have that covered in the future.

Here's the problem:

• The plugin relies on the post_mime_type to know what format the image has. This leads to the sources property having entries like 'image/jpeg' => array( /* data about an AVIF or WebP image */ ) which makes no sense.
• Using post_mime_type is unreliable, as WordPress Core does not update post_mime_type when the image format is changed.
  • While it may be worth thinking about changing that, this is how WordPress Core has always worked, so it's an existing problem we just never spotted.
  • FWIW our plugin doesn't update the post_mime_type either.
• We need to instead get the MIME type from the actual file, if possible. See the following code adjustment for the beginning of the webp_uploads_create_sources_property() function:

function webp_uploads_create_sources_property( array $metadata, int $attachment_id ): array {
	$file = get_attached_file( $attachment_id, true );
	// File does not exist.
	if ( false === $file || ! file_exists( $file ) ) {
		return $metadata;
	}

	/*
	 * We need to get the MIME type ideally from the file, as WordPress Core may have already altered it.
	 * The post MIME type is typically not updated during that process.
	 */
	$filetype = wp_check_filetype( $file );
	if ( $filetype['type'] ) {
		$mime_type = $filetype['type'];
	} else {
		$mime_type = get_post_mime_type( $attachment_id );
	}

	$valid_mime_transforms = webp_uploads_get_upload_image_mime_transforms();

	// Not a supported mime type to create the sources property.
	if ( ! is_string( $mime_type ) || ! isset( $valid_mime_transforms[ $mime_type ] ) ) {
		return $metadata;
	}

	// Make sure the top level `sources` key is a valid array.
	if ( ! isset( $metadata['sources'] ) || ! is_array( $metadata['sources'] ) ) {
		$metadata['sources'] = array();
	}

	// Remaining code can probably stay as is.
}

Additionally to fixing that, there's a few more places in the plugin where we call get_post_mime_type(), so we should validate this doesn't cause other bugs in those places or fix them accordingly.

Last but not least, as mentioned let's add test coverage using an image that is above the "big image" threshold. The easiest way is probably to keep using the existing tests, but expand the dataProvider approach to not only specify the MIME type to use but also the source file. This way we ensure the expected outcome is the same regardless of image size, which should ideally be the case.

[swissspidy]

Last but not least, as mentioned let's add test coverage using an image that is above the "big image" threshold. The easiest way is probably to keep using the existing tests, but expand the dataProvider approach to not only specify the MIME type to use but also the source file. This way we ensure the expected outcome is the same regardless of image size, which should ideally be the case.

Or just use the big_image_size_threshold filter to reduce the number a little bit? Otherwise tests will take a longer time with larger images.

[adamsilverstein]

Using post_mime_type is unreliable, as WordPress Core does not update post_mime_type when the image format is changed.

get_post_mime_type should always return the mime type of the uploaded image (input type).. It looks like here we want the output mime type, is that right?

[felixarntz]

Using post_mime_type is unreliable, as WordPress Core does not update post_mime_type when the image format is changed.

get_post_mime_type should always return the mime type of the uploaded image (input type).. It looks like here we want the output mime type, is that right?

It always returns the MIME type of the uploaded image, but here we need the MIME type that the image is currently in, which may or may not be the output MIME type. The issue is that Core may already have processed the image before this function in our plugin is called, and at that point get_post_mime_type would still return the originally uploaded image MIME type, not the MIME type the attachment file is actually in.

[azaozz]

It always returns the MIME type of the uploaded image, but here we need the MIME type that the image is currently in

Yea, this is a limitation of the image metadata: it doesn't store the actual mime type of the uploaded image if it was converted. One workaround to speed this up (that I think is still used in core) is to look at the first item from the sizes array where the actual mime-type is set. Of course it would be better to get the mime-type from the file if speed is not a big concern.

[felixarntz]

@mukeshpanchal27 Thanks for the update, this fixes the test failures. Just two non-blocking notes.

Additionally, please consider my note about other usages of get_post_mime_type(). I just checked, and the one in webp_uploads_wrap_image_in_picture() looks incorrect to me as well, for the same reason here. Maybe we could take the logic you have here in lines 65-70 and move them to their own helper function? That way we could use that in both places. This could be done in a separate PR though, as it's not causing test failures today.

[mukeshpanchal27]

This could be done in a separate PR though, as it's not causing test failures today.

That's the plan once these PR merged.