File and image fields are treated as temporary files and automatically deleted after six hours

Yikes! That sounds like a terrible problem. We should be updating the status to FILE_STATUS_PERMANENT on file_field_update(). Unfortunately the file_set_status() function was removed entirely from Drupal 7, so we don't have any API to actually do this query.

#331013: Remove file_set_status() in favor of file_save() seems to suggest that $file->status should be set manually:

file_set_status() was added in Drupal 6 as a way of changing a temporary file to a permanent file back before file_save() was added. In it's current implementation the function has an annoying overlap with the functionality provided by file_save().

All file_set_status() does is changes the status field of a row in the {files} table and call hook_file_status. But there's nothing to stop you from assigning the file's status and then calling file_save() which would circumvent the call to hook_file_status.

It also diverges from the patterns established else where in core. We don't have a node_set_status() function, you just do a $node->status = 1; node_save($node); and I think the same should be true for files.

According to the docs for file_save_upload(),

The file will be added to the {file} table as a temporary file. Temporary files are periodically cleaned. To make the file a permanent file, assign the status and use file_save() to save the changes.

I think this means that we should call file_save_upload(), set the status of the returned $file object to 1, and then call file_save() when the node is actually submitted.

Patch attached which does a very rough version of what I've described in #3 - it imitates what is done in Drupal 6. This is rough and there must be a better way to do it, but it does work and might get the ball rolling towards a more elegant solution.

Thanks for your work on this justinrandell. From the patch in #8, it looks like this will only mark files as permanent on new nodes (when is_new is TRUE), not when updating existing nodes and adding additional files.

Now if a new revision is made, new files will not be made permanent. So if you upload a file, then make a new revision on that edit, your new file will not be made permanent. It looks like we don't need to modify the is_new or revision IF statement at all, we just need to put the new foreach loop in there before the existing IF.

Crosslinking to #644338: file_field_update() triggers a full entity load durung entity_save(), since this patch seems to touch this exact area.

+ The discussion here is largely over my head, but if stuff needs to be done on both entity creation and entity update, maybe hook_field_presave() would be more appropriate ?
Sorry for the noise if this doesn't apply or has already been turned down ;-).

This is a nasty, horrible bug - giving it a better title :)

I'm pretty far out of my element here also, but after looking into it for a while, @yched's suggestion of using hook_field_presave() seems to make sense. If anyone has a reason why that's a bad idea, please explain!

The attached patch does this. I'm putting it at "needs review", but it definitely still needs tests. Hopefully I can work on those later.

subscribe

This seems okay to me, but why don't we just clear anything with uri LIKE "temporary://%" every X hours where X is a variable defined by the administrator?

Also, another thought: The default should be non-temporary, shouldn't it? Imagine people doing mass imports of files or grabbing them from URLs, etc

I have created a node type, added an image field and then uploaded a file. Status was 0. Saved the node. Status is now 1. Seems like the patch is working. That's what the test would need to do. However, we survived without a file.test so far and this issue is so crippling that I daresay test is another issue (or this issue after the initial commit). I think the only upload test we had was the defunct upload.test.

See attached with an updated "file field revisions test" to test permanence. The six new assertions fail without the rest of the patch, so this is a valid test.

C'mon testbot!

Better patch with a real description for the new method on FileFieldTestCase. (Duh.)

Code works as as intended. As a follow-up we should add more image field tests.

Some of that follow-up work is starting at #709904: Image field does not delete unused files where I'm looking at inconsistencies between file and image fields with regard to files that are no longer in use by a node.

Well this is truly a horrifying bug, indeed. :) Couple of things:

1. Why on earth is the default behaviour of a function called file_save_upload() to delete the files after 6 hours? Shouldn't rather the default behaviour be to save them permanently, and a module have to do special back-flips to get them to be temporary instead?

2. How will we not have exactly this same bug cropping up constantly from video module, media module, emfield module, and any other module that deals with file uploads? Will they all have to add a hook_field_presave() to prevent massive data-loss, too? That seems like a huge, gaping flaw in our file API.

In other words, I would've expected to see file.inc hunks in this patch, not modules/file and modules/image hunks. What gives?

