Create image sub-sizes in additional MIME types using `sources` for storage. #147

mitogh · 2022-02-03T01:57:11Z

Summary

The property is added to every single image size available, each value inside the properties array is a reference to
a different image mime type.

If the image size was created as a JPEG version a WebP version would be created, if a WebP version exists a JPEG version
would be created instead based on the details from ticket #142

This mechanism would allow extending the sources property for more additional mime types.

Fixes:

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.

The property is added to every single image size available, each value inside the properties array is a reference to a different image mime type. If the image size was created as a JPEG version a WebP version would be created, if a WebP version exists a JPEG version would be created instead based on the details from ticket #142 This mechanism would allow extending the sources properties for more additional mime types. The value for each mime type is an associative array with `file` as the only property for now. And the plan is to include more meaningful properties such as `filesize` to a different mime type.

Make sure file follows the standard from `phpcs`

Prevent to use numeric indexes for each image size, instead use the correct image name.

felixarntz

@mitogh This looks like a good start, however there are a couple things that need to be refined. I left some comments.

modules/images/webp-uploads/load.php

felixarntz · 2022-02-17T16:20:47Z

Summarizing a chat conversation on this between @mitogh, @adamsilverstein and myself:

We're in agreement to proceed with this approach since it's a more appropriate way to store the additional MIME type images, we can probably close the alternative approach Store the backup JPEG images in _wp_attachment_backup_sizes #154 then.
One high-level enhancement that still needs to be added to this PR before it can be merged is ensure that the new files are also being deleted when the image is deleted. We need to hook into the delete_attachment action for that.
The other thing that needs to be added is that for every new file generated, we need to update the attachment metadata immediately after, to allow for the process to fail gracefully and to be able to proceed with only the missing sizes on next invokation:
- Introduce a function to check for which sizes are still missing in the extra MIME type(s) to generate. For this we can probably create a copy of WP core's wp_get_missing_image_subsizes() function and add a MIME type parameter to it. For an eventual core merge, the real function could then be enhanced similarly.
- Adjust the logic in our wp_generate_attachment_metadata filter callback to always only generate versions for the missing MIME types, using the above function.
- Call wp_update_attachment_metadata() every time a new image sub-size file is generated, i.e. store the sources field just for that image sub-size file generated and immediately store it.
In a separate PR later, we need to integrate this approach also with the feature of editing images, i.e. when an image is edited within WordPress, we also need to generate versions in the additional MIME types for the edited versions, and we need to make sure the original sources are then included in the _wp_attachment_backup_sizes.

Use a function that already exists in core, no need to define a new function if core already provides this information.

Based on feedback on code review hooks where moved closer where the callbacks for each is defined.

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

…om:WordPress/performance into feature/142-multiple-mimes-for-image-sizes

Make sure function is only called once, due `wp_get_registered_image_subsizes` does not return a function.

This class is no longer required or used in the set of tests, so there's no real value in keeping the class in place.

The added images are going to be used as part of the tests suite.

The `load` method is required by the editor to process an image.

The result of the conditional can be used instead.

For every single image size a new mime type would be created if the original mime type of the image supports the specified mime type. Currently this only works with: `image/jpg` and `image/webp` and vice-versa. Only for all the available image sizes except the full size image. This PR fixes: #142, #8

When an attachment is deleted all the generated WebP versions of an attachment should be removed when the main attachment is removed.

Make sure the file follows the same format as the rest of the project by following the same guidelines.

modules/images/webp-uploads/load.php

felixarntz

@mitogh I left a few last nit-pick comments. Other than those, the one critical thing here is that I still think the cron usage isn't really appropriate here, since we should tie into WP core behavior, which doesn't have that. See my comment https://github.com/WordPress/performance/pull/147/files#r813258590 for more details.

modules/images/webp-uploads/load.php

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Move the WebP image generation into a single process instead of creating individual cron for each image.

adamsilverstein

Excellent work!

felixarntz

@mitogh All great now! Except can you please replace all the remaining usages of array_key_exists here with isset as outlined before?

Use `isset` as we are just interested if the key exists even if is `null`

mitogh · 2022-02-24T19:56:04Z

Sure @felixarntz just updated the code based on your feedback.

felixarntz

Awesome work @mitogh! 🎉

Let's proceed in #188 💪

christianmagill · 2022-06-08T19:01:38Z

I see a lot of these issues are merged and closed and I'm not sure where the trail ends. Was this ever introduced into a release?

mxbclang · 2022-06-08T19:26:54Z

Hi @christianmagill! The easiest way to tell if a PR/issue was released is via the Milestone field in the right sidebar – this was included in v1.0.0.

christianmagill · 2022-06-08T19:28:27Z

Much thanks Bethany! Christian Magill

…

On Wed, Jun 8, 2022 at 3:27 PM Bethany Chobanian Lang < ***@***.***> wrote: Hi @christianmagill <https://github.com/christianmagill>! The easiest way to tell if a PR/issue was released is via the Milestone field in the right sidebar – this was included in v1.0.0. — Reply to this email directly, view it on GitHub <#147 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQI26SJ5GYQ2VSOXMPELR3VODXYXANCNFSM5NNU7EHQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

mitogh mentioned this pull request Feb 3, 2022

When uploading, generate both WebP and jpeg format images by default #142

Closed

mitogh added the [Focus] Images label Feb 3, 2022

Fix linting errors from phpcs

9c43b89

Make sure file follows the standard from `phpcs`

mitogh added this to the 1.0.0-beta.1 milestone Feb 3, 2022

Make sure the correct size is updated

25479a1

Prevent to use numeric indexes for each image size, instead use the correct image name.

mitogh mentioned this pull request Feb 4, 2022

Update the_content with the appropiate image format #152

