get_attached_file() destroys file paths on Windows

[Whissi]

While working on ticket #36273 I noticed that get_attached_file() from wp-includes/post.php will destroy paths normalized by wp_normalize_path() on Windows:

For example the function starts with

$file = get_post_meta( $attachment_id, '_wp_attached_file', true );
// $file = 'C:/WWW/Sites/demo/htdocs/wordpress/wp-content/uploads/2016/03/example.jpg'

However this will become

$file = 'C:\WWW\Sites\demo\htdocs\wordpress/wp-content/uploads/C:/WWW/Sites/demo/htdocs/wordpress/wp-content/uploads/2016/03/example.jpg'

due to

if ( $file && 0 !== strpos($file, '/') && !preg_match('|^.:\\\|', $file) && ( ($uploads = wp_upload_dir()) && false === $uploads['error'] ) )
        $file = $uploads['basedir'] . "/$file";

This is similar to ticket #24824 however we are dealing will full qualified paths here, not URLs (well, both are URIs...).

PS: Yes, $uploads['basedir'] contains mixed directory separators. That's another thing.

[desrosj]

As noted above, related #24824.

[birgire]

There is actually a core function called path_is_absolute():

​https://developer.wordpress.org/reference/functions/path_is_absolute/

so I wonder why it isn't used in get_attached_file(), like:

if ( ! path_is_absolute( $file ) && ( ( $uploads = wp_get_upload_dir()) && false === $uploads['error'] ) )
        $file = $uploads['basedir'] . "/$file";

In that case path_is_absolute() could be further updated to handle url or normalized paths.

There's also the path_join():

​https://developer.wordpress.org/reference/functions/path_join/

that uses path_is_absolute() internally. This is e.g. used in _wp_upload_dir().

[DrewAPicture]

Assigning to mark the good-first-bug as "claimed".

@stevenlinx Thanks for the patches! It likes we're still in need of unit tests here.

[birgire]

In 36308.4.diff​ I adjusted 36308.3.diff​ according to the Coding Standard and added more tests to the test_path_is_absolute() test method.

[stevenlinx]

1.)
Thanks @birgire

2.)
@DrewAPicture,

I assume we should also commit ticket #24824 at the same time? since the two are quite related (as noted above).