The file_copy function in file.inc provides automatic file renaming in case of name conflict, but doesn't put the final name in $file->filename. This makes it harder for modules to determine whether they own a file presented to hook_file_download(); in particular, flexinode modules will fail to determine that they own a file that got renamed.

If it is agreed that the final filename should be returned in $file->filename, that behavior is easily imlemented by updating $basename as well as $dest around line 261. I will be happy to build and test a patch for this. If the current behavior is by design, I will hack on my flexinode module fields instead. What is the call?

Regards,
Claude

CommentFileSizeAuthor
#2 fix_file_copy_rename_without_notification.patch460 bytesjsaints
#1 file.inc.update_filename.patch623 bytesejort

Comments

ejort’s picture

ejort commented
Category: feature » bug
StatusFileSize
new file.inc.update_filename.patch623 bytes

Is there anybody who has an opinion on this? Personally I find it counter-intuitive for the filepath to be updated but not the filename, and I can't think of any uses where the original filename is preferable to the actual filename. In any case the caller still has access to the original filename if they care about it, what's returned should reflect reality IMO.

I've been using this patch (against 4.6) so that the flexinode file field is usable, as cidenton said it's a trivial fix, but without it any file that is uploaded through the flexinode file field that must be renamed cannot be downloaded (making this unusable in production). Because of this I'm changing this from a feature request to a bug report.

Cheers,
Eric

jsaints’s picture

jsaints commented
Priority: Normal » Critical
StatusFileSize
new fix_file_copy_rename_without_notification.patch460 bytes

I have the same problem in a custom module I wrote for drupal CVS (4.7 beta3 ) that uses the function file_copy with FILE_EXISTS_RENAME to move uploaded images from file/images/temp to files/images.

FILE_EXISTS_RENAME in file_copy() is renaming the destination file if there is a collision, but not sending the new name out to the world so that the rest of my code knows the new incremented file name where the image was really copied.

To clarify: If a file my_image.jpg already exists in the "images" directory and I run:

file_copy('temp/my_image.jpg', 'images/my_image.jpg');

file_copy, returns TRUE, yet upon file collision, silently renamed the file my_image.jpg to my_image_0.jpg and there is no indication of the rename returned from the file_copy function. The code calling the file_copy function needs to know this information in order to store/display the actual file the user copied. Running this code as is seems to me that it could cause major mix ups with file associations.

The solution for 4.7 beta 3 CVS is the following patch which simply passes the $dest variable by reference as &$dest so that after file copy is run, any code can look at dest and see that the file copied is named my_image_0.jpg and instead of my_image.jpg.

dries’s picture

dries commented

Maybe this behavior should be documented in the function's PHPdoc?

jsaints’s picture

jsaints commented
Status: Needs review » Closed (fixed)

Reading the PHPdoc closer I noticed:

 * @param $source A string specifying the file location of the original file.
 *   This parameter will contain the resulting destination filename in case of
 *   success.

I tested, and yes, the resulting file name is stored in the variable that is passed in as $source. This one is counterintuitive, but is not a bug.