I'm out of time for tonight on this one. Here's as far as I got (keep in mind that I'm still a D7 n00b.)

I had added a bit of logic in file_save_upload() to do the following before its call to file_save():

  // Save the file as permanent if it's not being moved to the temporary://
  // scheme.
  if (file_uri_scheme($file->uri) != 'temporary') {
    $file->status |= FILE_STATUS_PERMANENT;
  }

It seems innocuous enough, yet it causes plenty of fails in the file tests. One major area where it clearly falls over is file_managed_file_validate(), since the validator expects a file to not yet be permament when validated, or if it is permament, to have more than 0 references to it, lest you trigger the "Referencing to the file used in the image field is not allowed." The offending validation code is

/**
 * An #element_validate callback for the managed_file element.
 */
function file_managed_file_validate(&$element, &$form_state) {
  // If referencing an existing file, only allow if there are existing
  // references. This prevents unmanaged files from being deleted if this
  // item were to be deleted.
  $clicked_button = end($form_state['clicked_button']['#parents']);
  if ($clicked_button != 'remove_button' && !empty($element['fid']['#value'])) {
    if ($file = file_load($element['fid']['#value'])) {
      if ($file->status == FILE_STATUS_PERMANENT) {
        $reference_count = 0;
        foreach (module_invoke_all('file_references', $file) as $module => $references) {
          $reference_count += $references;
        }
        if ($reference_count == 0) {
          form_error($element, t('Referencing to the file used in the !name field is not allowed.', array('!name' => $element['#title'])));
        }
      }
    }
  // SNIPPED
  }
}

Here's the change above as a patch if you really want it. It won't pass tests.

@webchick
1. I think its this way, so if you ajax upload an image or hit preview on a form with a filefield, but never fully submit it, those files that are ultimately never used get cleaned up automatically. Not sure how you'd stop that if it wasn't this way, or are you just suggesting a name change to make it less confusing?

@webchick - that behavior is directly ported from D6

#22 should be committed asap. Yes, the logic is stupid, but it matches what's in Drupal 6 and we need at least to avoid a total #fail in core before further debating API changes.

#21: file-save-temp-618654-21.patch queued for re-testing.

If #21 doesn't pass, I can easily bzr up and re-roll.

#21 is RTBC. This is how Drupal 6 worked and until we find something better which I bet we can't within the confines of Drupal 7 we need to fix this horror now.

I'd like to get quicksketch to eyeball this first. He's the maintainer of filefield and also worked a lot on Drupal 7's file API, and was advocating a more "central" fix earlier, which makes a lot more sense to me.

But yes, agreed on the nastiness and that we urgently need a fix. I've sent him a mail about it.

I believe he was mainly advocating that the file_set_status() API function be added back to Drupal 7, which sounds like a good idea... the code involving the & and |= operators in the patch is a little crazy to expect people to copy around correctly all over the place. I thought it was a bit out of scope for this bugfix, but maybe not.

+function file_field_presave($obj_type, $object, $field, $instance, $langcode, &$items) {
+  // Make sure that each file which will be saved with this object has a
+  // permanent status, so that it will not be removed when temporary files are
+  // cleaned up.

There is obviously a lot of confusion around this, so we should probably expand the code comment to explain why we are saving things in a "presave" hook - Dries was confused about that when looking at this patch although he did not comment here :)

Basically, @meatsack's explanation in #27 is correct; in common use cases, the file itself needs to be saved before the filefield that points to it, so we only make that file permanent when the filefield itself is ready to save.

Everything looks good to me. #21 is exactly the implementation I'd pursue to fix this same problem. While I don't necessary think the *default* behavior should be to delete files after 6 hours, it makes a good amount of sense in the use-cases we use within file and image modules. For example:

- User goes to node/add/article.
- User uploads new file.
- User closes window and never saves the node.

In this situation we definitely want the file to marked as temporary so it will be removed after 6 hours. At the same time we also want the file to be uploaded to the final location because if the user wants to insert it into a WYSIWYG or have it on preview, the image should be in the final location so we don't have to do any ridiculous URL rewriting or pass-throughs like we had in Drupal 5 (which never worked well).

In the alternative situation, where the file is saved to a temporary directory initially when it's not needed/wanted immediately (such as if you're replacing the site logo), then temporary also makes sense. The file is moved to it's final location and marked permanent upon submission of the form.

Well, then. :) #21 it is.

Committed to HEAD. Can I get a follow-up patch for the comment clarifications?

I believe he was mainly advocating that the file_set_status() API function be added back to Drupal 7, which sounds like a good idea...

Right, at the time I was wondering what had happened to the function, but since it only requires the same number of queries as file_save() (minus hook_* queries added by other modules), it makes sense to drop the function. I just didn't see a direct equivalent initially.

we should probably expand the code comment to explain why we are saving things in a "presave" hook

Just picking up this issue tonight, it took me a second to think of what I'd do to fix the problem, but I quickly settled on presave even before I looked at the patch in #21. It makes sense to me. It's easy and direct. Oddly though we'll mark a file "permanent" in presave then delete it in "update" if the file is marked for deletion. To prevent this we could add one more check to not save a file as permanent if $item['delete'] is TRUE.

Crosspost. Maybe we can add the $item['delete'] check in the followup too then. ;-)

did anyone make a follow up?

There's a remaining reference to $form_state['clicked_button'] (file.module:536), so here's a patch that replaces it with $form_state['triggering_element']. This cleans up a warning ("end() expects parameter 1 to be array, null given") I'm seeing with AJAX updates.

#42: 681654.triggering_element.patch queued for re-testing.

subscribe

Still an issue for me in Drupal core 7.x-dev as of February 2011.

This is still an issue for me too using Drupal Core 7.0.

1. Create a CT
2. Add image field
3. Upload default image
4. Save CT
5. Add content to CT
6. Sleep
7. Wake up
8. Repeat steps 3-8.