Merged

3 tasks

felixarntz reviewed Feb 17, 2022

View reviewed changes

felixarntz added the [Type] Enhancement A suggestion for improvement of an existing feature label Feb 17, 2022

felixarntz mentioned this pull request Feb 17, 2022

Store the backup JPEG images in _wp_attachment_backup_sizes #154

Closed

3 tasks

mitogh and others added 8 commits February 17, 2022 11:52

Merge branch 'trunk' into feature/142-multiple-mimes-for-image-sizes

0f560d8

Update code to use existing WP Core function instead.

410c124

Use a function that already exists in core, no need to define a new function if core already provides this information.

Reorder functions and hooks.

7a8b0c5

Based on feedback on code review hooks where moved closer where the callbacks for each is defined.

Use a third person verb instead.

f71a897

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Fix webp typo in the extension

4b4cbc2

Merge branch 'feature/142-multiple-mimes-for-image-sizes' of github.c…

b01be99

…om:WordPress/performance into feature/142-multiple-mimes-for-image-sizes

Define the right type for the doc block.

4ba9409

Fix syntax error when calling wp_get_registered_image_subsizes

5321483

Make sure function is only called once, due `wp_get_registered_image_subsizes` does not return a function.

felixarntz mentioned this pull request Feb 18, 2022

[WIP] Generate additional mime types on upload #151

Closed

3 tasks

mitogh added 7 commits February 21, 2022 13:49

Removal of support class no longer required

2bf68f6

This class is no longer required or used in the set of tests, so there's no real value in keeping the class in place.

Add test images, the images would be used in tests.

de38c2d

The added images are going to be used as part of the tests suite.

Add required methods for the mock Editor.

dd13025

The `load` method is required by the editor to process an image.

Remove non required conditional on testing method.

70eeb33

The result of the conditional can be used instead.

Include tests for the deletion of an attachment.

975fddd

When an attachment is deleted all the generated WebP versions of an attachment should be removed when the main attachment is removed.

Update file to follow PHPCS rules.

e3c2e3a

Make sure the file follows the same format as the rest of the project by following the same guidelines.

mitogh marked this pull request as ready for review February 21, 2022 21:42

mitogh requested a review from adamsilverstein as a code owner February 21, 2022 21:42

adamsilverstein reviewed Feb 23, 2022

View reviewed changes

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

adamsilverstein reviewed Feb 23, 2022

View reviewed changes

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

felixarntz reviewed Feb 23, 2022

View reviewed changes

felixarntz mentioned this pull request Feb 23, 2022

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

Closed

mitogh and others added 6 commits February 23, 2022 15:10

Remove blank lines.

061ef31

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Add parenthesis to method references.

81ea713

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Add parenthesis to method references.

411efe4

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Update documentation on function return value.

a2c6815

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Update image generation into a single process.

a2c655c

Move the WebP image generation into a single process instead of creating individual cron for each image.

Fix linting errors for PHP

38224b0

adamsilverstein mentioned this pull request Feb 24, 2022

Update WordPress core functions to support multiple mime types #155

Open

adamsilverstein requested review from adamsilverstein and felixarntz February 24, 2022 18:23

adamsilverstein approved these changes Feb 24, 2022

View reviewed changes

mitogh mentioned this pull request Feb 24, 2022

Support retry mechanism for generating sub-sizes in additional MIME types on constrained environments. #188

Merged

3 tasks

felixarntz reviewed Feb 24, 2022

View reviewed changes

Replace array_key_exists with isset instead.

acd12f1

Use `isset` as we are just interested if the key exists even if is `null`

felixarntz approved these changes Feb 24, 2022

View reviewed changes

felixarntz merged commit 48845c7 into trunk Feb 24, 2022

eclarke1 mentioned this pull request Feb 25, 2022

Store file size (and other) media metadata #8

Closed

mitogh deleted the feature/142-multiple-mimes-for-image-sizes branch February 25, 2022 19:15

felixarntz changed the title ~~Add the sources property to each image size~~ Create image sub-sizes in additional MIME types using sources for storage. Feb 25, 2022

mitogh mentioned this pull request Mar 2, 2022

Select the smaller image size from the list of available sources #186

Closed

mxbclang mentioned this pull request Mar 4, 2022

Perform updates on secondary image mime types when original image is edited #158

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create image sub-sizes in additional MIME types using `sources` for storage. #147

Create image sub-sizes in additional MIME types using `sources` for storage. #147

mitogh commented Feb 3, 2022 •

edited

Loading

felixarntz left a comment

felixarntz commented Feb 17, 2022 •

edited

Loading

felixarntz left a comment

adamsilverstein left a comment

felixarntz left a comment

mitogh commented Feb 24, 2022

felixarntz left a comment

christianmagill commented Jun 8, 2022

mxbclang commented Jun 8, 2022

christianmagill commented Jun 8, 2022 via email

Create image sub-sizes in additional MIME types using sources for storage. #147

Create image sub-sizes in additional MIME types using sources for storage. #147

Conversation

mitogh commented Feb 3, 2022 • edited Loading

Summary

Relevant technical choices

Checklist

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz commented Feb 17, 2022 • edited Loading

felixarntz left a comment

Choose a reason for hiding this comment

adamsilverstein left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

mitogh commented Feb 24, 2022

felixarntz left a comment

Choose a reason for hiding this comment

christianmagill commented Jun 8, 2022

mxbclang commented Jun 8, 2022

christianmagill commented Jun 8, 2022 via email

Create image sub-sizes in additional MIME types using `sources` for storage. #147

Create image sub-sizes in additional MIME types using `sources` for storage. #147

mitogh commented Feb 3, 2022 •

edited

Loading

felixarntz commented Feb 17, 2022 •

edited

Loading