Restore and backup image sizes alongside the sources properties #242
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
added
[Focus] Images
[Plugin] Modern Image Formats
mitogh
added
the
[Type] Enhancement
felixarntz
reviewed
Mar 22, 2022
There was a problem hiding this comment.
@mitogh Left a few comments. High-level summary:
- We can simplify this implementation by introducing our own
_wp_attachment_backup_sourcesmeta key. - This meta key will be used to store the
sourcesonly for the original image (since WP core otherwise wouldn't store it). - The other
sourcesare 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
sourcesadded (that'll be part of Perform updates on secondary image mime types when original image is edited #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. |
felixarntz
reviewed
Mar 23, 2022
There was a problem hiding this comment.
@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>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
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.
felixarntz
reviewed
Mar 28, 2022
There was a problem hiding this comment.
@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>
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.
felixarntz
changed the base branch from
release/1.0.0-beta.4
to
release/1.0.0-rc.1
felixarntz
approved these changes
Mar 29, 2022
There was a problem hiding this comment.
@mitogh Awesome work!
cc @adamsilverstein for incorporating this into the core patch (in there we can do it without that hook weirdness present here)
eugene-manuilov
requested changes
Mar 30, 2022
There was a problem hiding this comment.
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>
Co-authored-by: Eugene Manuilov <manuilov@google.com>
Co-authored-by: Eugene Manuilov <manuilov@google.com>
Co-authored-by: Eugene Manuilov <manuilov@google.com>
Make sure the code performns in a more structured way
eugene-manuilov
approved these changes
Mar 31, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #228
Relevant technical choices
Usage of
_sourcesmetadata keyA
_sourcesproperty is stored to hold the top-levelsourcesfor the full image size when an image is edited, after the backup is updated the_sourcesproperty would be moved into the appropriatefullsize inside of the backup metadata and the temporarily_sourcesproperty 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
sourcesproperty is populated again from the backup metadata if thefull-orighas thesourcesproperty.Removal of edited images
When an image is edited and restored any edited version of the images are removed if
IMAGE_EDIT_OVERWRITEis defined as atruthyvalue.Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.