Define FILE_STATUS_TEMPORARY and expose $status in file_save_data

I like the idea. Some people could complain that there are not so many use-cases that this does not make sense, but I believe it does no harm to include status as an argument.

I definitely support re-introduction of FILE_STATUS_TEMPORARY. Let's wait a little bit if any other arguments appear before we RTBC.

#10: drupal--1659116--define-FILE_STATUS_TEMPORARY-and-expose-status-in-file_save_data-10.patch queued for re-testing.

+++ b/core/modules/file/file.moduleundefined
@@ -525,8 +531,8 @@ function file_save_data($data, $destination = NULL, $replace = FILE_EXISTS_RENAM
-      'uid' => $user->id(),
-      'status' => FILE_STATUS_PERMANENT,
+      'uid' => $user->uid,

Re-roll incorrectly changes $user->id() back.

+++ b/core/includes/file.incundefined
@@ -126,12 +126,18 @@
  */
+const FILE_STATUS_TEMPORARY = 0;
+
+/**
+ * Indicates that the file is permanent and should not be deleted.
+ */

Would it make sense to FILE_STATUS_TEMPORARY and FILE_STATUS_PERMANENT into File entity class as class constants?

+1 for back-port. This shouldn't brake any APIs.

FileInterface actually. And yes, but that's a API change.

@Berdir: Are you talking about moving constants to an interface or backporting to D7?

About the constants. Agreed that this can be backported, but it is a feature, so getting it in core and backporting it is going to take... a long time ;)

Should we leave this constants as they are then?

Looks good. Maybe we can open a follow-up to move constants to the interface?

+++ b/core/modules/file/file.module
@@ -509,13 +509,19 @@ function file_validate_image_resolution(File $file, $maximum_dimensions = 0, $mi
+function file_save_data($data, $destination = NULL, $replace = FILE_EXISTS_RENAME, $status = FILE_STATUS_PERMANENT) {

Missing test coverage of calling this with FILE_STATUS _TEMPORARY

+++ b/core/modules/file/src/Tests/SaveDataTest.php
@@ -67,6 +67,29 @@ function testWithFilename() {
+  function testWithFilenameTemporary() {
+    $contents = $this->randomName(8);

@amontero the test fails because $this->randomName(8) not exists. Probably you should use $this->randomMachineName(8)

Hi @amontero thanks for work on this. Unfortunately we're in beta phase for Drupal 8 which means we're not accepting any new features. See https://www.drupal.org/core/beta-changes.

Looking at the patch two thoughts spring to mind firstly contrib can create temporary file entities by just creating a temporary and a file entity directly and it'd be possible to add support for this in a non bc breaking manner so I'm going to postpone to 8.1

Need this for D7.

Reopening this since 8.1 was release a long time ago.

Reroll of #51

+++ b/core/modules/file/tests/src/Kernel/SaveDataTest.php
@@ -58,6 +58,29 @@ public function testWithFilename() {
+    $this->assertEqual($result->getMimeType(), 'text/plain', 'A MIME type was set.');

Please use assertEquals here. Please also keep in mind that on the left side of the equation you want to have the expected value.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.