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 intotrunkfrom
Conversation
mehulkaklotar
added
[Focus] Images
[Plugin] Modern Image Formats
mehulkaklotar
requested review from
adamsilverstein,
felixarntz and
mitogh
as code owners
mitogh
left a comment
mitogh
left a comment
There was a problem hiding this comment.
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. |
| $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 ); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, @mukeshpanchal27 We are checking after deleting attachment here that post meta is not found by asserting it is empty.
There was a problem hiding this comment.
this currently is only checking that the backup meta is empty, not that the original attachment meta is deleted.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Removed the post meta assertions for the plugin tests.
adamsilverstein
left a comment
adamsilverstein
left a comment
There was a problem hiding this comment.
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
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@mehulkaklotar Mostly looks great! I left just a few minor comments.
| $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 ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, right, added both assertions for not empty and is array.
| $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 ); |
There was a problem hiding this comment.
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.
felixarntz
changed the title
…ertions removed as it is core thing
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
Thanks @mehulkaklotar, this looks great!
5 participants
Summary
Fixes #375
Relevant technical choices
Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.