The error messages in file_unmanaged_copy have been horrible as long as I can remember. They're at their worst when the $destination parameter is omitted (which is probably more often than not.) What is worse is that these error messages are very common for new users who do not know how to set the permissions correctly on the sites/default/files directory, so the very confusing messages cause even more difficulty.

Some example code, with the results as changed by the attached patch:

  file_unmanaged_copy("/tmp/bad_file.txt");  // File does not exist
  file_unmanaged_copy("/tmp/good_file.txt"); // files directory perms bad
  file_unmanaged_copy("/tmp/good_file.txt","/tmp/nonexistent/good_file.txt"); // Nonexistent dir
  file_unmanaged_copy("/tmp/good_file.txt", "/tmp/directory_with_bad_perms/good_file.txt");

Old Messages:

* The specified file could not be copied, because no file by that name exists. Please check that you supplied the correct filename.
* The specified file /tmp/good_file.txt could not be copied, because the destination sites/default/files is not properly configured.
* The specified file /tmp/good_file.txt could not be copied, because the destination is not properly configured.
* The specified file /tmp/good_file.txt could not be copied, because the destination /tmp/directory_with_bad_perms/good_file.txt is not properly configured.

New Messages: 

* The specified file /tmp/bad_file.txt could not be copied, because no file by that name exists. Please check that you supplied the correct filename
* The specified file /tmp/good_file.txt could not be copied, because the destination sites/default/files is not properly configured. This is often a permissions problem.
* The specified file /tmp/good_file.txt could not be copied, because the destination /tmp/nonexistent/good_file.txt is not properly configured. This is often a permissions problem.
* The specified file /tmp/good_file.txt could not be copied, because the destination /tmp/directory_with_bad_perms/good_file.txt is not properly configured. This is often a permissions problem.

Let's consider this the simplest usability patch we can do!

CommentFileSizeAuthor
#16 file_unmanaged_copy_error_messages_511760_d7_06.patch1.71 KBrfay
#14 file_unmanaged_copy_error_messages_511760_d7_05.patch1.7 KBrfay
#11 file_copy_error_message_d6_01.patch3.17 KBrfay
#7 file_unmanaged_copy_error_messages_04.patch2.56 KBrfay
#4 file_unmanaged_copy_error_messages_03.patch2.56 KBrfay
#2 file_unmanaged_copy_error_messages_02.patch2.53 KBrfay
file_unmanaged_copy_error_messages_01.patch2.58 KBrfay
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drewish’s picture

drewish CreditAttribution: drewish commented
Status: Active » Needs work

i think this would be great but my only concern is renaming the parameters. since we need a copy of the original paths for the error messages i'd suggest making a copy of them at the beginning of the function ($original_source and $original_destination) and letting $source and $destination continue to be modified.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Needs work » Needs review
FileSize
file_unmanaged_copy_error_messages_02.patch2.53 KB

Rerolled the patch with parameters unchanged. Thanks for the review.

drewish’s picture

drewish CreditAttribution: drewish commented
Status: Needs review » Needs work

we typically try to avoid abbreviating so orig should be original. can we avoid adding $proposed_destination and just use $destination?

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Needs work » Needs review
FileSize
file_unmanaged_copy_error_messages_03.patch2.56 KB

I changed orig -> original.

However, $destination is reused so often here that something has to be used to keep the state of what we're trying to do so we can report it. Basically, the code always does things like $destination = dosomething($destination), and that's the reason for this.

So I kept $proposed_destination.

Dries’s picture

Dries CreditAttribution: Dries commented

Looks good to me but it would be great if drewish could sign off on it.

drewish’s picture

drewish CreditAttribution: drewish commented

that looks great, i'd be happy with that committed but if i'm nit picking i'd suggest that
!empty($original_destination) ? $original_destination : $proposed_destination))
be changed to:
empty($original_destination) ? $proposed_destination : $original_destination))

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
FileSize
file_unmanaged_copy_error_messages_04.patch2.56 KB

Your nitpick is my command.

drewish’s picture

drewish CreditAttribution: drewish commented
Status: Needs review » Reviewed & tested by the community

since dries was happy with it before and the test bot is happy, i think we can call this RTBC.

drewish’s picture

drewish CreditAttribution: drewish commented

i'd actually consider back porting this to 6 as well.

Dries’s picture

Dries CreditAttribution: Dries commented
Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Needs work

Committed to CVS HEAD for inclusing in Drupal 7. Updating version and status. Thanks all.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Assigned: Unassigned » rfay
Status: Needs work » Needs review
FileSize
file_copy_error_message_d6_01.patch3.17 KB

Here is a D6 version of the patch

drewish’s picture

drewish CreditAttribution: drewish commented
Status: Needs review » Needs work

I tested all the changes in this patch by hand and noticed that it was a big improvement in all cases except for this:

