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
7.x backport for #1377740: file_unmanaged_move() should issue rename() where possible instead of copy() & unlink(). The code has not changed much and the bug is exactly the same.
Comments
Comment #2
Fabianx CreditAttribution: Fabianx for Drupal Association commentedComment #3
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedUploading a patch exactly similar to the one committed to D8.
Comment #4
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC - looks good to me.
Comment #5
twistor CreditAttribution: twistor as a volunteer commentedWe can't use the short ternary.
Are we ok with these string changes?
Comment #6
paulihuhtiniemi CreditAttribution: paulihuhtiniemi at Wunder commentedFixed short ternary.
Comment #7
Fabianx CreditAttribution: Fabianx as a volunteer commentedBack to RTBC, thanks for fixing
Comment #8
Fabianx CreditAttribution: Fabianx as a volunteer commentedLooking 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.
Comment #9
ckngPer #8, reverted string changes.
Comment #11
ckngComment #12
Fabianx CreditAttribution: Fabianx as a volunteer commentedThis looks great to me now.
Assigning to David for review as this is base system core functionality.
Comment #13
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedWhy is WATCHDOG_ERROR being removed here?
Similarly, this should probably be WATCHDOG_ERROR also, to match the above (and to match Drupal 8).
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?
"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.
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.
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:".
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)...
Comment #14
jaxxed CreditAttribution: jaxxed at Wunder commentedre-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.
Comment #15
yogeshmpawarComment #16
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThanks for the reroll - I think the previous patch applied with the
patch
command when I tested it, but probably not withgit apply
since there was some fuzz.This still needs work for the comments in #13.
Comment #17
oadaeh CreditAttribution: oadaeh at Hook 42 commentedAttached 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:
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.
Comment #21
oadaeh CreditAttribution: oadaeh at Hook 42 commentedThe 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.
Comment #22
Collins405 CreditAttribution: Collins405 commentedHey all, have there been any updates on this?
Comment #23
MustangGB CreditAttribution: MustangGB commentedPatch looks good to go and #13 has been addressed.
Comment #24
joseph.olstadBumping to 7.70, this didn't make it into 7.60
Comment #25
joseph.olstadA similar solution was put into D8 a while back now
Comment #26
joseph.olstadComment #27
mcdruidComment #28
joseph.olstadComment #29
joseph.olstadComment #30
sjerdoComment #31
MustangGB CreditAttribution: MustangGB commentedComment #32
MustangGB CreditAttribution: MustangGB commentedComment #33
anrikun CreditAttribution: anrikun commentedWill this ever get committed?
Comment #34
joseph.olstadDrupal core 7.71 is due to be released soon, perhaps this can get in as well. Thanks
Comment #35
anrikun CreditAttribution: anrikun commented@joseph.olstad I hope so...
Please @mcdruid add this patch to next release.
Comment #36
Collins405 CreditAttribution: Collins405 commentedPlease can this be committed? I have to patch after every update!
Comment #37
joseph.olstadD8 has this already. It is a good idea.
Comment #38
Collins405 CreditAttribution: Collins405 commentedComment #39
MustangGB CreditAttribution: MustangGB commentedIt'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.
Comment #40
ressa CreditAttribution: ressa at Ardea commentedFrom #3179845: [meta] Priorities for 2020-12-02 bugfix release of Drupal 7.76 / 7.77.
Comment #41
izmeez CreditAttribution: izmeez commentedQueued patch in #17 for additional testing with php 7.4 and php 8.0