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
See #3034072: Move file uri/scheme functions from file.inc and FileSystem to StreamWrapperManager.
Removing support for that means we don't have a hidden dependency on the configuration system in FileSystem and an implicit copy/move into the top-level folder of the default scheme seems like way too much implicity to support, you should never copy things directly into that folder.
Proposed resolution
Make the arguments required.
Remaining tasks
After commit: Update https://www.drupal.org/node/3006851, with a similar sentence as the release note snippet.
User interface changes
API changes
This is an API change, but the methods only exist since 8.7, so it is an allowed change as long as we are still in alpha.
Data model changes
Release notes snippet
The destination parameter of the methods FileSystem::copy(), FileSystem::move() and FileSystem::saveData() is now required, this is an API changed compared to 8.7.0-alpha2. 'public://' or file_default_scheme() . '://' can be used as a replacement to get the same behavior but it is discouraged to copy files directly into the top-level directory.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 3038988-13.patch | 17.7 KB | seanB |
| #13 | interdiff-7-13.txt | 3.76 KB | seanB |
| #7 | require-destination-3038988-7-interdiff.txt | 10.09 KB | Berdir |
| #7 | require-destination-3038988-7.patch | 13.93 KB | Berdir |
| #2 | require-destination-3038988-2.patch | 4.31 KB | Berdir |
Comments
Comment #2
BerdirIf we can still get that into 8.7 then we don't need another BC layer, except updating the existing change record to mention this.
Comment #3
BerdirAh, what it will need is a BC layer for that in file_unmanaged_copy()/move().
Comment #5
kim.pepper+1 on this idea
Comment #6
andypost+1 here, if it doesn't fit in 8.7 it should be done in 8.8
Guess proper tag...
Comment #7
BerdirFixed those test fails I think, added BC to the old functions and updated the BC tests to make sure this still works.
Also, looks like very very few cases in core relied on this, the only one outside of tests were those theme files.
Comment #8
dww- This is a weird aspect of the file API that's being fixed here. There's no good reason to keep supporting this behavior. Callers should have to specify where they want their files to be moved or copied to. Relying on default directories and random names is a WTF, a potential for bugs, etc. I didn't super carefully audit this, but I wouldn't be surprised if there were race conditions lurking under this mess.
- Removes a hidden dependency and makes things better for other cleanup issues.
- The BC layer still supports the weirdness, so there's no break in the D8 world. There are tests to prove it.
- All tests are green.
- Code is clean.
Hence, RTBC.
However, tagging for a CR update, since it seems like we should mention that
$destinationis now required over at https://www.drupal.org/node/3006851 (right?)Thanks!
-Derek
p.s. Note: this sort of conflicts with #3039026: Deprecate file_directory_temp() and move to FileSystem service since we're calling
file_default_scheme()in here. Whichever patch lands 2nd will need a re-roll.Comment #9
kim.pepperI've postponed #3039026: Deprecate file_directory_temp() and move to FileSystem service on this.
Comment #10
kim.pepper+1 RTBC
Comment #12
seanBI'll take a look at the failing media test tonight!
Comment #13
seanBFixed the media library test!
Comment #14
BerdirThanks!
Comment #15
dww+1 to RTBC based on interdiff #13. Thanks!
Comment #16
larowlanThis makes sense to me
Comment #17
alexpott+1 this looks very sensible.
Comment #18
xjmComment #19
xjmd.o ate my comment... It's a good idea to fix this before beta1 rather than deprecating in 8.8.x, but we need to update the CR, add a CR for this change itself from alpha1, and also add a release note to inform site owners on one of the alphas that this break happened. Thanks!
Comment #20
BerdirUpdated the issue summary.
AFAIK we only update published change records once a change is committed, the related CR is https://www.drupal.org/node/3006851 and I'll update that as soon as it is committed, the CR already doesn't indicate that those parameters are optional and I'll add a sentence similar to the relesae note snippet there.
Comment #23
larowlanCommitted d1db7c7 and pushed to 8.8.x. Thanks!
C/p as 050dcd03e6 and pushed to 8.7.x
Added a standalone C/r for the post alpha changes at - https://www.drupal.org/node/3042234
Added the release notes section to the previous change record.
Unpostponed issues blocked on this.
Comment #24
dwwCrediting reviewers (including myself). Hope that's cool.
Thanks, everyone!
-Derek