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.
file_move() should only be called on managed files. The problem is that the file and image fields call file_move() on a managed file object that has yet to be saved. This causes problems with contrib modules that implement hook_file_delete() because they expect $file->id() to exist. This was fixed in Devel Generate in 7.x-1.x with #2429787: File generation should use file_unmanaged_move() instead of file_move() before the file object is saved
Comment | File | Size | Author |
---|---|---|---|
#46 | 2550973-46.patch | 1.03 KB | Nitin shrivastava |
#38 | 2550973-37.patch | 2.84 KB | Medha Kumari |
#35 | interdiff-31-35.txt | 1.97 KB | smustgrave |
#35 | 2550973-35.patch | 2.84 KB | smustgrave |
Comments
Comment #2
Dave ReidSeems limited just to Image fields.
Comment #3
Dave ReidComment #4
Dave ReidI should just use $destination_dir as the second argument for file_unmanaged_move(). There's no need to construct the exact destination URL.
Comment #6
tkuldeep17 CreditAttribution: tkuldeep17 at Axelerant commented@dave patch is failed due to undefined variable `file`, so I have fixed it. and uploading patch.
Comment #8
tkuldeep17 CreditAttribution: tkuldeep17 at Axelerant commentedSorry, I missed to fix file variable at one place. Updating patch again.
Comment #12
yogeshmpawarRerolled the patch #8.
Comment #23
larowlanThose methods are now on the file repository service, so I suspect this needs a reroll
Comment #24
kim.pepperfile_unmanaged_move was deprecated in Drupal 8.7.0. See https://www.drupal.org/node/3006851
Comment #25
kim.pepperComment #26
immaculatexavier CreditAttribution: immaculatexavier as a volunteer commentedComment #27
immaculatexavier CreditAttribution: immaculatexavier as a volunteer commentedAddressed #23,#24. Rerolled against #12. Attached reroll diff for the same.
Comment #28
immaculatexavier CreditAttribution: immaculatexavier as a volunteer commentedMissed to upload patch in #27
Addressed #23,#24. Rerolled against #12. Attached reroll diff for the same.
Comment #29
smustgrave CreditAttribution: smustgrave at Mobomo commentedFixed #28
Comment #31
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #33
smustgrave CreditAttribution: smustgrave at Mobomo commentedSo I removed this if statement
if ($path = \Drupal::service('file.repository')->move($path, $destination)) {
because $path couldn't be passed into move(). Wondering if that was the right approach?
Comment #34
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #35
smustgrave CreditAttribution: smustgrave at Mobomo commentedForgot to check commit before generating patch.
Comment #36
smustgrave CreditAttribution: smustgrave at Mobomo commentedJust realized this has no tests.
Comment #37
Medha KumariComment #38
Medha KumariReroll the patch #35 with Drupal 9.5.x
Comment #39
smustgrave CreditAttribution: smustgrave at Mobomo commentedStill no test case. Nor an interdiff so a reroll doesn’t help in this case.
Comment #40
smustgrave CreditAttribution: smustgrave at Mobomo commentedBrought this up in the bugsmash slack channel it seems more like an enhancement than a bug. Moving to Feature request.
Sorry for the confusion.
Comment #42
larowlanCan we simplify a lot of this by using the file.repository service's writeData method?
\Drupal\Tests\image\Kernel\ImageItemTest::testImageItem has test coverage for this method, so can we expand that to add the missing coverage?
Comment #43
SandeepSingh199 CreditAttribution: SandeepSingh199 as a volunteer commentedWith the help of #42, I have created the patch for D10.1.x. Please check & review.
Comment #44
Akram KhanFix CCF #43
Comment #45
smustgrave CreditAttribution: smustgrave at Mobomo commentedThank you for the patches but tests were not added.
No interdiff in 43
And you can check the patch will apply using core scrips adjusting credit
Comment #46
Nitin shrivastava CreditAttribution: Nitin shrivastava at OpenSense Labs commentedtry to fix #44 ccf error.