Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
felixarntz merged 10 commits into from
Jun 16, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ff1616a
Create set of tests to replicate the reported bug
mitogh Apr 24, 2022
ee8d1cb
Backup the sources for additional targets
mitogh Apr 24, 2022
577ea64
Remove non required conditionals
mitogh May 15, 2022
10e09b7
Merge branch 'trunk' into fix/295-backup-full-image-sources
mitogh May 15, 2022
8cfb0e4
Merge branch 'fix/295-backup-full-image-sources' of github.com:WordPr…
mitogh May 15, 2022
e1e0368
Merge branch 'trunk' into fix/295-backup-full-image-sources
mitogh May 18, 2022
d64771e
Merge branch 'trunk' into fix/295-backup-full-image-sources
mitogh Jun 5, 2022
d00cb2f
Rename variable to match core naming.
mitogh Jun 5, 2022
178cdba
Remove support for `IMAGE_EDIT_OVERWRITE` constant.
mitogh Jun 5, 2022
849c6fd
Remove duplicated code based on feedback.
mitogh Jun 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions modules/images/webp-uploads/image-edit.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ function webp_uploads_update_attachment_metadata( $data, $attachment_id ) {
return webp_uploads_backup_sources( $attachment_id, $data );
case 'wp_restore_image':
// When an image has been restored.
$data = webp_uploads_backup_sources( $attachment_id, $data );
return webp_uploads_restore_image( $attachment_id, $data );
}
}
Expand Down Expand Up @@ -387,9 +388,8 @@ function webp_uploads_get_next_full_size_key_from_backup( $attachment_id ) {
*/
function webp_uploads_restore_image( $attachment_id, $data ) {
$backup_sources = get_post_meta( $attachment_id, '_wp_attachment_backup_sources', true );

if ( ! is_array( $backup_sources ) ) {
return $data;
$backup_sources = array();
Copy link
Copy Markdown
Member

@mukeshpanchal27 mukeshpanchal27 Jun 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@felixarntz felixarntz Jun 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

if ( ! isset( $backup_sources['full-orig'] ) || ! is_array( $backup_sources['full-orig'] ) ) {
Expand Down
70 changes: 68 additions & 2 deletions tests/modules/images/webp-uploads/image-edit-tests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,13 @@ public function it_should_restore_the_sources_array_from_the_backup_when_an_imag
$this->assertImageHasSource( $attachment_id, 'image/jpeg' );
$this->assertImageHasSource( $attachment_id, 'image/webp' );

$metadata = wp_get_attachment_metadata( $attachment_id );
$metadata = wp_get_attachment_metadata( $attachment_id );
$updated_backup_sources = get_post_meta( $attachment_id, '_wp_attachment_backup_sources', true );

$this->assertSame( $backup_sources['full-orig'], $metadata['sources'] );
$this->assertSame( $backup_sources, get_post_meta( $attachment_id, '_wp_attachment_backup_sources', true ) );
$this->assertNotSame( $backup_sources, $updated_backup_sources );
$this->assertCount( 1, $backup_sources );
$this->assertCount( 2, $updated_backup_sources );

$backup_sizes = get_post_meta( $attachment_id, '_wp_attachment_backup_sizes', true );
foreach ( $backup_sizes as $size_name => $properties ) {
Expand Down Expand Up @@ -428,4 +431,67 @@ public function it_should_prevent_to_store_an_empty_set_of_sources() {
$this->assertTrue( $editor->success() );
$this->assertEmpty( get_post_meta( $attachment_id, '_wp_attachment_backup_sources', true ) );
}

/**
* Store the next image hash on the backup sources
*
* @test
*/
public function it_should_store_the_next_image_hash_on_the_backup_sources() {
$attachment_id = $this->factory->attachment->create_upload_object( TESTS_PLUGIN_DIR . '/tests/testdata/modules/images/leafs.jpg' );
$editor = new WP_Image_Edit( $attachment_id );
// Edit the image.
$editor->rotate_right()->save();
// Restore the image.
wp_restore_image( $attachment_id );

$backup_sources = get_post_meta( $attachment_id, '_wp_attachment_backup_sources', true );

$this->assertIsArray( $backup_sources );
$this->assertCount( 2, $backup_sources );
foreach ( array_keys( $backup_sources ) as $name ) {
if ( 'full-orig' === $name ) {
$this->assertSame( 'full-orig', $name );
} else {
$this->assertSizeNameIsHashed( '', $name );
}
}
}

/**
* Create backup of full size images with the same hash keys as the edited images
*
* @test
*/
public function it_should_create_backup_of_full_size_images_with_the_same_hash_keys_as_the_edited_images() {
$attachment_id = $this->factory->attachment->create_upload_object( TESTS_PLUGIN_DIR . '/tests/testdata/modules/images/leafs.jpg' );

$editor = new WP_Image_Edit( $attachment_id );
$editor->rotate_right()->save();
$this->assertTrue( $editor->success() );
$editor->rotate_right()->save();
$this->assertTrue( $editor->success() );
$editor->flip_right()->save();
$this->assertTrue( $editor->success() );

$backup_sizes = get_post_meta( $attachment_id, '_wp_attachment_backup_sizes', true );
$backup_sources = get_post_meta( $attachment_id, '_wp_attachment_backup_sources', true );

$this->assertIsArray( $backup_sources );
$this->assertIsArray( $backup_sizes );
$this->assertCount( 3, $backup_sources );

foreach ( $backup_sources as $size_name => $sources ) {
if ( 'full-orig' === $size_name ) {
// Skip this size name due this name does not have a hash name.
continue;
}

// Ensure the size name is an actual hashed value.
$this->assertSizeNameIsHashed( 'full', $size_name );
// Ensure the full-{hash} name is the same created on the backup sizes or that both hash names are the same.
$this->assertArrayHasKey( $size_name, $backup_sizes );
$this->assertIsArray( $backup_sizes[ $size_name ] );
}
}
}