glowsinthedark: That's not surprising -- the issue was fixed after 7.0 was released. You'll need to use a 7.x-dev version or wait for 7.1.

Edit: I've forgotten we are in the year 2011 now... Sorry! Regardless, there is a patch in #42 so the status should be needs review.

@glowsinthedark: The bug you're describing is already being addressed at #1028092: Default image is not set to permanent and saved to the wrong schema. This issue was for user-uploaded files (not default ones) and was fixed a long time ago.

As far as I know, this is just open for the residual cleanups listed above... and needs review for the patch in #42, although I'm not quite sure I understand how that patch is related?

@bfroehle, @David_Rothstein, thanks and sorry for the status change! I'll look into the issue mentioned in #49. Thanks again!

subscribe

Subscribe. I really, really wish issue summaries worked now :-)

Is one of the above patches able to be applied to 7.0 so I can just get the default images to not be deleted after six hours? That's all I'm really hoping for, for now at least. I'd rather not switch to 7.x-dev for one of my sites.

following #52 question

I am not using .dev

I lost the "default image" for an image field a couple of times. After reading this thread I see that the status in "file_managed" was 0.

I have set this to 1, hoping it will stick around awhile.

@52 & 53, check comment 49, sounds like issue #1028092: Default image is not set to permanent and saved to the wrong schema is what you're both experiencing.

subscribe. Still an issue in 7.2.

subscribed, got it here too in 7.2

You need to use 7.x-dev until 7.3 comes out.

#42: 681654.triggering_element.patch queued for re-testing.

subscribe

subscribe

subscribe Thanks!

This is now fixed in the new D7.4 - yay!

See comment #49 :)

subscribe

#42: 681654.triggering_element.patch queued for re-testing.

This is still a problem for me with Drupal 7 HEAD.

Steps to reproduce:

1 - Install standard profile
2 - Create/config private files directory
3 - Add an Image field to the page content type. Use all default options except set to use Private files.
4 - Upload a default image after completely saving the field.

In the files_managed table the default image will have a status of 0 (temporary). Changing it to 1 results in 403 error when viewing default image.

A similar error happens when adding the default image at the first opportunity:

Steps 1, 2 same as above

3 - Add an Image field to the page content type. On the overlay (field settings), select private files and upload a default image. Click 'save field settings'.

The default image is now has a record in the files_managed table and it's status is set to 1 (permanent). But viewing the image results in a 403 error.

I just tested the first "Steps to reproduce: in #68 in D8 HEAD.

The only difference is that the default image file is saved with a permanent status but attempts to view the image from the field settings result in a 403. Changing the status to temporary allows the file to be viewed.

I wonder if I'm missing something when it comes to files settings?

i cant believe this issue just lies here for 6 years. d8 keeps deleting random images - wtf :o

Are you seeing exactly this problem? What version of Drupal are you working with?

@sic this issue got lost, it looks like the patch got committed here but the issue never actually got marked fixed.

However, there have been literally dozens of issues dealing with this and numerous fixes, while we eventually realised we need to overhaul how to deal with unused files altogether. The one that actually stops deletion is #2801777: Give users the option to prevent drupal from automatically marking unused files as temporary and it'll be released in 8.4.0. There are a number of other issues from the meta that cilefen linked to which will need follow-up. Going to mark this one as fixed because the specific aspect of the bug reported here was fixed, even though the wider problem has indeed continued for years - but it's well covered by the other issues mentioned.

thanks guys.

were using 8.3. i'll have a look into the linked issue. thanks!

I've taken over the development of a site that is using Drupal 8.3.7 and I'm having a number of issues with File fields that may all be related to what I've noticed. I'm getting duplicate entries in the file usage table with the same file marked as both permanent and temporary.

I'm not sure if this is a common problem but what I've found is that when you upload a file using a node edit form, ajax is used to upload the file. Whilst this is going on, the node save button is still enabled. If the node is saved before the file is fully uploaded, you can end up with duplicate entries in the file usage table. One marked as permanent with a usage count of 1 and the other as temporary with a usage count of 0.
When the "temporary" file is deleted of course the permanent file disappears although the file is still referenced.

There seems to be a possible fixes for this in that the node save button could be disabled until the file upload is complete.

Can anyone duplicate this behaviour?

I'm also getting possibly related issues such as links to the files (which are stored in private storage) returning access denied even to user 1 and the browser sometimes trying to open the files (spreadsheets) rather then downloading them. I'm guessing that these issues could also be caused by broken file storage values.

Edit: Just found this thread, so it seems that this could be a long running issue: https://www.drupal.org/node/610462

Note that the change in #42 already got added somewhere else along the way, and the other followups discussed above (code comments, etc) are probably not earth-shatteringly important enough that they are worth a followup issue for... so closing this issue is correct.

However, the patch was committed to Drupal 7 in February 2010 (!) so it was already part of the codebase before Drupal 8 was ever branched; therefore, for posterity, this should be a Drupal 7 issue rather than Drupal 8.