API page: https://api.drupal.org/api/drupal/includes!file.inc/function/file_copy/7

Enter a descriptive title (above) relating to Preview comment, then describe the problem you have found:

Documentation says: "Checks that $source is not equal to $destination; if they are an error is reported." which might be confusing:
If you use FILE_EXISTS_RENAME in $replace param, the file will be copied EVEN if source and destination are the same, meaning you are duplicating a file (in the same directory).

Files: 
CommentFileSizeAuthor
#19 file-file_copy_doc_confusing-2084535-19.patch1.97 KBsivaji
PASSED: [[SimpleTest]]: [MySQL] 40,262 pass(es).
[ View ]
#17 2084535_FILE_EXIST_17.patch1.58 KBsivaji
PASSED: [[SimpleTest]]: [MySQL] 60,276 pass(es).
[ View ]
#15 file-file_copy_doc_confusing-2084535-15.patch1.96 KBsivaji
PASSED: [[SimpleTest]]: [MySQL] 40,258 pass(es).
[ View ]
#15 2084535_interdiff_12_15.txt679 bytessivaji
#13 file-file_copy_doc_confusing-2084535-13.patch699 bytessivaji
PASSED: [[SimpleTest]]: [MySQL] 60,275 pass(es).
[ View ]
#11 file-file_copy_doc_confusing-2084535-11.patch1.72 KBsivaji
PASSED: [[SimpleTest]]: [MySQL] 40,433 pass(es).
[ View ]
#9 file-file_copy_doc_confusing-2084535-9.patch1.86 KBmErilainen
PASSED: [[SimpleTest]]: [MySQL] 58,475 pass(es).
[ View ]
#7 file-file_copy_doc_confusing-2084535-7.patch1.06 KBmErilainen
PASSED: [[SimpleTest]]: [MySQL] 58,853 pass(es).
[ View ]
#2 file-file_copy_doc_confusing-2084535-2.patch1.04 KBtankerjoe
PASSED: [[SimpleTest]]: [MySQL] 59,209 pass(es).
[ View ]

Comments

Version:7.x-dev» 8.x-dev
Issue tags:+Novice, +needs backport to D7

Thanks for the report! I agree, that documentation is confusing. Probably it should say "... If they are, the action taken depends on the $replace parameter" or something similar.

Seems like a good Novice project. Needs to be fixed in 8.x and then backported to 7.x.

Status:Active» Needs review
StatusFileSize
new1.04 KB
PASSED: [[SimpleTest]]: [MySQL] 59,209 pass(es).
[ View ]

I checked out the source code to see what is actually happening. There is no explicit check anywhere to see if the $source and the $destination are the same file. It just follows the behavior specified by the $replace param like it would any other file that exists. That being said, I think it makes sense remove that bullet point and rework the documentation a bit.

Patch for 8.x

