Allow developers to tweak which image formats to generate on upload #227

eugene-manuilov · 2022-03-11T10:22:19Z

Summary

Fixes #187

Relevant technical choices

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

…webp_uploads_upload_image_mime_transforms.

…rly if only the original mime type is needed.

…e correct mime type for new uploads.

modules/images/webp-uploads/load.php

Co-authored-by: Crisoforo Gaspar Hernández <hello@crisoforo.com>

…to return an empty array if a wrong value is returned from the filter.

…rmat-filters

felixarntz

@eugene-manuilov Mostly looks good, I left a few comments for things that can be improved. One of them is crucial, we still need to run through the overall webp_uploads_create_sources_property() function as before.

modules/images/webp-uploads/load.php

…rmat-filters

felixarntz

@eugene-manuilov Awesome!

modules/images/webp-uploads/load.php

adamsilverstein · 2022-03-15T20:56:52Z

Overall this looks great, I will give this a test specifically to verify that the filter works to set output to a single mime type (eg transform jpeg->webp) and also test with and without WebP support.

Can we add some additional tests that validates new functionality as well?

…onstraints.

…rmat-filters

eugene-manuilov · 2022-03-16T17:20:43Z

Overall this looks great, I will give this a test specifically to verify that the filter works to set output to a single mime type (eg transform jpeg->webp) and also test with and without WebP support.

Can we add some additional tests that validates new functionality as well?

@adamsilverstein @felixarntz @mitogh I have found and fixed a few issues while working on phpunit tests. Could you, please, take another look at the implementation?

Also I have refactored a bit our phpunit tests for the webp-uploads module to make it easier to check sources. Let me know what you think.

adamsilverstein · 2022-03-16T17:26:50Z

modules/images/webp-uploads/load.php

@@ -276,7 +278,15 @@ function webp_uploads_get_upload_image_mime_transforms() {
 		return $default_transforms;
 	}

-	return array_filter( $transforms, 'is_array' );
+	// Ensure that all mime types have correct transforms. If a mime type has invalid transforms array,


mitogh

Minor comments, great work on refactoring the tests :)

mitogh · 2022-03-16T17:34:59Z

tests/modules/images/webp-uploads/webp-uploads-test.php

-		$this->assertStringEndsNotWith( '.jpeg', $metadata['sizes']['thumbnail']['sources']['image/webp']['file'] );
-		$this->assertStringEndsWith( '.webp', $metadata['sizes']['thumbnail']['sources']['image/webp']['file'] );
+		$this->assertImageHasSizeSource( 'image/jpeg', 'thumbnail', $attachment_id );
+		$this->assertImageHasSizeSource( 'image/webp', 'thumbnail', $attachment_id );


I think the abstraction works great to ensure the presence of the key this 2 assertions would ensure that the correct file extensions are used and can be used after assertImageHasSizeSource as at that point if the previous assertions pass it can safely be used at this stage.

Suggested change

$this->assertImageHasSizeSource( 'image/webp', 'thumbnail', $attachment_id );

$this->assertImageHasSizeSource( 'image/webp', 'thumbnail', $attachment_id );

$this->assertStringEndsNotWith( '.jpeg', $metadata['sizes']['thumbnail']['sources']['image/webp']['file'] );

$this->assertStringEndsWith( '.webp', $metadata['sizes']['thumbnail']['sources']['image/webp']['file'] );

I have added extensions check to image source constraints.

tests/modules/images/webp-uploads/webp-uploads-test.php

tests/utils/Constraint/ImageHasSizeSource.php

tests/utils/Constraint/ImageHasSource.php

tests/utils/Constraint/ImageHasSizeSource.php

mitogh · 2022-03-16T17:41:38Z

tests/utils/TestCase/ImagesTestCase.php

