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).
Comments
Comment #1
jhodgdonThanks 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.
Comment #2
tankerjoe CreditAttribution: tankerjoe commentedI 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
Comment #3
mErilainen CreditAttribution: mErilainen commentedBut 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:
Comment #4
tankerjoe CreditAttribution: tankerjoe commentedThat's true, and I missed that bit (code blindness?)... But before that code is called, there is this:
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:
Comment #5
mErilainen CreditAttribution: mErilainen commentedIn 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.
Comment #6
jhodgdonbased on the last few comments...
Comment #7
mErilainen CreditAttribution: mErilainen commentedHmm, 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.
Comment #8
jhodgdonI 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?
Comment #9
mErilainen CreditAttribution: mErilainen commentedThere ya gooo
Comment #10
jhodgdonAside 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?
Comment #10.0
jhodgdonfixed link to API docs page
Comment #11
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedAttached patch for d7.
Comment #12
jhodgdonOops. 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.
Comment #13
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedI have attached patch against 8.x to remove that copy. Will provide another updated patch for 7.x when this one is committed.
Comment #14
jhodgdonThanks, 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.
Comment #15
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedThanks for your prompt response. I have enclosed patch for d7 now.
Comment #16
jhodgdonOh 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?
Comment #17
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedNice catch! Find the patch to fix the same.
Comment #18
jhodgdonThanks again! Committed this to 8.x. Back to 7.x for one more try. :)
Comment #19
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedPatch for d7 attached.
Hope I have covered everything needed this time :)
Comment #20
jhodgdonExcellent, thanks! I think we're safe to actually commit this to 7.x now. :)
Comment #21
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedIt's okay. Thanks for your timely follow up!
Comment #22
jhodgdonThanks again - committed to 7.x and I think we are finally done with this issue.