$source = 'misc/grippie1.png';
file_copy($source, 0, FILE_EXISTS_ERROR);
file_copy($source, 0, FILE_EXISTS_ERROR);

This case gives two error messages:

* The file misc/grippie1.png could not be copied, because no file by that name exists. Please check that you supplied the correct filename.
* The file /Users/amorton/Sites/d6 could not be copied to sites/default/files/d6.

One from file_copy() and one from file_destination(). I'd suggest that we fix the message in file_destination() and replace the message in file_copy() with a comment that file_destination() will take care of the error message.

drewish’s picture

drewish CreditAttribution: drewish commented
Version: 6.x-dev » 7.x-dev

I should have checked this earlier but it's actually still a problem in HEAD too. Bumping this back there for a followup.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Needs work » Needs review
FileSize
file_unmanaged_copy_error_messages_511760_d7_05.patch1.7 KB

Here's a patch to fix the duplicate error messages.

After reviewing where the messages ought to go, I decided that file_destination() was probably not ever intended to emit error messages; it seems to be more of a probing function, returning info to the caller. Also, file_destination did not have adequate information to emit a good message and of course it would have required a signature change to get the info to it. So I removed the error message from file_destination().

There are two (known) callers of file_destination(), file_unmanaged_copy() and file_save_upload(). I fixed the error message in both of the callers.

drewish’s picture

drewish CreditAttribution: drewish commented

In file_unmanaged_copy(), going from:
'The specified file %file could not be copied because a file by that name already exists in the destination %directory.'
To:
'The specified file %file could not be copied because a file in the destination already exists. See %directory.'
Seems like a step back. I like the new message in file_save_upload():
'The file %source could not be uploaded because a file by that name already exists in the destination %directory.'
So how about a combo?:
'The file %source could not be copied because a file by that name already exists in the destination %directory.'

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
FileSize
file_unmanaged_copy_error_messages_511760_d7_06.patch1.71 KB

OK, I changed that one, with a slight variation on your wording.

The two types of error messages now emitted by this line are:

  • The file /tmp/good_file.txt could not be copied because a file by that name already exists in the destination directory (sites/default/files//tmp/existing_file.txt)
  • The file /tmp/good_file.txt could not be copied because a file by that name already exists in the destination directory (sites/default/files)
drewish’s picture

drewish CreditAttribution: drewish commented

that seems odd that it's trying to put the file into sites/default/files//tmp/existing_file.txt rather than sites/default/files/existing_file.txt

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

@drewish, I'm glad you're paying attention.

This is actually at *least* a discrepancy between the documentation of the various functions and their behavior.

file_unmanaged_copy() claims that the destination can be a "string containing the destination that $source should be copied to. This can be a complete file path, a directory path or, if this value is omitted, Drupal's 'files' directory will be used."

However, it calls file_create_path() on the $destination, which returns "A string containing the path to file, with file system directory appended if necessary, or FALSE if the path is invalid (i.e. outside the configured 'files' or temp directories)."

There are a number of problems here:

  • file_unmanaged_copy() is unable to work on a destination that is outside the configured files or temp directories. Maybe that's OK, maybe it's not. I don't know the intent.
  • The documentation for file_unmanaged_copy() doesn't indicate the reality (or the reality doesn't indicate the documentation
  • The documentation for file_create_path() says "with the system directory appended", when it means "with the system directory prepended".

The call that exposes this line of code is

file_unmanaged_copy('/tmp/good_file.txt', '/tmp/existing_file.txt', FILE_EXISTS_ERROR);

(both files exist)

I just had a look in the tests, and it appears that there is no test case where the destination directory is outside the system files directory.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

I will take a look today and see if this was introduced in D7, or if it's a documentation problem inherited in the code.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

@drewish, it turns out you caught a bug with your sharp eye (#17)

#528114: file_unmanaged_copy() will not write into $destination directory; it always goes into system 'files' or 'tmp' directory

Probably we can continue with this one and fix that separately.

Status: Needs review » Needs work

The last submitted patch failed testing.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Needs work » Needs review

Looks like this was the dreaded "Failed to install head"

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

@drewish, All the issues with this have been resolved - it turned out that it is in fact just an additional error message change. Due to the fact that we discovered another problem/issue (#528114: file_unmanaged_copy() will not write into $destination directory; it always goes into system 'files' or 'tmp' directory) we kind of got derailed. But I'd like to get this one done and get back to D6 and see if we can get that committed. What's your opinion?

drewish’s picture

drewish CreditAttribution: drewish commented
Status: Needs review » Reviewed & tested by the community

rfay, yeah i think we should just get this wrapped up in D7 and then backported to D6. the last patch in #16 looks like it's okay by the test bot. lets get this in.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks, committed to HEAD.

It looks like this will need a re-roll for 6.x. We normally don't break strings in a stable release, but I see that Dries already marked this for 6.x once.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Assigned: rfay » Unassigned

Status: Patch (to be ported) » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.