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).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

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.

tankerjoe’s picture

Status: Active » Needs review
FileSize
1.04 KB

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

mErilainen’s picture

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;
  } ?>
tankerjoe’s picture

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

$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.
mErilainen’s picture

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.

jhodgdon’s picture

Status: Needs review » Needs work

based on the last few comments...

mErilainen’s picture

Status: Needs work » Needs review
FileSize
1.06 KB

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.

jhodgdon’s picture

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?

mErilainen’s picture

Status: Needs work » Needs review
FileSize
1.86 KB

There ya gooo

jhodgdon’s picture

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?

jhodgdon’s picture

Issue summary: View changes

fixed link to API docs page

Sivaji_Ganesh_Jojodae’s picture

Issue summary: View changes
Status: Patch (to be ported) » Needs review
FileSize
1.72 KB

Attached patch for d7.

jhodgdon’s picture

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.

Sivaji_Ganesh_Jojodae’s picture

Status: Needs work » Needs review
FileSize
699 bytes

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

jhodgdon’s picture

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.

Sivaji_Ganesh_Jojodae’s picture

Status: Needs work » Needs review
FileSize
679 bytes
1.96 KB

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

jhodgdon’s picture

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?

Sivaji_Ganesh_Jojodae’s picture

Status: Needs work » Needs review
FileSize
1.58 KB

Nice catch! Find the patch to fix the same.

jhodgdon’s picture

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. :)

Sivaji_Ganesh_Jojodae’s picture

Status: Needs work » Needs review
FileSize
1.97 KB

Patch for d7 attached.

Hope I have covered everything needed this time :)

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

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

Sivaji_Ganesh_Jojodae’s picture

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

jhodgdon’s picture

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.