Back up edited full image sources when restoring the original image

[mitogh]

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.

[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.

[felixarntz]

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.

[mitogh]

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.

[mxbclang]

@mitogh Just checking in on this!

[mitogh]

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

[mitogh]

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.

[mukeshpanchal27]

PR looks good to me and is ready to marge.

[mukeshpanchal27]

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.

[felixarntz]

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