Back up edited `full` image sources when restoring the original image #314

mitogh · 2022-04-24T02:34:40Z

Summary

Store the sources and update the _wp_attachment_backup_sources meta when the image is restored, only update the meta if the meta location has not been updated yet.

Fixes #295

Relevant technical choices

A new class was introduced when the constant IMAGE_EDIT_OVERWITE is set to true due this needs to happen before WordPress has been bootrstaped, this class can be used as time move forwards for any additional scenario when this constant is defined as true and allowing the rest of the other tests to behave in a scenario when the constant is not defined. @runInIsolation can't be used as part of the tests due we are using clousures and the clousures can't be serialized when running a test in isolaion.

The backup tries to find the edited image when the image is restored in order to store it in the same way as when the image is edited.

Checklist

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

When an image is edited and restored, the full size sources are not stored in the meta `_wp_attachment_backup_sources` with this change we introduce the storage of those values when the constant IMAGE_EDIT_OVERWRITE is not defined or it contains a falsy value. In order to follow the same patter WordPress uses to store the backup sizes.

When restoring an image, make sure any edited source for the full size image is stored in the meta used to backup the sources, in the same way it happens for the sizes when the constant `IMAGE_EDIT_OVERWRITE` is defined.

felixarntz

@mitogh This is a bit of a detail, but the implementation here has still some disparities for how WordPress core works. Try editing and restoring an image and compare the data in _wp_attachment_backup_sizes with the data in _wp_attachment_backup_sources.

Note how the hash used on the array keys in "sizes" is different from the hash used on the files themselves. However, in our own "sources" the hash on the entries is the same as the hash used on the files. So while that may feel correct, it is not the same as core's behavior.

So the custom implementation here is a bit off still. After a closer inspection, I think we can actually simplify this a lot and fix it at the same time. Basically, we need to call our existing webp_uploads_backup_sources() function even when restoring an image. I've tested this locally, and with that change the result is as expected, with the hashes in webp_uploads_backup_sizes and webp_uploads_backup_sources matching correctly.

So this can be simplified quite a bit:

The changes here in webp_uploads_restore_image() are not necessary.
Instead, we need to add $data = webp_uploads_backup_sources( $attachment_id, $data ); to the case 'wp_restore_image' condition in webp_uploads_update_attachment_metadata(), right before calling webp_uploads_restore_image( $attachment_id, $data ).
The tests here need to be updated accordingly. We need to validate the hashes in webp_uploads_backup_sizes and webp_uploads_backup_sources match.

modules/images/webp-uploads/image-edit.php

felixarntz · 2022-05-02T22:49:21Z

modules/images/webp-uploads/image-edit.php

+	if ( empty( $data['sources'] ) ) {
+		return $data;
+	}


How can we expect sources to be set here? This hook is called from core's wp_update_attachment_metadata() call, and core doesn't set sources within the metadata. I think we need to look at the previous attachment metadata here via wp_get_attachment_metadata( $attachment_id ), similar to how we do it already in our webp_uploads_backup_sources() implementation.

Good question, the main point of this function is not to set up sources if sources is not present we can assume the image does not have any available sources. Since this function is being called when the restore image is executed. At that point, we are not creating sources but restoring them so is safe to assume here if a sources is not present there's nothing to be restored (at least nothing custom created by this plugin) since this plugin is the one creating sources property.

modules/images/webp-uploads/image-edit.php

Just use the result from `preg_match` as the short circuit step to move on. Co-authored-by: Eugene Manuilov <[email protected]>

…ess/performance into fix/295-backup-full-image-sources

mxbclang · 2022-06-03T10:52:08Z

@mitogh Just checking in on this!

mitogh · 2022-06-03T17:34:24Z

Thanks for the ping @bethanylang will wrap the work on this one over the weekend so is ready for a review on Monday :)

Rename `$target` variable to `$suffix` in order to match the same naming used in core.

Don't add support for the constant that exists in core and has not been fully supported here, all changes and behavior for `IMAGE_EDIT_OVERWRITE` would be addressed later.

Update the code by removing duplicated sections of code and reuising existing sections that helps to do the same logic. Add new test case to review that the hashes matches existing values from sizes. Removal of non required tests due the constant is not currently supported.

mitogh · 2022-06-06T03:25:54Z

Just a heads up @bethanylang that this PR is ready for another review due the feedback from @felixarntz has been addressed.

Let me know in case you need additional feedback about this one, or if there's anything else I can help with on this one.

Thanks.

felixarntz

Thanks @mitogh, looks great! One follow-up question for you, would be great if you could have a quick look so that we can merge this in time for 1.2.0.

modules/images/webp-uploads/image-edit.php

mukeshpanchal27

