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

CommentFileSizeAuthor
#46 2550973-46.patch1.03 KBnitin shrivastava
#44 reroll_diff_43-44.txt920 bytesakram khan
#44 2550973-44.patch1.03 KBakram khan
#43 2550973-43.patch1.04 KBsandeepsingh199
#38 2550973-37.patch2.84 KBmedha kumari
#35 interdiff-31-35.txt1.97 KBsmustgrave
#35 2550973-35.patch2.84 KBsmustgrave
#34 interdiff-31-34.txt2.35 KBsmustgrave
#34 2550973-34.patch2.23 KBsmustgrave
#31 interdiff-29-31.txt1.17 KBsmustgrave
#31 2550973-31.patch2.99 KBsmustgrave
#29 2550973-29.patch3.14 KBsmustgrave
#28 reroll_diff_12-28.txt3.81 KBimmaculatexavier
#28 2550973-28.patch2.87 KBimmaculatexavier
#12 image_generate_unmanaged_move-2550973-12.patch2.41 KByogeshmpawar
#8 2550973-8-image-generate-unmanaged-move.patch2.41 KBtkuldeep17
#6 2550973-6-image-generate-unmanaged-move.patch2.31 KBtkuldeep17
#2 2550973-image-generate-unmanaged-move.patch1.64 KBdave reid

Comments

Dave Reid created an issue. See original summary.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Title: File and image fields' generateSampleValue() should use file_unmanaged_move() before calling $file->save(). » ImageItem::generateSampleValue() should use file_unmanaged_move() before saving the file
Status: Active » Needs review
StatusFileSize
new 2550973-image-generate-unmanaged-move.patch1.64 KB

Seems limited just to Image fields.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Component: file system » image.module
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented 
+++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
@@ -341,16 +341,20 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
         $destination = $destination_dir . '/' . basename($path);
-        $file = file_move($image, $destination, FILE_CREATE_DIRECTORY);
-        $images[$extension][$min_resolution][$max_resolution][$file->id()] = $file;
+        if ($path = file_unmanaged_move($path, $destination)) {

I should just use $destination_dir as the second argument for file_unmanaged_move(). There's no need to construct the exact destination URL.

Status: Needs review » Needs work

The last submitted patch, 2: 2550973-image-generate-unmanaged-move.patch, failed testing.

tkuldeep17’s picture

tkuldeep17 commented
Status: Needs work » Needs review
StatusFileSize
new 2550973-6-image-generate-unmanaged-move.patch2.31 KB

@dave patch is failed due to undefined variable `file`, so I have fixed it. and uploading patch.

Status: Needs review » Needs work

The last submitted patch, 6: 2550973-6-image-generate-unmanaged-move.patch, failed testing.

tkuldeep17’s picture

tkuldeep17 commented
Status: Needs work » Needs review
StatusFileSize
new 2550973-8-image-generate-unmanaged-move.patch2.41 KB

Sorry, I missed to fix file variable at one place. Updating patch again.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Needs review » Needs work

The last submitted patch, 8: 2550973-8-image-generate-unmanaged-move.patch, failed testing.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Status: Needs work » Needs review
StatusFileSize
new image_generate_unmanaged_move-2550973-12.patch2.41 KB

Rerolled the patch #8.

Status: Needs review » Needs work

The last submitted patch, 12: image_generate_unmanaged_move-2550973-12.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: dave reid » Unassigned
Priority: Normal » Minor
Issue tags: +Needs reroll, +Bug Smash Initiative

Those methods are now on the file repository service, so I suspect this needs a reroll

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Assigned: Unassigned » dave reid
Priority: Minor » Normal

file_unmanaged_move was deprecated in Drupal 8.7.0. See https://www.drupal.org/node/3006851

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Assigned: dave reid » Unassigned
immaculatexavier’s picture

immaculatexavier commented
Assigned: Unassigned » immaculatexavier
immaculatexavier’s picture

immaculatexavier commented
Assigned: immaculatexavier » Unassigned
Status: Needs work » Needs review

Addressed #23,#24. Rerolled against #12. Attached reroll diff for the same.

immaculatexavier’s picture

immaculatexavier commented
StatusFileSize
new 2550973-28.patch2.87 KB
new reroll_diff_12-28.txt3.81 KB

Missed to upload patch in #27
Addressed #23,#24. Rerolled against #12. Attached reroll diff for the same.

smustgrave’s picture

smustgrave commented
StatusFileSize
new 2550973-29.patch3.14 KB

Fixed #28

Status: Needs review » Needs work

The last submitted patch, 29: 2550973-29.patch, failed testing. View results

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review
StatusFileSize
new 2550973-31.patch2.99 KB
new interdiff-29-31.txt1.17 KB

Status: Needs review » Needs work

The last submitted patch, 31: 2550973-31.patch, failed testing. View results

smustgrave’s picture

smustgrave commented

So 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?

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review
StatusFileSize
new 2550973-34.patch2.23 KB
new interdiff-31-34.txt2.35 KB
smustgrave’s picture

smustgrave commented
StatusFileSize
new 2550973-35.patch2.84 KB
new interdiff-31-35.txt1.97 KB

Forgot to check commit before generating patch.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Just realized this has no tests.

medha kumari’s picture

medha kumari
Location Jaipur
commented
Assigned: Unassigned » medha kumari
medha kumari’s picture

medha kumari
Location Jaipur
commented
Version: 9.4.x-dev » 9.5.x-dev
Assigned: medha kumari » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2550973-37.patch2.84 KB

Reroll the patch #35 with Drupal 9.5.x

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Still no test case. Nor an interdiff so a reroll doesn’t help in this case.

smustgrave’s picture

smustgrave commented
Category: Bug report » Feature request
Status: Needs work » Needs review

Brought this up in the bugsmash slack channel it seems more like an enhancement than a bug. Moving to Feature request.

Sorry for the confusion.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Needs work
Issue tags: +Needs tests 
+++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
@@ -349,23 +348,21 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
+        if ($path = \Drupal::service('file_system')->move($path, $destination)) {
+          $image = File::create([
+            'uri' => $path,
+            'uid' => \Drupal::currentUser()->id(),
+          ]);
+          $image->save();
+          $images[$extension][$min_resolution][$max_resolution][$image->id()] = $image;
+          $file = \Drupal::service('file.repository')->move($image, $destination);

Can 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?

sandeepsingh199’s picture

sandeepsingh199 commented
Status: Needs work » Needs review
StatusFileSize
new 2550973-43.patch1.04 KB

With the help of #42, I have created the patch for D10.1.x. Please check & review.

akram khan’s picture

akram khan
Primary language English
Location Cuttack, Odisha
commented
StatusFileSize
new 2550973-44.patch1.03 KB
new reroll_diff_43-44.txt920 bytes

Fix CCF #43

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Thank 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

nitin shrivastava’s picture

nitin shrivastava commented
StatusFileSize
new 2550973-46.patch1.03 KB

try to fix #44 ccf error.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.