Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Hi,
file_copy() D7 docs say:
"If this value is omitted, Drupal's default files scheme will be used, usually "public://"."
From my experience this is not true.
file_valid_uri() will call file_uri_scheme() with the NULL value and they just return false and you get all those warnigs displayed.
My patch adds an empty check so we can just use file_unmanaged_copy() as this will handle a NULL destination correctly.
Steps to reproduce
Proposed resolution
TBD
Remaining tasks
TBD
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | 1048114-21.patch | 1.85 KB | quietone |
| #20 | 1048114-20.patch | 2.02 KB | quietone |
Comments
Comment #1
Kars-T CreditAttribution: Kars-T commentedRenamed patch for autotesting.
Comment #2
trobey CreditAttribution: trobey commentedThe function signature for file_copy and file_unmanaged_copy:
function file_copy(stdClass $source, $destination = NULL, $replace = FILE_EXISTS_RENAME)
function file_unmanaged_copy($source, $destination = NULL, $replace = FILE_EXISTS_RENAME)
If NULL is used as the default argument for $destination then the two functions have to work with this default; otherwise, NULL should not be specified as the default and the function should require two arguments. The first thing that is called in file_copy() is file_valid_uri(). file_valid_uri() calls file_uri_scheme(). file_uri_scheme returns FALSE for a NULL argument. The fix in the patch corrects this problem.
Once that is fixed, NULL is passed to file_unmanaged_copy(). After checking the $source, we have this code:
// Build a destination URI if necessary.
if (!isset($destination)) {
$destination = file_build_uri(basename($source));
}
So the code in the patch should work fine.
Testing
I installed the examples module and used the file_example to create a file. I added a file_copy to the end of the submit function to test this. A second argument was not supplied. The result was the error:
The specified file public://test/test could not be copied, because the destination is invalid. More information is available in the system log.
After the patch the file was copied with no error.
Comment #3
catchWhy can't file_valid_uri() check for empty() instead?
Moving to 8.x and adding backport tag.
Comment #4
Kars-T CreditAttribution: Kars-T commentedI not 100% sure about if there is any agreement for core development. But as it is an API function maybe it should require some string or it wouldn't make sense?
Comment #5
trobey CreditAttribution: trobey commentedI do not think it is a good idea for file_valid_uri() to check for empty. It is called by functions such as file_delete(). File_delete() would either have to handle the empty case or it would try to remove a null file and delete it from the database. Furthermore, the documentation requires that a scheme such as private, public or temporary be supplied. Since NULL is often used to return a failure I do not think passing it to field_valid_uri() should pass. If it does than code will have to check for both empty() and field_valid_uri().
Comment #6
catchI meant file_valid_uri() could check for empty and immediately return false. Null isn't a valid file uri. If we think it's a silly check to do and it should just happen in file_copy() then fine but I wan't suggesting return true from it.
Comment #7
trobey CreditAttribution: trobey commentedThe problem is that the default value of $destination is null. file_copy() must be able to handle a default argument. You stated that null is not a valid file uri. If file_valid_uri returns false for a null URI then file_copy() fails. So it makes sense to check for null before generating error messages and exiting just like this patch does.
Comment #8
jhedstromNeeds a reroll.
Comment #9
AjitSRerolled manually for 8.0.x
Comment #10
mgiffordRe-submitting the prior patch for the bots to review.
Comment #13
20th CreditAttribution: 20th commentedComment #20
quietone CreditAttribution: quietone as a volunteer commentedTriaging for Bug Smash Initiative.
This issue is from 2011 and while the file system has had many changes since then the $destination argument is not tested for NULL in file_copy. I looked for tests of that case and could not find any, so I wrote a test for when the destination is null. The tests pass without changing any code so functionally there is no bug. I suppose an argument could be made for some optimization, to test $destination early in the function, log a message and exit. If that is done this is still not a bug, so changing to a task.
Comment #21
quietone CreditAttribution: quietone as a volunteer commentedIgnore that patch.
Comment #22
kim.pepper#3223209: deprecate file_save_data, file_copy and file_move and replace with a service makes $destination a required parameter, so this should resolve the issue.
Comment #23
andypostComment #24
phenaproximaPer #1048114: file_copy(), $destination cannot be NULL, I've transferred credit to that issue and am closing this one out as a duplicate.