Opened 6 years ago
Last modified 6 years ago
#45075 new defect (bug)
wp_delete_attachment() has unreliable return and wrong process order
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | |
Component: | Media | Keywords: | |
Focuses: | Cc: |
Description
When doing permanent deletion, this function should also remove the files of the target attachment. However successful return of this function does not guarantee this even though it still can delete the attachment object and its relations.
Problems when permanent deletion is in process:
wp_delete_attachment()
must check forwp_delete_attachment_files()
's return and should fail too whenwp_delete_attachment_files()
fails
wp_delete_attachment_files()
should be called first - what is the point of deleting object and it's relations when physical data could not be removed...
Change History (3)
#1
@
6 years ago
- Component changed from General to Media
- Keywords attachment delete attachments thumbnails files removed
#2
in reply to:
↑ description
@
6 years ago
- Severity changed from major to critical
#3
@
6 years ago
- Milestone changed from Awaiting Review to Future Release
- Severity changed from critical to normal
Hi @javeweb, thanks for the detailed report and investigation into the logic chain here.
wp_delete_attachment_files() should be called first - what is the point of deleting object and it's relations when physical data could not be removed...
Deleting files as the last stage in the process seems to be intentional, since the first time this is called, the attachment post is moved to the trash instead of deleted. We might consider returning an error if there was an error with deleting the file, but at that point the post will already be gone from the database, so the file will need to be manually deleted.
In terms of severity, this is not a major breakage from previous behavior, so I don't see this being a critical bug. What's more important would be to see cases where deletion fails and fix those issues specifically.
Replying to jave.web:
I've just investigated more and I find a horrifying thing:
WP does not check if the files were really deleted or not at all!
wp_delete_attachment_files()
- checks forwp_delete_file_from_directory()
returnhowever:
wp_delete_file_from_directory()
is guessing its return value from filepath comparsion while just callingwp_delete_file()
to the wind (agaín, not checking for its return)This whole shocker ends in
wp_delete_file()
which does not bother to check if the file was really deleted, altough it is aware of possible errors since it silences them with@
- which is a good practice - but only if you check if it really happened.wp_delete_file()
- Shockingly, it does not return anything at all and this is the place, I think, it all started.If you consider user's right to his personal data and the right to delete them, leaving the files over the internet can cause some legal issues - therefore I am raising the severity to critical. (One should check H-self in this case, but still WP core should handle this correctly)