Restore and backup image sizes alongside the sources properties by mitogh · Pull Request #242 · WordPress/performance · GitHub

Summary

Fixes #228

Relevant technical choices

Usage of _sources metadata key

A _sources property is stored to hold the top-level sources for the full image size when an image is edited, after the backup is updated the _sources property would be moved into the appropriate full size inside of the backup metadata and the temporarily _sources property would be removed from the attachment image.

This key is used so when the metadata is updated we still have a reference to the previous sources values to the full size image when the image is edited. This is not the case for the rest of real sizes, this is only the case for full sizes.

When the image is restored the sources property is populated again from the backup metadata if the full-orig has the sources property.

Removal of edited images

When an image is edited and restored any edited version of the images are removed if IMAGE_EDIT_OVERWRITE is defined as a truthy value.

Checklist

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

The class is in charge of performing operations to an image as if a user is performing those actions from the image editor but from the BE. This class is a utility class to be used from within the test context to mimic the user behavior of performing edits into an image.

When an image is edited the `sources` properties is going to be backup with the rest of the sizes properties, however that's not the case for the `full` size which is a virtual type and the top level `sources` would be moved into the backup metadata. When the image is restored any edited image available in the sources array would be removed before the restore takes place. The `sources` property for the `full-orig` would be placed at the top level `sources` with the rest of the metadata.

@mitogh Left a few comments. High-level summary:

We can simplify this implementation by introducing our own _wp_attachment_backup_sources meta key.
This meta key will be used to store the sources only for the original image (since WP core otherwise wouldn't store it).
The other sources are automatically backed up by WP core, since it just copies everything over for the sub-sizes. That actually already works today, which is nice to see.
When restoring, we then only need to include logic to restore the original image sources.
We don't need to delete any image files because edited images will not have any sources added (that'll be part of #158). In other words, there will not be anything to delete other than what WP core already handles.

This would be handled once #158 is merged so the current approach can be adjusted accordingly.

When an image is edited using a target other than the all and the thumbnail make sure the `success` method returns the correct value.

Due to the only value that is not correctly stored is the top level sources attributes this now is stored in a separate meta value. Stored in `_wp_attachment_backup_sources` Logic was added to handle scenarios where only the thumbnail or all images except the thumbnail are modified.

Thanks for this review @felixarntz just updated this PR based on your feedback, let me know in case you have any additional comments or feedback.

@mitogh Left a few follow-up comments, I still have some open questions, and a few small nit-picks.

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

Move the hooks for the update right into the place where they are actually used.

@mitogh This looks great now from a functionality perspective! I left a few comments on cleaning up a few things, we can simplify some code and reuse the function instead of rewriting it twice.

The main point of feedback here is that we also should do an extra security check to ensure we're updating the correct attachment.

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

The `backup_sizes` parameter was not used so it can be safely removed from the code execution.

Reuse the hook as now is stored in a variable, so it can be reused in both hooks.

@mitogh Awesome work!

cc @adamsilverstein for incorporating this into the core patch (in there we can do it without that hook weirdness present here)

Nice work, @mitogh! Added some comments with minor improvements. Please, let me know whether it makes sense to you.

Co-authored-by: Eugene Manuilov <manuilov@google.com>

Make sure the code performns in a more structured way

LGTM! Thanks, @mitogh.

mitogh added 2 commits March 19, 2022 12:39

mitogh requested review from adamsilverstein and getsource as code owners March 19, 2022 18:53

mitogh self-assigned this Mar 19, 2022

mitogh added [Focus] Images [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Mar 19, 2022

mitogh added this to the 1.0.0-beta.3 milestone Mar 19, 2022

mitogh added the [Type] Enhancement A suggestion for improvement of an existing feature label Mar 19, 2022

mitogh mentioned this pull request Mar 19, 2022

Restore and backup Image sizes with sources when an image is edited #228

Closed

felixarntz reviewed Mar 22, 2022

View reviewed changes

mitogh added 4 commits March 22, 2022 16:09

Merge branch 'trunk' into feature/228-restore-and-backup-image-sources

ab7bf05

Remove handling of edited images

7e82041

This would be handled once #158 is merged so the current approach can be adjusted accordingly.

Update the success logic to handle additional cases.

a2b82e3

When an image is edited using a target other than the all and the thumbnail make sure the `success` method returns the correct value.

mitogh requested a review from felixarntz March 23, 2022 20:38

felixarntz reviewed Mar 23, 2022

View reviewed changes

mitogh and others added 8 commits March 23, 2022 15:11

Remove empty space

2dd29d8

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

Update prefix on functions

b16cee1

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

Update prefix on functions

8a8e967

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

Update prefix on functions

322dd3d

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

Update hooks reference

226a5aa

Backup the data from previous metadata

8a809ed

Merge branch 'trunk' into feature/228-restore-and-backup-image-sources

6d134e5

Merge branch 'trunk' into feature/228-restore-and-backup-image-sources

76307e6

mitogh changed the base branch from trunk to release/1.0.0-beta.4 March 24, 2022 23:54

Remove the need of auxilary variable to hold the sources

951c4ca

Move the hooks for the update right into the place where they are actually used.

felixarntz reviewed Mar 28, 2022

View reviewed changes

Add since doc block

61e25e8

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

mitogh and others added 5 commits March 28, 2022 16:35

Align parameters on the doc block

4c26f4f

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

Removal of non required parameter

973a7b8

The `backup_sizes` parameter was not used so it can be safely removed from the code execution.

Align parameters to follow WordPress guidelines

cb35f42

Include the @since tags on missing functions

7880f6b

Move logic of the hook into a variable

ca8d28e

Reuse the hook as now is stored in a variable, so it can be reused in both hooks.

mitogh requested a review from felixarntz March 28, 2022 22:51

felixarntz changed the base branch from release/1.0.0-beta.4 to release/1.0.0-rc.1 March 29, 2022 17:59

felixarntz approved these changes Mar 29, 2022

View reviewed changes

eugene-manuilov requested changes Mar 30, 2022

View reviewed changes

mitogh and others added 7 commits March 30, 2022 10:49

Add white spaces on tests

c381398

Co-authored-by: Eugene Manuilov <manuilov@google.com>

Add white spaces on tests

93e0d01

Co-authored-by: Eugene Manuilov <manuilov@google.com>

Removal of sanitize_text_field due this value is not required.

c165a11

Co-authored-by: Eugene Manuilov <manuilov@google.com>

Run logic only if was executed when doing ajax.

7d96583

Co-authored-by: Eugene Manuilov <manuilov@google.com>

Update code flow for more performant checks

7a113d3

Make sure the code performns in a more structured way

Split conditionals into separate statements

e202548

Removal of non requried conditional

188c7df

mitogh requested a review from eugene-manuilov March 30, 2022 19:44

eugene-manuilov approved these changes Mar 31, 2022

View reviewed changes

felixarntz merged commit ad0a330 into release/1.0.0-rc.1 Mar 31, 2022

felixarntz deleted the feature/228-restore-and-backup-image-sources branch March 31, 2022 15:46

Restore and backup image sizes alongside the sources properties by mitogh · Pull Request #242 · WordPress/performance · GitHub

Uh oh!

Restore and backup image sizes alongside the sources properties - #242

Conversation

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

Uh oh!

Summary

Relevant technical choices

Checklist

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mitogh commented Mar 23, 2022

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

eugene-manuilov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eugene-manuilov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mitogh commented Mar 19, 2022 •
edited
Loading