wp_delete_attachment returning successfully, deleting all DB data, but NOT deleting files, and NOT returning false

[Jossnaz]

from
​https://wordpress.stackexchange.com/questions/344976/wp-delete-attachment-returning-successfully-deleting-all-db-data-but-not-delet?noredirect=1#comment505976_344976

I digged into wp_delete_attachment here https://core.trac.wordpress.org/browser/tags/5.2.1/src/wp-includes/post.php#L5450 , it calls wp_delete_attachment_files

wp_delete_attachment_files returns false on failure, but this is ignored! in wp_delete_attachment. Now I'm not gonna go on a rant how bad that 'design' is. My question is, how can I make sure that the files DO get deleted?

I'm calling

 $attachments = get_attached_media('', $post->ID);
 foreach ($attachments as $attachment) {
    wp_delete_attachment($attachment->ID, true);
wp_delete_attachment never returns falsy.

How can I figure out and fix wp_delete_attachment ?

in my case it seems that some post_meta might be damaged, as the file location sometimes can be lost for some reason. This should return false or better throw and error

[donmhico]

Hello @Jossnaz,

I cannot reproduce the bug in my end. But if you want to be able to see if the files are indeed deleted. You may try this.

<?php
$attachments = get_attached_media( '', $post->ID );

foreach( $attachments as $attachment ) {
    $meta = wp_get_attachment_metadata( $attachment->ID );
    $backup_sizes = get_post_meta( $attachment->ID, '_wp_attachment_backup_sizes', true );
    $file = get_attached_file( $attachment->ID );

    // Here you'll get either true or false.
    $delete_files = wp_delete_attachment_files( $attachment->ID, $meta, $backup_sizes, $file );

    // Still need to call this to delete the `attachment post`.
    wp_delete_attachment( $attachment->ID, true );
}

Reference:
​https://developer.wordpress.org/reference/functions/wp_delete_attachment/

[Jossnaz]

Replying to donmhico:

maybe I should clarify. This is about wp_delete_attachment never returning false, even if no files are deleted. Try this, upload a media to wordpress. Lets call it myimage.png.
now go to the wordpress DB and run

SELECT * 
FROM  `wp_postmeta` 
WHERE  `meta_value` LIKE  '%myimage%'

delete that entry or damage it otherwise somehow. Not all plugins and themes create proper wp_postmeta entries, right?

now if you call wp_delete_attachment on that attachment, it will delete the DB entries, but not the file and it will not return false. It deleted something somehow a little bit, but doesnt let the programmer know that something happened a little bit but not really. Just imagine you have a couple thousand slightly damaged files in your wordpress DB, you run this function, never returns false or throws an error, then all DB entries are gone, and all files are still there.

Bad.

let me know if you need further explanation

[donmhico]

@Jossnaz - makes sense. I attached a potential fix for the issue. 47868.diff

I also created a test unit for change but an existing test in the REST API fails.

1) WP_Test_REST_Attachments_Controller::test_delete_item
Failed asserting that 500 matches expected 200.

I'm gonna look more into this a little later.

[donmhico]

47868.2.diff fixes the failing REST test. The problem is with the creation of the dummy attachment. It's not creating proper metas for the attachment which will result to wp_delete_attachment() returning false since no file was deleted. Hence the response status isn't 200 making the test fail.

What I did is to make the test use $this->_make_attachment() to create the dummy attachment.

We're gonna need to ask for more insights from core devs regarding this.