Posted by Deciphered on October 30, 2009 at 4:28am
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | file.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | File API, needs backport to D7 |
Issue Summary
Hi,
It appears that both the File module and Image module are only saving files as temporary files ($file->status == 0), as they are using file_save_upload() to save the files.
Cheers,
Deciphered.
Comments
#1
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.
#2
Let me know how you go and I would be more than happy to test anything for you as I was hoping to use the status in the Drupal 7 FileField Paths port.
#3
#331013: Remove file_set_status() in favor of file_save() seems to suggest that $file->status should be set manually:
According to the docs for file_save_upload(),
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.
#4
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.
#5
#6
The last submitted patch failed testing.
#7
subscribe
#8
updated patch attached. not 100% sure its correct, but all file and file api tests pass.
i've tried to differentiate between the insert of a new object with a file field attached and a new revision of an existing object. lets see what the bot thinks.
#9
tagging.
#10
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.
#11
quicksketch: thanks for the review. yeah, i mixed up the containing object and the field. attached patch fixes that, so we always set the status to permanent regardless of the newness of the containing object.
i guess we also need a test, if my patch at #8 got the green light but was broken.
#12
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.
#13
i'm really messing with stuff i don't quite grok here. seems what we really need is tests. no way this should keep coming back green if its broken in two different ways.
working on it...
#14
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 ;-).
#15
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.
#16
subscribe
#17
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?
#18
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
#19
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.
#20
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!
#21
Better patch with a real description for the new method on FileFieldTestCase. (Duh.)
#22
Code works as as intended. As a follow-up we should add more image field tests.
#23
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.
#24
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?
#25
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():
<?php// 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
<?php/**
* 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
}
}
?>
#26
Here's the change above as a patch if you really want it. It won't pass tests.
#27
@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?
#28
@webchick - that behavior is directly ported from D6
#29
#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.
#30
The last submitted patch, 618654-patch-work-26.patch, failed testing.
#31
#21: file-save-temp-618654-21.patch queued for re-testing.
#32
If #21 doesn't pass, I can easily bzr up and re-roll.
#33
#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.
#34
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.
#35
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.
#36
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.
#37
Well, then. :) #21 it is.
Committed to HEAD. Can I get a follow-up patch for the comment clarifications?
#38
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.
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.
#39
Crosspost. Maybe we can add the $item['delete'] check in the followup too then. ;-)
#41
did anyone make a follow up?
#42
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.
#43
The last submitted patch, 681654.triggering_element.patch, failed testing.
#44
#42: 681654.triggering_element.patch queued for re-testing.
#45
subscribe
#46
Still an issue for me in Drupal core 7.x-dev as of February 2011.
#47
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.
#48
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.
#49
@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?
#50
@bfroehle, @David_Rothstein, thanks and sorry for the status change! I'll look into the issue mentioned in #49. Thanks again!
#51
subscribe
#52
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.
#53
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.
#54
@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.
#55
subscribe. Still an issue in 7.2.
#56
subscribed, got it here too in 7.2
#57
You need to use 7.x-dev until 7.3 comes out.
#58
#42: 681654.triggering_element.patch queued for re-testing.
#59
#60
subscribe
#61
subscribe
#62
subscribe Thanks!
#63
This is now fixed in the new D7.4 - yay!
#64
See comment #49 :)
#65
subscribe
#66
#42: 681654.triggering_element.patch queued for re-testing.
#67
The last submitted patch, 681654.triggering_element.patch, failed testing.