The DRUPAL_MAXIMUM_TEMP_FILE_AGE constant seems too short to handle nodes with large file attachments (audio/video etc.). It's a constant set to 24 minutes. Tell me if I'm wrong but it looks like nodes can have their file attachments deleted before the node is saved, if the process of uploading, previewing, etc. takes more than 24 minutes.

With 100MB of attachments on a node it could easily be an hour long (or more) process to upload files and save the node.

Is there any reason System module is so aggressive about garbage collection of temporary files?

Dries' suggestion at http://drupal.org/node/115267#comment-503595 was to remove the setting from the patch and make it a define. This issue could be resolved by either swapping it back for a setting, or increasing the constant to something like 2 or 3 hours, which should be enough for most sites. Although there certainly can be exceptions here..

CommentFileSizeAuthor
#8 d7-MAXIMUM_TEMP_FILE_AGE.patch651 bytesmfb
#3 maximum_temp_file_age.patch1.4 KBmfb
#1 maximum_temp_file_age.patch601 bytesmfb

Comments

mfb’s picture

mfb
they or he
commented
Version: 6.2 » 7.x-dev
Status: Active » Needs review
StatusFileSize
new maximum_temp_file_age.patch601 bytes

Here's a suggested fix, increasing the maximum temp file age from 24 minutes to 4 hours. Attached patch is for HEAD.

R.Muilwijk’s picture

R.Muilwijk commented
Status: Needs review » Needs work

Most of the sites wouldn't want the temp file age to be that long. The variable is also just used on 2 places. The declaration and the query:

grep -R DRUPAL_MAXIMUM_TEMP_FILE_AGE *
modules/system/system.module:define('DRUPAL_MAXIMUM_TEMP_FILE_AGE', 1440);
modules/system/system.module:  // Remove temporary files that are older than DRUPAL_MAXIMUM_TEMP_FILE_AGE.
modules/system/system.module:  $result = db_query('SELECT * FROM {files} WHERE status = %d and timestamp < %d', FILE_STATUS_TEMPORARY, time() - DRUPAL_MAXIMUM_TEMP_FILE_AGE);

Why not make a variable of it so people can adjust to their needs with a default of 24 minutes? Though it would be nice to add some documentation of why 24 minutes.

mfb’s picture

mfb
they or he
commented
Status: Needs work » Needs review
StatusFileSize
new maximum_temp_file_age.patch1.4 KB

I don't have a strong preference for either solution. Although if there is some reason why some sites would prefer only 24 minute life for temporary files then I suppose it should be a variable, since I definitely need a couple hours to handle more than one large file attachment.

mfb’s picture

mfb
they or he
commented

If forms are cached for six hours (21600 seconds), then why not allow temporary files to also live for up to six hours? Or at least four hours as in my patch above.

drewish’s picture

drewish commented

if you look at dopry's early patches before the constant was added, it was 14400 (4 hours). i'm guessing that the zero got dropped during the copy-paste to the constant.

update: whoops... looking at the next patch it's pretty clear that i'm the one that dropped the zero.

drewish’s picture

drewish commented
Status: Needs review » Needs work

i think the timelimit needs to be fixed as part of this patch. i'm not sure it needs to be a variable though.

damien tournoud’s picture

damien tournoud commented

I like the idea of aligning that to the FormAPI cache cleaning.

mfb’s picture

mfb
they or he
commented
Status: Needs work » Needs review
StatusFileSize
new d7-MAXIMUM_TEMP_FILE_AGE.patch651 bytes

ok here's a 6 hour version... longer than I need but I guess theoretically someone could spend most of a day's work uploading attachments and previewing.. (it won't allow people to get 8 hours sleep while uploading attachments).

obsidiandesign’s picture

obsidiandesign commented

Patch applies cleanly. I agree that matching FormAPI's cache time is a good approach. SimpleTests run successfully. +1 for RTBC.

lilou’s picture

lilou commented
Status: Needs review » Reviewed & tested by the community
dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD and DRUPAL-6. Thanks! :)

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.