PR looks good to me and is ready to marge.

mukeshpanchal27 · 2022-06-16T07:23:26Z

modules/images/webp-uploads/image-edit.php

-
 	if ( ! is_array( $backup_sources ) ) {
-		return $data;
+		$backup_sources = array();


I have the same question as Felix and would like to explore that edge case.

I tried this edge case in which the _wp_attachment_backup_sources meta value is null or not an array. As per the below code snippet, it will initialize an empty array for $backup_sources.

if ( ! is_array( $backup_sources ) ) { $backup_sources = array(); }

After this snippet, we have another snippet that checks two things if full-orig key is not set in array $backup_sources or it's not an array.

if ( ! isset( $backup_sources['full-orig'] ) || ! is_array( $backup_sources['full-orig'] ) ) { return $data; }

In this edge case the function return $data so as I understood it is worth initializing an empty array instead of returning the $data as pervious it does.

Please let me know if I'm getting anything wrong.

From what I can tell it doesn't really matter though which way to go about this line of code, for the following reason:

If $backup_sources is not an array, the following condition ! isset( $backup_sources['full-orig'] ) || ! is_array( $backup_sources['full-orig'] ) will also fore sure evaluate to false.

So we can either initialize $backup_sources as empty array, causing $data to be returned in the following clause, or we can return $data right away. I don't think it changes the function behavior at all honestly.

cc @mitogh

mitogh added 2 commits April 23, 2022 21:25

Backup the sources for additional targets

ee8d1cb

When restoring an image, make sure any edited source for the full size image is stored in the meta used to backup the sources, in the same way it happens for the sizes when the constant `IMAGE_EDIT_OVERWRITE` is defined.

mitogh added [Type] Bug An existing feature is broken [Focus] Images [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Apr 24, 2022

mitogh added this to the First release after 1.0.0 milestone Apr 24, 2022

mitogh self-assigned this Apr 24, 2022

mitogh requested review from adamsilverstein and getsource as code owners April 24, 2022 02:34

felixarntz requested changes May 2, 2022

View reviewed changes

felixarntz changed the title ~~Backup any additional full size image sources in _wp_attachment_backup_sources~~ Back up edited full image sources when restoring the original image May 2, 2022

mxbclang added the Needs Review label May 5, 2022

felixarntz modified the milestones: 1.1.0, 1.2.0 May 9, 2022

eugene-manuilov reviewed May 14, 2022

View reviewed changes

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

mitogh and others added 4 commits May 15, 2022 09:03

Remove non required conditionals

577ea64

Just use the result from `preg_match` as the short circuit step to move on. Co-authored-by: Eugene Manuilov <[email protected]>

Merge branch 'trunk' into fix/295-backup-full-image-sources

10e09b7

Merge branch 'fix/295-backup-full-image-sources' of github.com:WordPr…

8cfb0e4

…ess/performance into fix/295-backup-full-image-sources

Merge branch 'trunk' into fix/295-backup-full-image-sources

e1e0368

mitogh added 4 commits June 5, 2022 17:06

Merge branch 'trunk' into fix/295-backup-full-image-sources

d64771e

Rename variable to match core naming.

d00cb2f

Rename `$target` variable to `$suffix` in order to match the same naming used in core.

Remove support for IMAGE_EDIT_OVERWRITE constant.

178cdba

Don't add support for the constant that exists in core and has not been fully supported here, all changes and behavior for `IMAGE_EDIT_OVERWRITE` would be addressed later.

mitogh requested review from eugene-manuilov and felixarntz June 6, 2022 03:24

felixarntz approved these changes Jun 14, 2022

View reviewed changes

modules/images/webp-uploads/image-edit.php Show resolved Hide resolved

mukeshpanchal27 approved these changes Jun 16, 2022

View reviewed changes

felixarntz merged commit f4d2e3d into trunk Jun 16, 2022

felixarntz deleted the fix/295-backup-full-image-sources branch June 16, 2022 15:04

Back up edited full image sources when restoring the original image #314

Back up edited full image sources when restoring the original image #314

Uh oh!

Conversation

mitogh commented Apr 24, 2022

Summary

Relevant technical choices

Checklist

Uh oh!

felixarntz left a comment • 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!

Uh oh!

felixarntz May 2, 2022

Choose a reason for hiding this comment

Uh oh!

mitogh May 18, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mxbclang commented Jun 3, 2022

Uh oh!

mitogh commented Jun 3, 2022

Uh oh!

mitogh commented Jun 6, 2022

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

mukeshpanchal27 Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

felixarntz Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Back up edited `full` image sources when restoring the original image #314

Back up edited `full` image sources when restoring the original image #314

felixarntz left a comment •

edited

Loading