Store the backup JPEG images in `_wp_attachment_backup_sizes` #154

mitogh · 2022-02-04T23:39:08Z

Summary

Creates the JPEG version of the WebP images and store the references into _wp_attachment_backup_sizes similar to image transformations in order to take advantage of benefits from WP Core like files removed automatically by WordPress and uploaded into CDNs

Fixes:

Relevant technical choices

The changes introduced in this PR replicate the same behavior from WordPress core to store edited versions of additional mime types and the additional mime types themselves in the meta _wp_attachment_backup_sizes.

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.

In this case the `_wp_attached_file` needs to be used in order to gain any update to the image, instead of using the original image for any subsize.

Create an store the backup images for all the subsizes and supported image sizes.

Make sure the file is formatted properly.

modules/images/webp-uploads/load.php

adamsilverstein · 2022-02-09T18:25:53Z

modules/images/webp-uploads/load.php

+			}
+
+			// TODO: Store the file size of the created image.
+			// $image['filesize'] = filesize( $image['path'] );


I'm in favor of storing this for all sizes, this is useful for performance analysis and potentially other things.

Great. for now would keep the TODO, and we can came back to this once we start working on:

Store file size (and other) media metadata #8

modules/images/webp-uploads/load.php

adamsilverstein · 2022-02-09T18:31:15Z

modules/images/webp-uploads/load.php

+	$backup_sizes = get_post_meta( $attachment_id, '_wp_attachment_backup_sizes', true );
+	$backup_sizes = is_array( $backup_sizes ) ? $backup_sizes : array();
+
+	foreach ( webp_uploads_get_image_sizes() as $size => $properties ) {


instead of trying to recreate sizes, can we just copy the meta 'sizes'?

We can certainly do that but aside from that, we need to know if the newly created image should be cropped or not and that's the main reason why we need this function.

I think something we can do at the top of the foreach loop to make sure this $size exists in the original metadata before creating a targeted image instead.

Actually, this is already the case on this line 😃

performance/modules/images/webp-uploads/load.php

Line 81 in 84eb3ec

if ( empty( $metadata['sizes'][ $size ] ) || ! is_array( $metadata['sizes'][ $size ] ) ) {

One thing I noticed in testing when I uploading a very large image, I got a jpeg created with a -scaled suffix, then all of the generated .webp sub size files also included the -scaled suffix:

In addition, I noticed there was no full sized .webp created (that is {filename}-scaled.webp) - we need that full sized file for the image markup (only applies when you upload images larger than big_image_size_threshold)

Thanks for testing and for the feedback. Some follow-up comments.

One thing I noticed in testing when I uploading a very large image, I got a jpeg created with a -scaled suffix, then all of the generated .webp sub size files also included the -scaled suffix.

Great point mostly because this is created out of the scaled version. But let me fix this one :)

In addition, I noticed there was no full sized .webp created (that is {filename}-scaled.webp) - we need that full sized file for the image markup (only applies when you upload images larger than big_image_size_threshold)

Actually, this follows the same logic applied here #122 and #143 this behavior would go against this solved issue, as this scaled version is the full size image. Based on the issue scaled and full size always remains in the original image format. (not converted)

Let me know if you think we should change it instead.

Co-authored-by: Adam Silverstein <[email protected]>

Update the logic for hwo the additional mime types were created in order to allow image edits to take effect, when an image is cropped, flipped or rotated, those transformations are reflected into the additional mime types.

modules/images/webp-uploads/load.php

This would allow to quickly locate `webp` or a defined extension images and split between `-orig` images suffix as well.

Prevent namespace collisions when using the function from within the plugin.

The callback was passed as a parameter, no change is placed on the callback so we can safely return it.

This was due in some cases when restoring an image and editing it again, the image would result in not executing correctly due the meta was the same as before and the update callback was not executed, instead we are moving this work into an ajax callback.

…with-backup

adamsilverstein · 2022-02-15T21:12:01Z

modules/images/webp-uploads/load.php

+	}
+
+	// All subsizes are created out of the `file` property and not the original image.
+	$file = get_attached_file( $attachment_id, true );


Why do you use get_attached_file here instead of the original image, is this to use the -scaled image?

Correct any edits to an image are performed to the scaled version (if present) by WordPress so any subsize should be based out of the modified image and not the original image instead.

The original image is only used to create the scaled version if not required it would be used as the attached file instead.

adamsilverstein · 2022-02-15T21:14:56Z

modules/images/webp-uploads/load.php

