DRUPAL_MAXIMUM_TEMP_FILE_AGE is too short
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | system.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
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..
#1
Here's a suggested fix, increasing the maximum temp file age from 24 minutes to 4 hours. Attached patch is for HEAD.
#2
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.
#3
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.
#4
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.
#5
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.
#6
i think the timelimit needs to be fixed as part of this patch. i'm not sure it needs to be a variable though.
#7
I like the idea of aligning that to the FormAPI cache cleaning.
#8
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).
#9
Patch applies cleanly. I agree that matching FormAPI's cache time is a good approach. SimpleTests run successfully. +1 for RTBC.
#10
#11
Committed to CVS HEAD and DRUPAL-6. Thanks! :)
#12
Automatically closed -- issue fixed for two weeks with no activity.