Fix wp_unique_filename() when converting images, take 2 by azaozz · Pull Request #1591 · WordPress/wordpress-develop

azaozz · 2021-08-16T08:43:49Z

Alt patch for https://core.trac.wordpress.org/ticket/53668 partially based on https://core.trac.wordpress.org/attachment/ticket/53668/53668.2.diff (take 2).

Extends wp_unique_filename() to account for image files that will be converted after uploading. The file names with the original and expected extensions are checked against existing files.

Trac ticket: https://core.trac.wordpress.org/ticket/53668.

ianmjones · 2021-08-16T12:29:35Z

src/wp-includes/functions.php

+
+		// If the extension is uppercase add an alternate file name with lowercase extension. Both need to be tested
+		// for uniqueness as the extension will be changed to lowercase for better compatibility with different filesystems.
+		if ( $ext && $lc_ext !== $ext ) {


As the new filename's extension is always going to be lowercase on exit, have you considered just setting the extension as lowercase early in the function, and then routinely testing both that filename and a version with an uppercase extension for existence later?

That would negate the need for if statements like this, but also cater for the case whereby a lowercase filename is uploaded but files with uppercase extensions exist that were added by some other means that does not run through wp_unique_filename (e.g. FTP).

have you considered just setting the extension as lowercase early in the function

Unfortunately $ext is passed as arg in the wp_unique_filename filter at the bottom. Changing it might affect something and would be considered a regression.

files with uppercase extensions exist that were added by some other means

That's not impossible but never seen a bug report about it, and is generally not supported. Files added "by hand" to the upload dirs would not have attachment posts and will be unusable from whitin WP. Perhaps if there's a bug report/ticket it can be fixed there.

Unfortunately $ext is passed as arg in the wp_unique_filename filter at the bottom. Changing it might affect something and would be considered a regression.

Ah, good shout, I'll fix my patch for that.

That's not impossible but never seen a bug report about it, and is generally not supported.

We've seen this very scenario a bunch of times in support, either via a dump of media from Windows and then sucked in to a gallery plugin that doesn't use the Media Library, or imported into the Media Library from the filesystem by some importer.

We've seen this very scenario a bunch of times

Did it result in lost/overwritten files? Sure, lets fix it but in another ticket and better not in a dot release.

Hm, actually, wouldn't file_exists() catch possible collisions caused by upper/lower case? It does a filesystem call so thinking it will.

Hm, actually, wouldn't file_exists() catch possible collisions caused by upper/lower case? It does a filesystem call so thinking it will.

No, on a case sensitive filesystem (effectively all Linux servers) file_exists( 'wibble.png' ) will not see a wibble.PNG file.

ianmjones · 2021-08-16T13:21:57Z

src/wp-includes/functions.php

+ * @param array    $files     An array of existing files in the directory. May be empty.
+ * @return bool True if the tested file name could match an existing file, false otherwise.
+ */
+function _wp_check_alternate_file_names( $filenames, $dir, $files ) {


I thought a primary reason for proposing an alternate patch to mine was to avoid the introduction of a private helper function? 😄

Hehe, not primary but yes, avoid if possible. It only makes things a bit more readable, the conditionals can also be in the while() loop.

The primary reason is to not run wp_unique_filename() recursively.

The primary reason is to not run wp_unique_filename() recursively.

This is where I very much disagree.

wp_unique_filename() should be called recursively to ensure the best method for checking filenames is always used, and to ensure that its filter is fired for each proposed filename, including the alternate format filenames that otherwise only wp_unique_filename() itself would check.

My next path has a test case that explains this better.

should be called recursively...

This introduces a bug/edge case, see https://core.trac.wordpress.org/ticket/53668#comment:25. All alternate filenames should be checked together at the same time or "gaps" in the numbering (i.e. existing files like file-1, file-2, file-4) may result in overwriting.

This introduces a bug/edge case, see https://core.trac.wordpress.org/ticket/53668#comment:25. All alternate filenames should be checked together at the same time or "gaps" in the numbering (i.e. file-1, file-2, file-4) will result in overwriting.

I could be wrong, but I don't believe my code has this bug as it re-checks the original and alternate extensions set should a new version be proposed on exit from the protected loop.

Do you have a test case for this that breaks my latest patch?

ianmjones · 2021-08-17T12:59:34Z

src/wp-includes/functions.php


 		// The (resized) image files would have name and extension, and will be in the uploads dir.
-		if ( $name && $ext && @is_dir( $dir ) && false !== strpos( $dir, $upload_dir['basedir'] ) ) {
+		if ( $name && $ext && $is_image && @is_dir( $dir ) && false !== strpos( $dir, $upload_dir['basedir'] ) ) {


I think testing for $is_image here could cause problems. As per the first early test for thumbnail/scaled/rotated to append a version regardless of type to avoid clashes with images that might end up with those suffixes, entering this section to test the newly versioned file name for the existence of files that signify a potential base name clash is important.

It's especially important when file types like PDF and audio could have preview/cover images generated for them, avoiding potential clashes and disassociated filenames by keeping the existing functionality seems worthwhile.

Right, fixed.

…llisions

ianmjones

Apart from the one minor code comment regarding undefined variables, looking great @azaozz. 🎉

I ran the tests, manually tested a bunch of scenarios, and reviewed the code, passed with flying colours!

src/wp-includes/functions.php

azaozz · 2021-08-24T20:53:10Z

Committed in https://core.trac.wordpress.org/changeset/51653.

Add changed files..

2f69572

ianmjones reviewed Aug 17, 2021

View reviewed changes

azaozz added 4 commits August 17, 2021 09:51

Check all file types when looking for possible image sub-size name co…

5f28461

…llisions

Add $alt_filenames and $number to the wp_unique_filename filter params

ecdb72e

Move $alt_filenames

424ed61

Update tests and inline comments, add two more test images

e222a97

ianmjones reviewed Aug 24, 2021

View reviewed changes

src/wp-includes/functions.php Outdated Show resolved Hide resolved

azaozz added 2 commits August 24, 2021 12:51

Initialize vars used in the wp_unique_filename filter

b71161b

Fix white spaces

d033c3a

azaozz closed this Aug 24, 2021

azaozz deleted the Fix-wp_unique_filename()-when-converting-images branch August 24, 2021 20:57

Conversation

azaozz commented Aug 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azaozz Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azaozz Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianmjones left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

azaozz commented Aug 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

azaozz Aug 17, 2021 •

edited

Loading

azaozz Aug 17, 2021 •

edited

Loading