Remove additional image backup sources & sizes files when attachment deleted by mehulkaklotar · Pull Request #411 · WordPress/performance · GitHub

Summary

Fixes #375

Relevant technical choices

Checklist

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

…leted attachment after edit

Changes look solid to me (great work). Question looking into these changes, are we expecting to delete the meta fields as well created by the plugin if the image is deleted such as:

_wp_attachment_backup_sources

And any other custom meta associated to an image, or those are deleted when the attachment ID is effectively deleted from within the DB?

@mitogh Meta fields are deleted when attachment ID is deleted. so we don't need to do anything to delete them.

Thanks @mehulkaklotar makes sense.

The PR looks good to me.

One question for the unit test: When the attachment is deleted using the wp_delete_attachment function, it also removes the meta associated with it, so it's worthwhile to check the post meta after it. What do you think?

Yes, @mukeshpanchal27 We are checking after deleting attachment here that post meta is not found by asserting it is empty.

this currently is only checking that the backup meta is empty, not that the original attachment meta is deleted.

we only add these two backup meta from the plugin, so we just need to check if these two meta is deleted. To check them deleted or not, we need to assert that it is empty when we get them using get_post_meta function which returns an empty string if a valid but non-existing post ID is passed.

I personally think we should remove the checks for the post meta being empty. The post metadata is being deleted by core, so what we're testing with those two assertions is core, not our own logic. I guess it doesn't hurt having them, but if we have them here, we may also add a ton more for core tests - I think that's a bit beyond the point of unit tests, they should be for our plugin code, not the core code we're integrating with.

Removed the post meta assertions for the plugin tests.

Overall looks good - left some questions. I'm going to test this locally to verify the data handling works as expected as well as verifying -scaled or -rotated image handling is correct

@mehulkaklotar Mostly looks great! I left just a few minor comments.

Why not use the same assertions for both? Shouldn't they both be non-empty arrays? Not a big deal, but this just struck me as a bit odd.

Yes, right, added both assertions for not empty and is array.

I personally think we should remove the checks for the post meta being empty. The post metadata is being deleted by core, so what we're testing with those two assertions is core, not our own logic. I guess it doesn't hurt having them, but if we have them here, we may also add a ton more for core tests - I think that's a bit beyond the point of unit tests, they should be for our plugin code, not the core code we're integrating with.

…ertions removed as it is core thing

Thanks @mehulkaklotar, this looks great!

#375 remove backup sizes files when deleted attachment

b609f0e

mehulkaklotar added [Focus] Images [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Jul 1, 2022

mehulkaklotar added this to the 1.3.0 milestone Jul 1, 2022

mehulkaklotar self-assigned this Jul 1, 2022

mehulkaklotar added the [Type] Bug An existing feature is broken label Jul 1, 2022

mehulkaklotar added 2 commits July 1, 2022 20:05

#375 remove backup sources files when deleted attachment

f51ebcf

#375 test case to prove that webp versions of files not exist when de…

fcbd062

…leted attachment after edit

mehulkaklotar marked this pull request as ready for review July 4, 2022 11:47

mehulkaklotar requested review from adamsilverstein, felixarntz and mitogh as code owners July 4, 2022 11:47

mitogh approved these changes Jul 4, 2022

View reviewed changes

mehulkaklotar added the Needs Review label Jul 5, 2022

mukeshpanchal27 reviewed Jul 6, 2022

View reviewed changes

adamsilverstein reviewed Jul 6, 2022

View reviewed changes

Comment thread modules/images/webp-uploads/load.php

adamsilverstein reviewed Jul 6, 2022

View reviewed changes

Comment thread tests/modules/images/webp-uploads/load-tests.php Outdated

adamsilverstein reviewed Jul 6, 2022

View reviewed changes

Comment thread tests/modules/images/webp-uploads/load-tests.php Outdated

adamsilverstein reviewed Jul 6, 2022

View reviewed changes

#375 test function comment fix

e708b70

felixarntz reviewed Jul 6, 2022

View reviewed changes

felixarntz changed the title Remove backup sources & sizes files when attachment deleted Remove additional image backup sources & sizes files when attachment deleted Jul 6, 2022

#375 - tests: asserts for not empty array added & attachment meta ass…

5345210

…ertions removed as it is core thing

felixarntz approved these changes Jul 7, 2022

View reviewed changes

felixarntz merged commit 044a8f2 into trunk Jul 7, 2022

felixarntz deleted the remove/backup-sources-files branch July 7, 2022 15:49

+              		$backup_sources = get_post_meta( $attachment_id, '_wp_attachment_backup_sources', true );
+              		$this->assertEmpty( $backup_sources );
+              		$backup_sizes = get_post_meta( $attachment_id, '_wp_attachment_backup_sizes', true );
+              		$this->assertEmpty( $backup_sizes );

+              		$backup_sources = get_post_meta( $attachment_id, '_wp_attachment_backup_sources', true );
+              		$this->assertNotEmpty( $backup_sources );
+              		$backup_sizes = get_post_meta( $attachment_id, '_wp_attachment_backup_sizes', true );
+              		$this->assertIsArray( $backup_sizes );

+              		$backup_sources = get_post_meta( $attachment_id, '_wp_attachment_backup_sources', true );
+              		$this->assertEmpty( $backup_sources );
+              		$backup_sizes = get_post_meta( $attachment_id, '_wp_attachment_backup_sizes', true );
+              		$this->assertEmpty( $backup_sizes );

Remove additional image backup sources & sizes files when attachment deleted by mehulkaklotar · Pull Request #411 · WordPress/performance · GitHub

Uh oh!

Remove additional image backup sources & sizes files when attachment deleted - #411

Remove additional image backup sources & sizes files when attachment deleted#411felixarntz merged 5 commits intotrunkWordPress/performance:trunkfrom remove/backup-sources-filesWordPress/performance:remove/backup-sources-filesCopy head branch name to clipboard

Conversation

mehulkaklotar commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Relevant technical choices

Checklist

Uh oh!

mitogh left a comment

Choose a reason for hiding this comment

Uh oh!

mehulkaklotar commented Jul 5, 2022

Uh oh!

mitogh commented Jul 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamsilverstein left a comment

Choose a reason for hiding this comment

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Remove additional image backup sources & sizes files when attachment deleted#411
felixarntz merged 5 commits into
trunkfrom
remove/backup-sources-files

mehulkaklotar commented Jul 1, 2022 •
edited
Loading