+	 * @param int    $attachment_id The attachment ID.
+	 * @param string $message An optional message to show on failure.
+	 */
+	public static function assertImageHasSource( $mime_type, $attachment_id, $message = '' ) {


I would update the parameters as the mime type depends on the $attachment_id it makes more sense to set first the attachment_id

Suggested change

public static function assertImageHasSource( $mime_type, $attachment_id, $message = '' ) {

public static function assertImageHasSource( $attachment_id, $expected_mime_type, $message = '' ) {

The current order is used to mimic how the ArrayHasKey constraint work. The first param/params are needly and then goes the target that we want to check.

Right, however by using this abstraction the second argument is no longer an array and if you are unfamiliar with the abstraction itself is difficult to know which is the base of the abstraction like if I want to use the assertion I would expect the first parameter to be the attachment ID similarly as how WP functions work due the expected_mime_type and additional parameters are based out of the attachment ID.

The logic comes from importance order:

Attachment ID -> This is required and would always be present

Size -> This is also present as is a WP API the only difference is the size name in some cases might not be defined

Mime Type -> This is a newly introduced value so it might not be present in most cases.

Based on that order it makes more sense to pass the parameters in that order instead of the other way around.

I don't have a strong opinion on either, but slightly leaning towards @mitogh's point here, since that order is what's more common in WordPress image functions.

The argument around assertArrayHasKey makes sense, but here we're passing the attachment ID, and the assertion is not looking for the size "in" the attachment ID. It's looking for it in the metadata array. If the assertion was assertMetadataHasSource, I think the assertArrayHasKey comparison would make more sense, but because we're passing an attachment ID here, IMO sticking to the order WordPress developers are used to makes more sense.

Ok, sounds good. Updated.

mitogh · 2022-03-16T17:43:10Z

tests/utils/TestCase/ImagesTestCase.php

+	 * @param int    $attachment_id The attachment ID.
+	 * @param string $message An optional message to show on failure.
+	 */
+	public static function assertImageNotHasSource( $mime_type, $attachment_id, $message = '' ) {


Same as above.

Suggested change

public static function assertImageNotHasSource( $mime_type, $attachment_id, $message = '' ) {

public static function assertImageNotHasSource( $attachment_id, $expected_mime_type, $message = '' ) {

Same reason. Just mimic how other constraints in phpunit work.

See my reply above, slightly leaning towards @mitogh's point. But I think either way has its benefits here, I don't have a strong opinion.

Ok, updated.

mitogh · 2022-03-16T17:44:31Z

tests/utils/TestCase/ImagesTestCase.php

+	 * @param int    $attachment_id The attachment ID.
+	 * @param string $message An optional message to show on failure.
+	 */
+	public static function assertImageHasSizeSource( $mime_type, $size, $attachment_id, $message = '' ) {


Suggested change

public static function assertImageHasSizeSource( $mime_type, $size, $attachment_id, $message = '' ) {

public static function assertImageHasSizeSource( $attachment_id, $size, $expected_mime_type, $message = '' ) {

Since you are already using types would be great to specify the types as well for the parameters.

Suggested change

public static function assertImageHasSizeSource( $mime_type, $size, $attachment_id, $message = '' ) {

public static function assertImageHasSizeSource( int $attachment_id, string $size, string $expected_mime_type, string $message = '' ) {

I have added return types to custom constraints because it was required by php to make sure that the signature is compatible with the parent class. I am not sure about adding these types for params. Seems less important to me, but if we agree to add it, I'll update.

mitogh · 2022-03-16T17:45:26Z

tests/utils/TestCase/ImagesTestCase.php

+	 * @param int    $attachment_id The attachment ID.
+	 * @param string $message An optional message to show on failure.
+	 */
+	public static function assertImageNotHasSizeSource( $mime_type, $size, $attachment_id, $message = '' ) {


Similar to the above.

Suggested change

public static function assertImageNotHasSizeSource( $mime_type, $size, $attachment_id, $message = '' ) {

public static function assertImageNotHasSizeSource( $attachment_id, $size, $expected_mime_type, $message = '' ) {

Co-authored-by: Crisoforo Gaspar Hernández <hello@crisoforo.com>

felixarntz

@eugene-manuilov Great work refactoring the tests with the extra assertions! I have some minor comments, but nothing blocking.

felixarntz · 2022-03-16T19:13:49Z

tests/utils/TestCase/ImagesTestCase.php

+	 * @param int    $attachment_id The attachment ID.
+	 * @param string $message An optional message to show on failure.
+	 */
+	public static function assertImageHasSource( $mime_type, $attachment_id, $message = '' ) {


I don't have a strong opinion on either, but slightly leaning towards @mitogh's point here, since that order is what's more common in WordPress image functions.

The argument around assertArrayHasKey makes sense, but here we're passing the attachment ID, and the assertion is not looking for the size "in" the attachment ID. It's looking for it in the metadata array. If the assertion was assertMetadataHasSource, I think the assertArrayHasKey comparison would make more sense, but because we're passing an attachment ID here, IMO sticking to the order WordPress developers are used to makes more sense.

felixarntz · 2022-03-16T19:15:25Z

tests/utils/TestCase/ImagesTestCase.php

+	 * @param int    $attachment_id The attachment ID.
+	 * @param string $message An optional message to show on failure.
+	 */
+	public static function assertImageNotHasSource( $mime_type, $attachment_id, $message = '' ) {


See my reply above, slightly leaning towards @mitogh's point. But I think either way has its benefits here, I don't have a strong opinion.

felixarntz · 2022-03-16T19:17:34Z

composer.json

+  "autoload-dev": {
+    "psr-4": {
+      "PerformanceLab\\Tests\\": "tests/utils"
+    }
  }


Not sure Composer usage like this would play well together with WordPress core, but since it's only tests IMO it should be okay at least in terms of this plugin. The classes would need to be renamed for core anyway.

Do you mean I need to rename util classes to follow class-... pattern? If so, can we make an exception for phpunit classes only?

WordPress core still doesn't use any autoloading, there's always some sort of "includes" file that require_onces all files. Obviously not great, but that's how I would do it, for maximum "compatibility" with the core paradigms.

However, since this is purely about PHPUnit classes and we're in this plugin, I'm not opposed to this (obviously better) approach :)

Awesome! Thanks 😄

adamsilverstein · 2022-03-16T19:26:39Z

I did some testing to verify that the filter mapping works as expected, using the filter to test the following scenarios when uploading both jpegs and webp images:

generate only WebP sub-sizes
generate only jpeg sub-sizes
generate both WebP and jpeg sub-sizes
generate both, reversing the default

In code:

add_filter( 'webp_uploads_upload_image_mime_transforms', function() {
	$test = 'only-webp'; // Try each

	switch ( $test ) {
		case 'only-webp':
			return [
				'image/jpeg' => [ 'image/webp' ],
				'image/webp' => [ 'image/webp' ],
			];
		case 'only-jpg':
			return [
				'image/jpeg' => [ 'image/jpeg' ],
				'image/webp' => [ 'image/jpeg' ],
			];
		case 'both':
			return [
				'image/jpeg' =>  [ 'image/jpeg', 'image/webp' ],
				'image/webp' =>  [ 'image/webp', 'image/jpeg' ],
			];
		case 'reverse':
			return [
				'image/jpeg' =>  [ 'image/webp', 'image/jpeg' ],
				'image/webp' =>  [ 'image/jpeg', 'image/webp' ],
			];
		case 'empty':
		default:
			return [];
	}

});

I reviewed the files generated as well as the meta data. Comments below:

✅ only-webp: worked as expected, only webp images generated for either webp or jpeg uploads
⚠️ only-jpeg: worked for jpegs, only jpeg was generated. when I uploaded a WebP it generated webp, I was expecting jpeg - not sure of the use case, but was expecting this to work
✅ both: worked as expected, both formats created including -scaled version, worked for both jpeg and webp uploads
❌ reverse: didn't work as expected - behavior was the same as 'both', I was expecting the alternate mime type to be stored as the "primary" image, meaning stored in the sizes array, and first in the sources arrays. Not sure this is an issue or if others will expect this behavior as well. Not sure this is completely needed here, this is a little trickier to achieve in the plugin, for the core patch it will be much easier to just start with the first mime type.
✅ 'empty': returning an empty array worked as expected, essentially disabling any additional mime type generation. When I uploaded a WebP only WebP sub-sizes were generated, when I uploaded a JPEG, only JPEG sub-sizes were created.

adamsilverstein

I noticed some issues in testing that might be worth fixing, please review comment

…beginning.

eugene-manuilov · 2022-03-17T11:32:25Z

⚠️ only-jpeg: worked for jpegs, only jpeg was generated. when I uploaded a WebP it generated webp, I was expecting jpeg - not sure of the use case, but was expecting this to work

Hm... it works correctly on my end. When I upload a webp image, all subsizes have been created in the jpeg format. The original image is still in webp though. Is it what you mean, @adamsilverstein?

eugene-manuilov · 2022-03-17T11:41:21Z

❌ reverse: didn't work as expected - behavior was the same as 'both', I was expecting the alternate mime type to be stored as the "primary" image, meaning stored in the sizes array, and first in the sources arrays. Not sure this is an issue or if others will expect this behavior as well. Not sure this is completely needed here, this is a little trickier to achieve in the plugin, for the core patch it will be much easier to just start with the first mime type.

@adamsilverstein see #187 (comment):

If the filter is being used in a way so that for a given upload's original MIME type that same MIME type is not part of the transformation array (e.g. a developer decides that no JPEGs should be generated even if a JPEG is uploaded), the first format in the transformation list should be used as the default format to generate the image sizes for. In order to force WordPress core to not generate the original MIME type, the image_editor_output_format needs to be used. A callback should be added to that which calls the webp_uploads_get_supported_image_mime_transforms() function firing the filter and if the original format is not in the list, the filter value should be modified to generate the appropriate other image format. For that purpose and this particular scenario, the original implementation of the WebP Uploads module (see #32) should essentially be reintroduced.

Basically, what it says is that we need to select the alternate mime type for the "primary" image only if the original mime type is not included into the transformation list which is not the case in your situation. Does it make sense to you or I miss something?

mitogh

Few minor comments, other than that great job 👍

tests/modules/images/webp-uploads/webp-uploads-test.php

Rename the webp_uploads_supported_image_mime_transforms filter to be …

57cfc27

…webp_uploads_upload_image_mime_transforms.

eugene-manuilov added [Type] Enhancement A suggestion for improvement of an existing feature [Focus] Images Issues related to the Images focus area [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Mar 11, 2022

eugene-manuilov added this to the 1.0.0-beta.2 milestone Mar 11, 2022

eugene-manuilov added 4 commits March 11, 2022 16:09

Update the webp_uploads_create_sources_property function to return ea…

c1550b2

…rly if only the original mime type is needed.

Add a new hook for the image_editor_output_format filter to select th…

04e67a6

…e correct mime type for new uploads.

Fix the phpcs issue.

daf8620

Update phpdoc for the webp_uploads_upload_image_mime_transforms filter.

579f7a1

eugene-manuilov marked this pull request as ready for review March 11, 2022 15:13

eugene-manuilov requested review from adamsilverstein and getsource as code owners March 11, 2022 15:13

eugene-manuilov mentioned this pull request Mar 11, 2022

Allow developers to tweak which image formats should be used #187

Closed

Fix formatting issue.

87cc7f2

mitogh reviewed Mar 11, 2022

View reviewed changes

eugene-manuilov and others added 4 commits March 12, 2022 10:26

Apply suggestions from code review

0b3b687

Co-authored-by: Crisoforo Gaspar Hernández <hello@crisoforo.com>

Update the webp_uploads_get_supported_image_mime_transforms function …

dd30d0f

…to return an empty array if a wrong value is returned from the filter.

Remove "-scaled" condition.

a8911bc

Merge remote-tracking branch 'origin/trunk' into feature/187-image-fo…

3a04771

…rmat-filters

felixarntz reviewed Mar 14, 2022

View reviewed changes

eugene-manuilov added 2 commits March 15, 2022 15:13

Merge remote-tracking branch 'origin/trunk' into feature/187-image-fo…

4024e3d

…rmat-filters

Address code review feedback.

f118537

eugene-manuilov requested a review from felixarntz March 15, 2022 13:32

felixarntz approved these changes Mar 15, 2022

View reviewed changes

modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved

modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved

felixarntz requested a review from mitogh March 15, 2022 18:15

Update formatting.

401ecd6

felixarntz mentioned this pull request Mar 16, 2022

Allow developers to select which image format to use for images in the content #230

Merged

3 tasks

felixarntz changed the title ~~Allow developers to tweak which image format to use~~ Allow developers to tweak which image formats to generate on upload Mar 16, 2022

Refactor webp-uploads tests by moving the same checks into separate c…

66fea39

…onstraints.

eugene-manuilov added 4 commits March 16, 2022 18:44

Refactor wepb-uploads tests.

1ef8544

Add tests for the new functionality and fix found issues.

fe32ee3

Merge remote-tracking branch 'origin/trunk' into feature/187-image-fo…

cd40fa0

…rmat-filters

Fix formatting issues.

77737da

eugene-manuilov requested a review from felixarntz March 16, 2022 17:18

adamsilverstein reviewed Mar 16, 2022

View reviewed changes

mitogh reviewed Mar 16, 2022

View reviewed changes

Update tests/modules/images/webp-uploads/webp-uploads-test.php

b341963

Co-authored-by: Crisoforo Gaspar Hernández <hello@crisoforo.com>

felixarntz approved these changes Mar 16, 2022

View reviewed changes

felixarntz requested review from adamsilverstein and mitogh March 16, 2022 19:17

adamsilverstein reviewed Mar 16, 2022

View reviewed changes

eugene-manuilov added 2 commits March 17, 2022 11:34

Update custom constraints to have the attachment ID parameter at the …

630929e

…beginning.

Add the file extension check to image source constraints.

1babcc9

eugene-manuilov requested review from felixarntz and adamsilverstein March 17, 2022 11:42

felixarntz approved these changes Mar 17, 2022

View reviewed changes

mitogh reviewed Mar 17, 2022

View reviewed changes

tests/modules/images/webp-uploads/webp-uploads-test.php Show resolved Hide resolved

tests/modules/images/webp-uploads/webp-uploads-test.php Show resolved Hide resolved

mitogh approved these changes Mar 17, 2022

View reviewed changes

felixarntz merged commit 160bb6d into trunk Mar 17, 2022

felixarntz deleted the feature/187-image-format-filters branch March 17, 2022 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow developers to tweak which image formats to generate on upload #227

Allow developers to tweak which image formats to generate on upload #227

eugene-manuilov commented Mar 11, 2022 •

edited by bethanylang

felixarntz left a comment

felixarntz left a comment

adamsilverstein commented Mar 15, 2022

eugene-manuilov commented Mar 16, 2022

adamsilverstein Mar 16, 2022

mitogh left a comment

mitogh Mar 16, 2022

eugene-manuilov Mar 17, 2022

mitogh Mar 16, 2022

eugene-manuilov Mar 16, 2022

mitogh Mar 16, 2022

felixarntz Mar 16, 2022

eugene-manuilov Mar 17, 2022

mitogh Mar 16, 2022

eugene-manuilov Mar 16, 2022

felixarntz Mar 16, 2022

eugene-manuilov Mar 17, 2022

mitogh Mar 16, 2022

eugene-manuilov Mar 16, 2022

mitogh Mar 16, 2022

eugene-manuilov Mar 17, 2022

felixarntz left a comment

felixarntz Mar 16, 2022

felixarntz Mar 16, 2022

felixarntz Mar 16, 2022

eugene-manuilov Mar 17, 2022

felixarntz Mar 17, 2022

eugene-manuilov Mar 17, 2022

adamsilverstein commented Mar 16, 2022 •

edited

adamsilverstein left a comment

eugene-manuilov commented Mar 17, 2022

eugene-manuilov commented Mar 17, 2022 •

edited

mitogh left a comment

	public static function assertImageHasSource( $mime_type, $attachment_id, $message = '' ) {
	public static function assertImageHasSource( $attachment_id, $expected_mime_type, $message = '' ) {

	public static function assertImageNotHasSource( $mime_type, $attachment_id, $message = '' ) {
	public static function assertImageNotHasSource( $attachment_id, $expected_mime_type, $message = '' ) {

	public static function assertImageHasSizeSource( $mime_type, $size, $attachment_id, $message = '' ) {
	public static function assertImageHasSizeSource( $attachment_id, $size, $expected_mime_type, $message = '' ) {

Allow developers to tweak which image formats to generate on upload #227

Allow developers to tweak which image formats to generate on upload #227

Conversation

eugene-manuilov commented Mar 11, 2022 • edited by bethanylang

Summary

Relevant technical choices

Checklist

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

adamsilverstein commented Mar 15, 2022

eugene-manuilov commented Mar 16, 2022

Choose a reason for hiding this comment

mitogh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsilverstein commented Mar 16, 2022 • edited

adamsilverstein left a comment

Choose a reason for hiding this comment

eugene-manuilov commented Mar 17, 2022

eugene-manuilov commented Mar 17, 2022 • edited

mitogh left a comment

Choose a reason for hiding this comment

eugene-manuilov commented Mar 11, 2022 •

edited by bethanylang

adamsilverstein commented Mar 16, 2022 •

edited

eugene-manuilov commented Mar 17, 2022 •

edited