+			// The file already exists as part of the backup sizes in the same key.
+			if ( array_key_exists( $key, $backup_sizes ) ) {
+				// This case would remove the file if is an edited version.
+				if ( defined( 'IMAGE_EDIT_OVERWRITE' ) && IMAGE_EDIT_OVERWRITE ) {


modules/images/webp-uploads/load.php

adamsilverstein · 2022-02-15T21:21:01Z

modules/images/webp-uploads/load.php

+				unset( $backup_sizes[ $key ] );
+			}
+
+			// Editor needs to be recreated every time as there is not flush() or clear() function that can be used after we created an image.


Did you try$editor->__destruct (or even just $editor = null which should trigger __destruct)? For both Imagick and GD this should clear the image.

No, let me try that thanks for the suggestion.

Actually calling __destruct seems like the same as creating a new variable altogether as essentially this is called by the engine itself so, for now, will keep it like that.

One thing that I've noted out of the class WP_Image_Editor is the fact that has a property get_size but not a set_size which would be the one we can use here to reset the size to the appropriate dimensions. Any thoughts on that one ?

modules/images/webp-uploads/load.php

adamsilverstein · 2022-02-15T21:40:50Z

modules/images/webp-uploads/load.php

- * @since 1.0.0
 */

+add_filter( 'wp_generate_attachment_metadata', 'webp_uploads_create_images_with_additional_mime_types', 10, 3 );


Generating the additional mime types immediately might fail because it will double execution time, right? I can test this locally, but I think this might break uploads for some users.

Can we instead add an immediately one-time executing cron callback (wp_schedule_single_event)? that way the images will get generated on the next callback as part of a separate request?

Agree I'm going to explore this as part of enchantment on top of this PR as you describe has become quite large at this point.

Actually I just updated this PR to handle this case.

adamsilverstein · 2022-02-15T21:48:16Z

modules/images/webp-uploads/load.php

+	);
+
+	/**
+	 * An array representing all the valid mime types where multiple images would be created if


We should make sure that by returning only webp in the array only webp images are generated - that is, the uploads would work the way they do now in the plugin.

Although we decided this shouldn't be the default approach, it may turn out to be the best approach for most users, and over time it may become our choice for default. Code wise, we should be able to keep the existing plugin approach, but only use it in the case when a single mime type is specified.

Great point.

Let me think about how I can go about this use case when only a single mime type is present either JPEG or WebP so I can update this PR accordingly. Another use case we need to consider is the fact that due this is filterable it can be empty as well. Under that scenario we can either:

Generate all subsizes with the same mime-type as the original image regardless if this filter is empty

Not generate any subsize to follow what was specified on the filter.

It feels like the last point would be the way to go for consistency but let me know your thoughts on that one.

Fixed this one and added a TODO to handle this last point.

adamsilverstein · 2022-02-15T21:54:28Z

@mitogh this looks good so far, I left some small comments and questions.

This PR has grown quite large because in addition to handling image generation you are supporting image edit and restore. I realize these are related because of the data storage, but still wonder if we could break them out so we can focus solely on the image generation and meta data storage. Once we land that, we can tackle the edit/restore bits in a separate PR.

THe hash extraction ahs been moved into a custom function to avoid confusion and bring clarity when used.

Remove the suffix: - scaled - rotated - imagifyresized From the file names if present.

The logic to handle the case for image edits like: rotation, flip and crop would be handled in a separate PR. This PR addresses the concern for #142 and #150.

The result of the condition is always a boolean, so this section can be simplified with a single line instead.

Additional images would be created during a cron execution, removing any workload from the initial request. Tests where fixed and some new ones where introduced.

felixarntz · 2022-02-17T01:22:14Z

@mitogh @adamsilverstein I think the approach in #147 makes more sense than this one. _wp_attachment_backup_sizes is used for edited/cropped versions of an image, while the sizes field of the regular image metadata is what is being used for the registered sizes. Since we are focusing on generating the sub-sizes in progressive image formats, I think our data should be in the sizes array.

Regarding backward-compatibility, I don't think the approach here justifies going with it given the unexpected data storage, specifically for the following reasons:

It shouldn't be too hard deleting the "custom" sub-sizes generated in the approach from Create image sub-sizes in additional MIME types using sources for storage. #147, we just have to use the delete_attachment action to delete any of those image files that exist.
Regarding CDNs, that indeed is a valid concern. However, we also don't break anything in these plugins or WordPress core with the sources implementation in Create image sub-sizes in additional MIME types using sources for storage. #147, and the eventual dev note post about this feature in WordPress core should include guidance on how to update such type of plugins to support the new sources field. It's okay to introduce a new API for plugins to use, and since we're adding a new feature to WordPress core, it's reasonable to expect plugin developers that want to support the feature to adopt the new "API" (which here really is just newly stored data).

mitogh · 2022-02-17T17:38:08Z

Closing this one in favor of:

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

Details #147 (comment)

mitogh added 6 commits February 2, 2022 19:56

Fix linting errors from phpcs

9c43b89

Make sure file follows the standard from `phpcs`

Make sure the correct size is updated

25479a1

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

Make sure the scaled version is used.

d7bf62c

In this case the `_wp_attached_file` needs to be used in order to gain any update to the image, instead of using the original image for any subsize.

Generate all the backup images

d6f90d4

Create an store the backup images for all the subsizes and supported image sizes.

Fix phocs standard guidelines

84eb3ec

Make sure the file is formatted properly.

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

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

mitogh self-assigned this Feb 4, 2022

mitogh requested review from adamsilverstein and getsource as code owners February 4, 2022 23:39

mitogh added the [Type] Feature A new feature within an existing module label Feb 4, 2022

mitogh mentioned this pull request Feb 4, 2022

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

Closed

mitogh marked this pull request as draft February 4, 2022 23:41

felixarntz mentioned this pull request Feb 8, 2022

Progressive transition into WebP formats #150

Closed

adamsilverstein reviewed Feb 9, 2022

View reviewed changes

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

adamsilverstein reviewed Feb 9, 2022

View reviewed changes

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

adamsilverstein reviewed Feb 9, 2022

View reviewed changes

mitogh and others added 2 commits February 9, 2022 15:35

Use dash for file name modification.

dbded24

Co-authored-by: Adam Silverstein <[email protected]>

Create additional mime types when an image is edited.

02c52e1

Update the logic for hwo the additional mime types were created in order to allow image edits to take effect, when an image is cropped, flipped or rotated, those transformations are reflected into the additional mime types.

mitogh mentioned this pull request Feb 12, 2022

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

Closed

adamsilverstein mentioned this pull request Feb 14, 2022

Update WordPress core functions to support multiple mime types #155

Open

eugene-manuilov reviewed Feb 14, 2022

View reviewed changes

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

mitogh added 4 commits February 14, 2022 16:50

Update key structure to be extension-type

caea66b

This would allow to quickly locate `webp` or a defined extension images and split between `-orig` images suffix as well.

Rename function to avoid namespace collisions.

4c0a1ab

Prevent namespace collisions when using the function from within the plugin.

Return the provided callback if present

5f1beb7

The callback was passed as a parameter, no change is placed on the callback so we can safely return it.

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

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

1567202

…with-backup

eclarke1 added the Needs Review label Feb 15, 2022

adamsilverstein reviewed Feb 15, 2022

View reviewed changes

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

adamsilverstein reviewed Feb 15, 2022

View reviewed changes

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

adamsilverstein reviewed Feb 15, 2022

View reviewed changes

mitogh added 8 commits February 15, 2022 18:50

Remove non required character.

ca2c3e3

Move hash parsing into a custom function.

436659a

THe hash extraction ahs been moved into a custom function to avoid confusion and bring clarity when used.

Remove suffix from file names.

d513f20

Remove the suffix: - scaled - rotated - imagifyresized From the file names if present.

Split the edit logic into a separate Pull Request

e421806

The logic to handle the case for image edits like: rotation, flip and crop would be handled in a separate PR. This PR addresses the concern for #142 and #150.

Format function declaration and parameters

34142b6

Simplify mock classes with an inline return instead.

eb81912

The result of the condition is always a boolean, so this section can be simplified with a single line instead.

Move the generation of additional images into a cron.

8158319

Additional images would be created during a cron execution, removing any workload from the initial request. Tests where fixed and some new ones where introduced.

Fix linter errors on test file

cebae7e

felixarntz mentioned this pull request Feb 17, 2022

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

Merged

3 tasks

mitogh closed this Feb 17, 2022

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

felixarntz added the no milestone PRs that do not have a defined milestone for release label Mar 7, 2022

felixarntz removed this from the 1.0.0-beta.1 milestone Mar 7, 2022

mitogh mentioned this pull request Mar 7, 2022

Prepare the 1.0.0-beta.1 release #208

Merged

3 tasks

Store the backup JPEG images in _wp_attachment_backup_sizes #154

Store the backup JPEG images in _wp_attachment_backup_sizes #154

Uh oh!

Conversation

mitogh commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Relevant technical choices

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitogh Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsilverstein commented Feb 15, 2022

Uh oh!

felixarntz commented Feb 17, 2022

Uh oh!

mitogh commented Feb 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Store the backup JPEG images in `_wp_attachment_backup_sizes` #154

Store the backup JPEG images in `_wp_attachment_backup_sizes` #154

mitogh commented Feb 4, 2022 •

edited

Loading

mitogh Feb 9, 2022 •

edited

Loading