But in file_unmanaged_copy() (https://api.drupal.org/api/drupal/includes!file.inc/function/file_unmana...) there is a check with an error message.

I didn't test this, but looking at code the if rule should return false:
if ($uri = file_unmanaged_copy($source->getFileUri(), $destination, $replace))
because in file_unmanaged_copy() is this check:

<?php
if ($source == $destination || ($real_source !== FALSE) && ($real_source == $real_destination)) {
   
drupal_set_message(t('The specified file %file was not copied because it would overwrite itself.', array('%file' => $source)), 'error');
   
watchdog('file', 'File %file could not be copied because it would overwrite itself.', array('%file' => $source));
    return
FALSE;
  }
?>

That's true, and I missed that bit (code blindness?)... But before that code is called, there is this:

<?php
$destination
= file_destination($destination, $replace);
?>

https://api.drupal.org/api/drupal/core%21includes%21file.inc/function/file_destination/8

So basically, if $source and $destination are the same file, it will only return false if $replace is not FILE_EXISTS_RENAME. Maybe something like this would make it more clear:

* If the $source and $destination are equal, the behaviour depends on the
* $replace parameter.  FILE_EXIST_REPLACE will error out.  FILE_EXIST_RENAME
* will rename the file until the $destination is unique.

In the current state the mention "Checks that $source is not equal to $destination; if they are an error is reported." has been removed, so I think this issue has been resolved. There is no need to say what the different parameters do here, because they are documented a couple of lines lower.

Status:Needs review» Needs work

based on the last few comments...

Status:Needs work» Needs review
StatusFileSize
new1.06 KB
PASSED: [[SimpleTest]]: [MySQL] 58,853 pass(es).
[ View ]

Hmm, them mention I mentioned is still there, maybe I was looking at wrong version on my editor.
Anyway, here is the patch with the change which tankerjoe proposed. It makes sense and explains the function logic for newbie.

Status:Needs review» Needs work

I agree with all of the above analysis -- thanks for all the participation!

I also like the current patch in #7. However, we need to fix this in the documentation of both file_unmanaged_copy and file_copy. Can you add a bit to the patch so that the same fix is in both functions?

Status:Needs work» Needs review
StatusFileSize
new1.86 KB
PASSED: [[SimpleTest]]: [MySQL] 58,475 pass(es).
[ View ]

There ya gooo

Version:8.x-dev» 7.x-dev
Status:Needs review» Patch (to be ported)

Aside from the British spelling of behaviour (should be behavior without the u, as we use American spelling in the Drupal project), this looks great!

So I fixed that and committed the patch to 8.x. Thanks all!

The patch doesn't quite apply to 7.x. Can someone port it (with the correct spelling) please?

Issue summary:View changes

fixed link to API docs page

Issue summary:View changes
Status:Patch (to be ported)» Needs review
StatusFileSize
new1.72 KB
PASSED: [[SimpleTest]]: [MySQL] 40,433 pass(es).
[ View ]

Attached patch for d7.

Version:7.x-dev» 8.x-dev
Status:Needs review» Needs work

Oops. It looks like we need to go back to Drupal 8.x and remove one more copy of that line that says an error will happen if source and destination are equal (and then that needs to be removed from 7.x as well):
https://api.drupal.org/api/drupal/core!includes!file.inc/function/file_u...

I noticed this while reviewing the 7.x patch and we all missed it in the 8.x patch.

Status:Needs work» Needs review
StatusFileSize
new699 bytes
PASSED: [[SimpleTest]]: [MySQL] 60,275 pass(es).
[ View ]

I have attached patch against 8.x to remove that copy. Will provide another updated patch for 7.x when this one is committed.

Version:8.x-dev» 7.x-dev
Status:Needs review» Needs work

Thanks, quick work! I committed this patch to 8.x and we can go back to 7.x, where the patch needs this same addition to it.

Status:Needs work» Needs review
StatusFileSize
new679 bytes
new1.96 KB
PASSED: [[SimpleTest]]: [MySQL] 40,258 pass(es).
[ View ]

Thanks for your prompt response. I have enclosed patch for d7 now.

Version:7.x-dev» 8.x-dev
Status:Needs review» Needs work

Oh gracious, one more 8.x problem: The constants are called FILE_EXISTS* not FILE_EXIST* -- can someone fix that and then we can (again) go back to 7.x?

Status:Needs work» Needs review
StatusFileSize
new1.58 KB
PASSED: [[SimpleTest]]: [MySQL] 60,276 pass(es).
[ View ]

Nice catch! Find the patch to fix the same.

Version:8.x-dev» 7.x-dev
Status:Needs review» Needs work

Thanks again! Committed this to 8.x. Back to 7.x for one more try. :)

Status:Needs work» Needs review
StatusFileSize
new1.97 KB
PASSED: [[SimpleTest]]: [MySQL] 40,262 pass(es).
[ View ]

Patch for d7 attached.

Hope I have covered everything needed this time :)

Status:Needs review» Reviewed & tested by the community

Excellent, thanks! I think we're safe to actually commit this to 7.x now. :)

It's okay. Thanks for your timely follow up!

Status:Reviewed & tested by the community» Fixed

Thanks again - committed to 7.x and I think we are finally done with this issue.

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.