wp_delete_file filter expects both absolute and relative paths

[rmccue]

The wp_delete_file filter is called in a number of places. In particular, it's called in wp_delete_file() and wp_delete_attachment(). Unfortunately, the filter acts inconsistently.

In wp_delete_file(), the result of the filter is passed to unlink directly (assuming it's not empty). In wp_delete_attachment() however, the file is combined with the upload base directory using path_join().

In most normal circumstances, this is not an issue, as path_join() no-ops when passed an absolute path. However, when using stream wrappers with a custom protocol (such as s3://), path_join() does not recognise these as absolute paths and redundantly joins the paths, resulting in a broken path. The error suppression asperand (@) hides this invalid path error.

There's two obvious solutions we could choose:

1. Be consistent with passing paths in. Either always absolute or always relative.
2. Just use wp_delete_file in wp_delete_attachment. This forces it to be consistent. This implies being consistently absolute.

As the filter is currently inconsistent, I don't think there's a huge amount of damage to changing it to be always absolute.

When wp_delete_file() was added in #17864, it was used in most places, but not in wp_delete_attachment() for Unknown Reasons. Every other invocation of the function (and hence filter) is absolute.

Patch attached.

[rmccue]

Use wp_delete_file() in wp_delete_attachment

[joemcgill]

Not sure about the deep history here, but this seems sane to me. We should probably do a cursory look to see if anyone is doing anything unusual with that filter, where relative paths are expected.

[dd32]

Seems sane to me also, I have no idea of the history here either.

I'm running a scan on a plugins checkout to see if I can see anything relying upon the behaviour, but even if I do, I don't think that's not a good reason to change this.
I don't see a need to push this for 4.7.1 though.

[rmccue]

Replying to dd32:

I don't see a need to push this for 4.7.1 though (with the pending commit deadline, and testing time).

4.7.2 is fine by me.

[dd32]

Seems most plugins are related to deleting caches and extra files after attachment deletion. A bunch of the plugins however appear to expect to receive relative paths on the filter.
A bunch of others pass what appears to probably be relative paths to the wp_delete_file filter too.

I don't really see it as a blocker to change here, but I question if it should be in a point release. Here's the list of plugins who include wp_delete_file and filter on the same line if someone would like to look closer.

polylang
post-thumbnail-editor
selectel-storage-upload
wp2cloud-wordpress-to-cloud
geodirectory
mailbase
evdbfiles
tantan-s3
upload-to-ftp
scissors-continued
profile-custom-content-type
editor-by-mintboard
hammy
detuyun-image-cloud-storage
qiniu-cloud
upyun
file-gallery
scissors-watermark
hm-portfolio
storageqloud-for-wordpress
mapify-lite-google-maps-plus
wp-thumb
rotating-header
scissors
cdn-tools
tantan-s3-cloudfront
xili-language
foogallery
conoha-object-sync

[Eric3D]

Can we simply look for :// in the string and treat it like absolute path if it's present and as relative path if it's not?

[rmccue]

@Eric3D I'd be happy with that, but we need to be careful making that change to ensure it's always applied. In any case, I still think it's a good idea to unify the behaviour here; whether that's to relative or to absolute, I don't mind.

[joemcgill]

I think the approach in 39476.diff​ makes sense, but this should probably have longer time in trunk so I'm going move this to the major.

[gitlost]

Of the 3 branches, only the last $backup_sizes gives a relative path to the wp_delete_file filter, as the other 2, $meta['thumb'] and $meta['sizes'], do a str_replace() on the absolute path $file, so the path_join() with $uploadpath['basedir'] is always a no-op (except when it messes up).

So the behavioural change would only be for the backup sizes.

I think it would make sense to make all 3 cases the same basic code, avoiding the non-semantic str_replace()/basename() combo by using dirname() instead, which would also fix the UTF-8 issue #33227.

The following patch does this, with unit tests for backup sizes/wp_delete_file filter and the #33227 UTF-8 case, and some cleaning up of other tests.

(I looked at the reasonably recently updated plugins in those listed above

polylang, post-thumbnail-editor, geodirectory, upload-to-ftp, editor-by-mintboard, file-gallery, mapify-lite-google-maps-plus, xili-language, foogallery, conoha-object-sync

and only mapify-lite-google-maps-plus and foogallery, which both use the no longer maintained ​https://github.com/humanmade/WPThumb, were expecting a relative path.)

[gitlost]

Use dirname(), with unit tests.

[gitlost]

Remove unnecessary "cleanup" (save/restore of options).

[gitlost]

Set $prev_ctype_locale properly in unit test.

[kirasong]

After chatting with @azaozz and @stephdau about this, thinking that it needs more time to soak than we have before 4.8.

I'd like to see this land, but going to punt to Future Release for now, with the idea that it should land early in the next major.

[joemcgill]

#38907 was marked as a duplicate.

[calin]

@mikeschroder, @joemcgill is thare any resolution for this bug?

[SergeyBiryukov]

It looks like this was fixed in 4.9.7 by [43392].

wp_delete_attachment() no longer applies the wp_delete_file filter, only wp_delete_file() does.