[D7] file_unmanaged_move() should issue rename() where possible instead of copy() & unlink()

Uploading a patch exactly similar to the one committed to D8.

RTBC - looks good to me.

1. +++ b/includes/file.inc
   @@ -888,18 +887,66 @@ function file_valid_uri($uri) {
   +  $real_source = drupal_realpath($source) ?: $source;
   +  $real_destination = drupal_realpath($destination) ?: $destination;
   

   We can't use the short ternary.

2. +++ b/includes/file.inc
   @@ -888,18 +887,66 @@ function file_valid_uri($uri) {
   -    drupal_set_message(t('The specified file %file could not be copied, because no file by that name exists. Please check that you supplied the correct filename.', array('%file' => $original_source)), 'error');
   +    drupal_set_message(t('The specified file %file could not be moved/copied, because no file by that name exists. Please check that you supplied the correct filename.', array('%file' => $original_source)), 'error');
   ...
   -      watchdog('file', 'File %file (%realpath) could not be copied because it does not exist.', array('%file' => $original_source, '%realpath' => $realpath));
   +      watchdog('file', 'File %file (%realpath) could not be moved/copied because it does not exist.', array('%file' => $original_source, '%realpath' => $realpath));
   

   Are we ok with these string changes?

Fixed short ternary.

Back to RTBC, thanks for fixing

Looking at this again with my core committer head on, I think we should revert the string changes.

While it is technically more correct, we should avoid changing strings as much as possible in D7.

The rest still looks good to me.

Per #8, reverted string changes.

The last submitted patch, 9: drupal-file_unmanaged_move_rename_where_possible-2752783-9.patch, failed testing.

This looks great to me now.

Assigning to David for review as this is base system core functionality.

1. +    watchdog('file', 'The specified file %file could not be copied to %destination.', array('%file' => $source, '%destination' => $destination));
   ....
   -      watchdog('file', 'The specified file %file could not be copied to %destination.', array('%file' => $source, '%destination' => $destination), WATCHDOG_ERROR);
   

   Why is WATCHDOG_ERROR being removed here?

2. +      watchdog('file', 'The specified file %file could not be moved to %destination.', array('%file' => $source, '%destination' => $destination));
   

   Similarly, this should probably be WATCHDOG_ERROR also, to match the above (and to match Drupal 8).

3.  function file_unmanaged_move($source, $destination = NULL, $replace = FILE_EXISTS_RENAME) {
   .....
   +  // Ensure compatibility with Windows.
   +  // @see drupal_unlink()
   +  if ((substr(PHP_OS, 0, 3) == 'WIN') && (!file_stream_wrapper_valid_scheme(file_uri_scheme($source)))) {
   +    chmod($source, 0600);
   

   I am unclear on why we'd want to make a file writable before moving it. (Currently this code only runs on a file which is about to be deleted, not renamed, right?) I would think we'd want to attempt a rename based on the current file permissions only?

4. - *   support streams if safe_mode or open_basedir are enabled. See
   - *   https://bugs.php.net/bug.php?id=60456
   + * - Works around a PHP bug where copy() does not properly support streams if
   + *   safe_mode or open_basedir are enabled.
   + *   @see https://bugs.php.net/bug.php:id=60456
   

   "See" is correct here, not "@see", since it's an inline code comment. (Using @see would cause api.drupal.org not to display it.) There are other places in the patch that have a similar problem, and it looks like Drupal 8 does also.

5. + * @param $destination
   + *   A URI containing the destination that $source should be moved/copied to.
   + *   The URI may be a bare filepath (without a scheme) and in that case the
   + *   default scheme (file://) will be used. If this value is omitted, Drupal's
   + *   default files scheme will be used, usually "public://".
   

   I don't think the comment about "default scheme (file://)" adds much here, and it's actually confusing when compared with "Drupal's default files scheme" in the next sentence. Also, the patch is introducing that comment about file:// for file_unmanaged_move() and file_unmanaged_prepare(), but not introducing it for file_unmanaged_copy(). I would expect the documentation to be consistent between all three functions.

6. + * Internal function that prepares the destination for a file_unmanaged_copy or
   + * file_unmanaged_move operation.
   + *
   + * - Checks if $source and $destination are valid and readable/writable.
   + * - Checks that $source is not equal to $destination; if they are an error
   + *   is reported.
   + * - If file already exists in $destination either the call will error out,
   + *   replace the file or rename the file based on the $replace parameter.
   

   Ideally, the first line of the docblock should be 80 characters or less and fit on one line ( https://www.drupal.org/docs/develop/coding-standards/api-documentation-a... → ).

   Also, the list of bullets should probably have something on the line in front of it, for example "This function:".

7.   * Moves a file to a new location and update the file's database entry.
     *
   - * Moving a file is performed by copying the file to the new location and then
   - * deleting the original.
     * - Checks if $source and $destination are valid and readable/writable.
     * - Performs a file move if $source is not equal to $destination.
     * - If file already exists in $destination either the call will error out,
   

   Similarly, this should probably have something like "This function:" on the line in front of the bullets.

Overall, the patch looks like it's potentially a very nice improvement; could maybe use a review by some file system experts since this is somewhat of a big change? I'm also wondering if anyone has tested it in a real-world Drupal 7 environment, in particular some more unusual circumstances (moving files between different schemes, especially local vs. remote; on Windows; etc)...

re-rolled #9 for D7 head (7.54) as the previous patch doesn't apply.

interdiff is pretty hard to interpret, as the patch had to be manually applied, and the changes are resulted in just different line numbers.

I have not extensively tested the patch yet.

Thanks for the reroll - I think the previous patch applied with the patch command when I tested it, but probably not with git apply since there was some fuzz.

This still needs work for the comments in #13.

Attached is a patch that addresses 1 and 2 of #2752783-13: [D7] file_unmanaged_move() should issue rename() where possible instead of copy() & unlink() → .

@David_Rothstein, all of 3-7 are in what was committed in the D8 commit: http://cgit.drupalcode.org/drupal/commit/?id=26cf55b

Regarding 3, the explanation for that is in these comments:

• #1377740-5: file_unmanaged_move() should issue rename() where possible instead of copy() & unlink() →
• #1377740-10: file_unmanaged_move() should issue rename() where possible instead of copy() & unlink() →
• #1377740-21: file_unmanaged_move() should issue rename() where possible instead of copy() & unlink() → together with #1377740-22: file_unmanaged_move() should issue rename() where possible instead of copy() & unlink() →

Regarding 4-7, considering that's the way they went into D8, did you want them changed anyway? I agree that there is much in those comments that could be improved, but I'm not sure I would change them without also changing them in D8.

The last submitted patch, 17: interdiff-2752783-14-17.patch, failed testing. View results →

The last submitted patch, 17: drupal-file_unmanaged_move-rename-where-possible-2752783-17.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 17: drupal-file_unmanaged_move-rename-where-possible-2752783-17.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The test that failed previously was passing for me locally, but I started a complete test suite to see if anything different comes up. I also triggered the test bot here for multiple environments, in case the problem is with some unrelated PHP 7 incompatibility causing the failure.

Hey all, have there been any updates on this?

Patch looks good to go and #13 has been addressed.

Bumping to 7.70, this didn't make it into 7.60

A similar solution was put into D8 a while back now

Will this ever get committed?

Drupal core 7.71 is due to be released soon, perhaps this can get in as well. Thanks

@joseph.olstad I hope so...

Please @mcdruid add this patch to next release.

Please can this be committed? I have to patch after every update!

D8 has this already. It is a good idea.

It's already on the priority list #3207851: [meta] Priorities for 2021-06-02 release of Drupal 7 → , we don't need to update the labels anymore.

Let's try using meta issues to list the priority issues for upcoming bugfix / maintenance releases of D7.

Using the e.g. "Drupal 7.74 target" tags frequently gets messed up by security releases, and it's generally harder to keep track.

From #3179845: [meta] Priorities for 2020-12-02 bugfix release of Drupal 7.76 / 7.77 → .

Queued patch in #17 for additional testing with php 7.4 and php 8.0