hook_file_references() was not designed for a highly flexible field storage

Makes sense, but this isn't really a database issue. It's a file subsystem issue. Refiling.

Object and oid are both reserved words for PostgreSQL at least IIRC, if not in ANSI SQL, so -1 on using those.

I'm in favor of this concept, but since the code is all DBTNG which I haven't gotten familiar with and testing framework which I haven't gotten familiar with, I can't actually offer a code review.

Couldn't the query in file_get_usage() be static? Doesn't seem to be any need for db_select() there.

subscribing

Oh man? d.o just swallowed my comment... good thing that Konqueror can recover it with "Back". So.

I had a look at it, and I think that when the last reference is removed, the file should be automatically deleted - there's no reason why modules should be calling both file_remove_usage() *and* file_delete(), because there's really only a use case for file_remove_usage() when file_delete() is called directly afterwards.

Removing the usage can't be done in file_delete() because it hasn't got all the arguments that file_remove_usage() needs, so I took the other road and put file_delete() into file_remove_usage() if no usages remain. As a bonus, that also makes it possible to simplify the semantics of file_delete(): it now always behaves like it did before with $force == TRUE, and the "soft delete" is now solely covered by file_remove_usage().

Other minor points tackled by this change of the patch:

• Bug fixed: $file in file_add_usage() in user_save() doesn't exist (anymore?), that's $picture instead.
• In user_save(), neither file_delete() nor file_remove_usage() is required for moved files, because the fid stays the same for the existing filepath when using file_move() with FILE_EXISTS_REPLACE, and file_move() also takes care of the remaining file_delete() stuff for the source file.
• user_file_references() is not required anymore... removed.

I think my SimpleTest setup is kinda broken because totally unrelated tests break, so here's the updated patch without making sure that all the tests pass.

The status change also got swallowed yesterday. Now, code needs review.

Maybe I get around to set up a proper installation where the tests work as they should, and find out what goes wrong. (Can't be too difficult, actually.) But maybe someone else can get around to do that sooner than me :P

Seems like no one has picked this up yet, and I'm back again. So, bugfixing time!
FileDeleteTest::testInUse(), bugs in system_cron(), and upload.module's form handling are now fixed - with a little luck, the test bot likes us now.

Feedback and reviews appreciated.

with patch.

Nobody told the test bot that it can use -p1 as option for patch. Freaking standard Git patch format.

Subscribing for later review, if no one beats me to it.

Rerolling to current HEAD. Also, improving hook_file_move() docs and example implementation. Also, implementing hook_file_move() for user.module and upload.module (using the example implementation) so that file references remain valid. That would fix the one TODO item that was still in there, I believe it's now ready for wide appeal.

- testInUse(): yup, good point - those strings must be leftovers from previous versions where file_remove_usage() didn't delete files when they weren't used anymore. I'll look into improving the explanations.

- I'd like to emphasize that you introduced the 'count' value by yourself, and while it's not that helpful for building backlinks, it frees modules from the need of counting the usages by themselves. Of course one might wonder if modules will at all need to use a single file on the same type+id multiple times, but in case we assume so, the 'count' value does have its merits.

subscribe

New version: fixes the test messages that drewish mentioned, and updates the update number in system.install. (Sorry for the long wait for just a few string fixes.)

My argument for the usefulness of the 'count' value has not yet been addressed, please comment on that. (Personally, I would expect an RTBC this time.)

Resending for test run.

Introducing a new tag for feature freeze: API clean-up.

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

Re-rolled against HEAD.

This still looks ready to fly...

1) ok, the tests fail, because the new File module in core implements *heavy* workarounds and really awkward functions to make hook_file_references() properly work with file fields. I'll try to get the tests working first, afterwards we can think about further clean-ups in there (and it looks like a couple of functions could be simplified or even removed). This patch also allows to remove a major @todo from file.module.

That alone justifies this patch to land even after API freeze.

2) A second thought I had may be deferred to D8, but: Those file usage functions basically have the potential to entirely eliminate the {file}.status column. A file that isn't used anywhere should be deleted during garbage collection, no? :)

subscribing

1) It turns out that the workarounds for hook_file_references() implemented in File field are quite heavy, so I'm attaching a work-in-progress patch here.

2) Still seems very valid to me. Probably really too late, but oh, nice conclusion. But then again, please also note:

3) All files uploaded via File field will be deleted after 6 hours. That is, because {file}.status is not updated properly. (And since that does not happen in File field at all so far, I'm heavily struggling to find the proper place to call file_add_usage()....)

Any feedback/help from someone who knows all this stuff more than me would be highly appreciated.

Might be good to ping quicksketch about this...

I'm heavily struggling to find the proper place to call file_add_usage()....

The call to file_add_usage() (or setting the status column to 1, if we don't get this in) should be done in file_field_update(). This function is called when the node is actually saved. It should also be done on insert, but since file_field_insert() calls file_field_update(), we can add it in just that one place.

Generally my feeling on this patch is a huge +1. File module *does* have to do all kinds of wonky querying to get file references, especially with Field API's abstraction layer over performing actual queries. File module right now is doing individual queries on every file field on the Drupal site, retrieving entire rows, then just calling count() on the complete rows. It's ridiculously inefficient, so I'd love to see this added if possible.

Considering all kind of possible field storage engines, I see no way around fixing this, in the way this patch attempts to.

If I understand correctly, this is also needed for modules that write files to the files table without being referenced anywhere (like the various file managers for D6 do at the moment), because File would otherwise delete them?

Thinking about file fields, image fields, my contributed modules, and overall usage in general, this still is critical. We totally have "managed unmanaged" files in D7 right now. :(

We need to get this sucker in prior March 1st.

this makes a lot of sense. re-roll of the patch from #42, revised to apply to current HEAD.

still need to look at "3) All files uploaded via File field will be deleted after 6 hours. That is, because {file}.status is not updated properly. (And since that does not happen in File field at all so far, I'm heavily struggling to find the proper place to call file_add_usage()....)" but see #618654: File and image fields are treated as temporary files and automatically deleted after six hours.

ah, #618654: File and image fields are treated as temporary files and automatically deleted after six hours went in, so maybe that's no longer an issue?

Thanks! This patch definitely needs help and reviews from file system maintainers.

Slightly cleaned up and fixed a couple of issues in the last patch, and also renamed those 3 functions to file_usage_list, file_usage_add, file_usage_delete().

.

This patch is really, really, really important. Would be great to get some traction here -- sorry, not much of a help here myself, the new File API is still slightly above my head :(

I agree that this is critical. We can't have simple node/user/comment/term deletion leading to a count of all files on your system.

Is there some reason that file_usage_add can't be hidden away inside of file_copy and file_move. That way, few modules would need to care about usage.

Any chance the file api maintainers, aaron and drewish, could push this along or postpone?

will look tomorrow. would be nice if we could throw proper errors rather than overloading return values with booleans or arrays like w/ file_delete()...

I really hope that this will be in D7. Is there any way to help here, or does it need love from File API maintainers?

I'm working on this patch again. After fixing all the minor code changes and failed hunks, this patch still has some critical problem in it. Though it puts all the infrastructure in place for recording usage, it doesn't actually keep track of usage properly yet when creating new content or deleting files when updating content. I'm trying to get all the usage recorded correctly but it might take a little while.

quicksketch is right, it wasn't actually recording anything. getting this to fly might be a little tricky from what I 've seen, good luck!

Here's the patch so far, at least it applies now and properly inserts/updates usage records. The tests are currently failing regarding advanced management, like deleting revisions but not the whole node and managing the files correctly. I'll still be working on this again tonight.

function file_usage_list(stdClass $file) {
  $result = db_query('SELECT f.module, f.type, f.id, f.count FROM {file_usage} f WHERE f.fid = :fid AND count > 0', array(':fid' => $file->fid));
  $references = array();
  foreach ($result as $usage) {

Add ->fetchAssoc() to db_query?

check to make sure $result isn't empty or else PHP notices will ensue at the foreach loop??

Add ->fetchAssoc() to db_query?

You don't need to actually turn the result into an array or object at all, since you can already iterate over results from PDO with foreach() just fine. However if you're returning the results or using them outside of the function we should definitely convert them to a standard array.

This patch adds full management of files again just like before. I have to say that generally speaking things are much better than before. The syntax now actually makes sense. Previously every time you called file_delete() you had to set a bunch of special flags on the $file object so that your module would skip counting it in hook_file_references(). Now you just remove your module's references before calling file_delete(). All in all this is a huge reduction of complexity when deleting files or doing any operation that requires checking a file's usage.

As far as I know, this patch provides full functionality. I've test all the operations manually, because currently SimpleTest is broken in HEAD and it is impossible to run tests through the UI. Unless tests are fixed, it'll be very difficult for me to proceed any further (unless by some miracle this passes test-bot on the first try).

Blind attempt at trying to fix a few tests.

Found a way to hack around SimpleTest UI. It might be throwing thousands of errors when run through the Testing UI (as are all other tests), but hopefully this will pass TestBot.

Heh, at this rate it will only take me three more patches. ;-)

More test fixes. Feeling good about this one.

Down to just 2 exceptions...

Apologies to issue subscribers, normally I'd test my patches *before* posting them. :P

There are many unindexed queries in there, I see at least two, the file_usage_list and the delete one. The patch lacks -p making it a bit hard to see what changes. expression needs to use arguments not intval and string concat, brrr.

So far, great.

Adding indexes per chx's suggestions. Adding use of arguments in the expression() method. Rerolled with -p for further review.

Quick style review. (possibly didn't see everything)

Sidenote: I know lots of these "errors" are not caused by the writer(s) of this patch. But it would be nice if we could fix documentation in small steps. Maybe it looks weird now because we wil have some fixed headers and some unfixed. But in the end the result will be nice ;)

+++ includes/file.inc	3 Jul 2010 07:04:18 -0000
@@ -531,6 +531,120 @@ function file_save(stdClass $file) {
+ * Determine where a file is used.

Determines

+++ includes/file.inc	3 Jul 2010 07:04:18 -0000
@@ -531,6 +531,120 @@ function file_save(stdClass $file) {
+ * @param $file
+ *   A file object.
+ * @return

newline needed

+++ includes/file.inc	3 Jul 2010 07:04:18 -0000
@@ -531,6 +531,120 @@ function file_save(stdClass $file) {
+ * Record that a module is using a file.

Records

+++ includes/file.inc	3 Jul 2010 07:04:18 -0000
@@ -531,6 +531,120 @@ function file_save(stdClass $file) {
+ * Remove a record to indicate that a module is no longer using a file.

Removes

+++ includes/file.inc	3 Jul 2010 07:04:18 -0000
@@ -531,6 +531,120 @@ function file_save(stdClass $file) {
+ * @param $count
+ *   The number of references to delete from the object. Defaults to 1. 0 may
+ *   be specified to delete all references to the file within a specific object.
+ * @return

newline needed

+++ includes/file.inc	3 Jul 2010 07:04:18 -0000
@@ -531,6 +531,120 @@ function file_save(stdClass $file) {
  * Copy a file to a new location and adds a file record to the database.
  *

copies

+++ includes/file.inc	3 Jul 2010 07:04:18 -0000
@@ -949,29 +1063,30 @@ function file_create_filename($basename,
 /**
  * Delete a file and its database record.

Deletes

+++ modules/file/file.field.inc	3 Jul 2010 07:04:18 -0000
@@ -316,32 +346,29 @@ function file_field_delete($entity_type,
+ * Decrement a file usage count and attempt to delete it.
+ *

Decrements / attempts

+++ modules/file/file.module	3 Jul 2010 07:04:19 -0000
@@ -929,72 +918,6 @@ function file_icon_map($file) {
  * Get a list of references to a file.

gets

+++ modules/file/file.module	3 Jul 2010 07:04:19 -0000
@@ -1007,8 +930,8 @@ function file_get_file_reference_count($
+ *   (optional) The name of a field type. If given, limits the reference check
+ *   to fields of the given type.
  * @return

newline

36 critical left. Go review some!

Comment fixes recommended by aspilicious plus a few others to convert function documentation to the proper tense. I didn't add the suggested newlines (I'm assuming they were supposed to go before the @return?) because there are no new lines before @return anywhere in all of file.inc. If we're going to make a bid for consistency of @return statements we should do it all at once and consistently across all Drupal. Right now it's mixed and adding new lines at just a few places would just make file.inc inconsistent with itself.

Ok quicksketch, I'll make an issue about the newlines in file.inc.
Thnx for fixing the other issues :)

EDIT: already made the issue: #844392: Add extra newlines in file.inc

+++ includes/file.inc	3 Jul 2010 17:47:32 -0000
@@ -531,7 +531,121 @@ function file_save(stdClass $file) {
+ *   An nested array with usage data. The first level is keyed by module name,

A nested

+++ includes/file.inc	3 Jul 2010 17:47:32 -0000
@@ -531,7 +531,121 @@ function file_save(stdClass $file) {
+ *   the second by table name, the third has 'id' and 'count' keys.

table name? and yet we use $usage->type for the second level.

+++ includes/file.inc	3 Jul 2010 17:47:32 -0000
@@ -531,7 +531,121 @@ function file_save(stdClass $file) {
+  $result = db_query('SELECT f.module, f.type, f.id, f.count FROM {file_usage} f WHERE f.fid = :fid AND count > 0', array(':fid' => $file->fid));

Not really a problem, but why do we add all these f. prefixes? You have one table to boot and the query is superb unlikely to be altered.

+++ includes/file.inc	3 Jul 2010 17:47:32 -0000
@@ -531,7 +531,121 @@ function file_save(stdClass $file) {
+ *   The name of the object where the file is referenced.

The type of the object.

+++ includes/file.inc	3 Jul 2010 17:47:32 -0000
@@ -531,7 +531,121 @@ function file_save(stdClass $file) {
+    $query->condition('type', $type);
+    $query->condition('id', $id);

No need to write out query twice.

+++ includes/file.inc	3 Jul 2010 17:47:32 -0000
@@ -531,7 +531,121 @@ function file_save(stdClass $file) {
+  if ($file && !$result) {

An object casted to bool will always be TRUE AFAIK so what did you want to say in this if? As it stands this is equivalent to if (!$result) which looks fine to me. Also does delete return the number of rows deleted? I have no idea. Has this been tested?

+++ modules/file/file.field.inc	3 Jul 2010 17:47:33 -0000
@@ -263,37 +263,71 @@ function file_field_presave($entity_type
+  // Create a bare-bones entity so that we can load its previous values.

There is a entity_create_stub_entity() which does just what you want here.

+++ modules/file/file.field.inc	3 Jul 2010 17:47:33 -0000
@@ -316,32 +346,29 @@ function file_field_delete($entity_type,
+function file_field_delete_file($item, $field, $entity_type, $id, $count = 1) {

Where is the doxygen of the arguments?

+++ modules/file/file.module	3 Jul 2010 17:47:33 -0000
@@ -520,11 +512,8 @@ function file_managed_file_validate(&$el
+        if (empty($references)) {
           form_error($element, t('Referencing to the file used in the !name field is not allowed.', array('!name' => $element['#title'])));

I do not grok this. If there are no references then we throw a form error ... why exactly?

+++ includes/file.inc	3 Jul 2010 17:47:32 -0000
@@ -531,7 +531,121 @@ function file_save(stdClass $file) {
+ *   An nested array with usage data. The first level is keyed by module name,

A nested

37 critical left. Go review some!

Thanks chx, and excellent review. Regarding this question:

if (empty($references)) {
  form_error($element, t('Referencing to the file used in the !name field is not allowed.', array('!name' => $element['#title'])));

I do not grok this. If there are no references then we throw a form error ... why exactly?

This is because if a file exists in the database but is not referenced at all (say it's a file uploaded to a node but never actually saved), you shouldn't be able to attach it to a field because it would inadvertently give you the ability to delete a file. You shouldn't be able to attach a file and then delete the node and have the file be deleted when you weren't responsible for uploading it in the first place. So in other words, this check prevents a security problem which would allow users to delete arbitrary files (that are not in use elsewhere) by attaching them and then deleting the node.

I'll get a reroll out with the rest of the changes, great feedback.

Don't forget to add "this check prevents a security problem which would allow users to delete arbitrary files (that are not in use elsewhere) by attaching them to a node and then deleting the node" or something like that as inline comment to that form_error section as you described in #87. It's definitely not self evident.

Crosslinking #846296: file_file_download() only implements access checks for nodes and users, this seems related but not only/exactly the same issue as this. Can someone please look into that?

Here's a reroll with all the recommended changes. Most of them are just grammatical fixes so there's no need for explanation on those. I fixed any references to "table name" as a parameter, since it was inaccurate and I meant to get rid of those references entirely anyway. The new terminology is "object type" (i.e. node, user, term, etc.) though it's not 100% accurate (it's doesn't need to be an "object" in the OO sense), I think it's close enough and more accurate than something like "entity", since there's no need for file_usage entries to be owned by entities.

Don't forget to add "this check prevents a security problem which would allow users to delete arbitrary files

Turns out there is already an explanation in the code, it's just not right next to the changed lines so it doesn't show up in the patch.

The only thing I wasn't sure about was chx's suggestion to reuse the query arguments:

+++ includes/file.inc 3 Jul 2010 17:47:32 -0000
@@ -531,7 +531,121 @@ function file_save(stdClass $file) {
+    $query->condition('type', $type);
+    $query->condition('id', $id);

No need to write out query twice.

There are 2 queries here, one is a delete and one is an update. They're also slightly different in that one uses an expression and the other uses a condition for the "count" column. So they're similar but not really the same queries. I'm not sure reusing conditions would make the code any clearer, but I can't figure out how you would re-use conditions between two different types of queries anyway. So for the time being they're just separate and a few conditions are repeated.

"No need to write out query twice. "

I think he just meant it syntactic:

$query->condition('type', $type)->condition('id', $id);

Reroll after #844392: Add extra newlines in file.inc rather unfortunately broke the line offsets.

I didn't review this in depth yet but it looks so much cleaner/saner.

system_update_7035() runs before the one added here, user pictures too I think, this means that there are going to be zero records in {file_managed}{file_usage} on a site upgraded from Drupal 6 to 7. Additionally the update->file updated doesn't use API functions so it wouldn't add those records anyway.

Also adding D7 upgrade path tag - if this goes in after a beta then it's going to require an additional upgrade path for any existing file usage on D7 sites too.

Thanks catch. Putting this back to needs work since the upgrade path from upload.module needs to add the usage entries, in addition to user.modules defining its entries in a separate table (really it's just going to be one usage per user picture, so a simple upgrade). Functionality and code-wise, I'd still appreciate further reviews so we can get them out of the way. From my perspective this is already totally ready to go from the functional perspective.

Re #92, right instead of

      $query->condition('type', $type);
      $query->condition('id', $id);

just write

      $query
          ->condition('type', $type);
          ->condition('id', $id);

Reroll including minor change suggested by chx in #96 and including a fixed upgrade path for upload -> file module and a new update for registering user.module file usage.

Re-roll of #97 to match HEAD.

Okay, let's try this one instead...Only change is to rename:
system_update_7059 to: system_update_7060

Thanks noahb! Hard to keep up with all the constant updates. I swear I CVS updated only about 5 minutes before I uploaded that last patch. :-)

Doh! We're at review right now. I've tested the new upgrade path with success, it's a pretty simple round of updates.

+1. Code looks very good, nice clean-up. I can't thoroughly comment on the approach, though, so not setting RTBC.

Add beta blocker tag

Somehow between patches #93 and #97 I uploaded a patch that did not include some of the suggested changes. Here's the patch that includes the new indexes and other fixes that were mistakenly lost between rerolls.

Wonderful patch, removes a lot of ugly code. Thanks for making it work, quicksketch!

Looks like we're very close; only outstanding issues:

+++ includes/file.inc	11 Jul 2010 18:33:12 -0000
@@ -542,7 +542,127 @@ function file_save(stdClass $file) {
+ * @param $file
...
+ * @return

(everywhere) There should be a blank line between @param and @return.

+++ includes/file.inc	11 Jul 2010 18:33:12 -0000
@@ -542,7 +542,127 @@ function file_save(stdClass $file) {
+function file_usage_list(stdClass $file) {

(and elsewhere) If I'm not mistaken, then we do not use type hinting for objects of type stdClass, so as to allow for alternate and more advanced implementations.

+++ includes/file.inc	11 Jul 2010 18:33:12 -0000
@@ -542,7 +542,127 @@ function file_save(stdClass $file) {
+ * - A module that associates files with node revisions, so $type would be
+ *   'node' and $id would be the node's nid.

This example is odd. When associating with a node revision, then the $id should be the node's vid, no?

+++ includes/file.inc	11 Jul 2010 18:33:12 -0000
@@ -542,7 +542,127 @@ function file_save(stdClass $file) {
+ * @param $count
+ *   The number of references to add to the object. Defaults to 1.

(and elsewhere) Optional function argument descriptions must be prefixed with "(optional)". See http://drupal.org/node/1354 for more information.

+++ includes/file.inc	11 Jul 2010 18:33:12 -0000
@@ -542,7 +542,127 @@ function file_save(stdClass $file) {
+function file_usage_delete(stdClass $file, $module, $type = NULL, $id = NULL, $count = 1) {
...
+  if ($type && $id) {
+    $query
+      ->condition('type', $type)
+      ->condition('id', $id);
+  }

I'm not sure I understand why $type and $id can be optional, and, if they can be omitted, what that effectively means to the deletion, and, when I'm allowed to do that. At minimum, the PHPDoc should clarify and perhaps also provide an example for this. But also, perhaps these arguments don't need to be optional?

+++ includes/file.inc	11 Jul 2010 18:33:12 -0000
@@ -542,7 +542,127 @@ function file_save(stdClass $file) {
+ * @param $count
+ *   The number of references to delete from the object. Defaults to 1. 0 may
+ *   be specified to delete all references to the file within a specific object.
...
+  $query = db_delete('file_usage')
...
+  if ($count) {
+    $query->condition('count', $count);
+  }
+  $result = $query->execute();

Same here...? Though in this case, by passing a $count of 2, the delete query would look for exactly 2, would not find 1, proceed to the relative decrease, and ending up with a negative count (1 - 2 == -1).

+++ includes/file.inc	11 Jul 2010 18:33:12 -0000
@@ -966,30 +1086,30 @@ function file_create_filename($basename,
 function file_delete(stdClass $file, $force = FALSE) {

Only now I see that we have stdClass type hints thoughout file.inc already -- so that's a separate issue not to be discussed here.

+++ modules/file/file.field.inc	11 Jul 2010 18:33:13 -0000
@@ -263,37 +263,65 @@ function file_field_presave($entity_type
 function file_field_update($entity_type, $entity, $field, $instance, $langcode, &$items) {
...
+  list($id, $vid, $bundle) = entity_extract_ids($entity_type, $entity);
+
+  // On new revisions, all files are considered to be a new usage and no
+  // deletion of previous file usages are necessary.
   if (!empty($entity->revision)) {
+    foreach ($items as $item) {
+      $file = (object) $item;
+      file_usage_add($file, 'file', $entity_type, $id);
+    }
     return;
   }

This somehow looks as if we should additionally key file usage per $revision_id.

It doesn't completely feel right to key by $id only, and also not by $vid only.

Is there a reason we don't want to support tracking file usage on revision level? I could imagine that this might turn out quite limiting.

+++ modules/system/system.module	11 Jul 2010 18:33:37 -0000
@@ -2799,8 +2799,14 @@ function system_cron() {
+      else {
+        watchdog('file system', 'Could not delete temporary file "%path" during garbage collection, because it is still in use.', array('%path' => $file->uri), WATCHDOG_ERROR);
       }

This second/alternative watchdog shouldn't be an error, rather info.

+++ modules/user/user.install	11 Jul 2010 18:33:38 -0000
@@ -715,6 +719,35 @@ function user_update_7011() {
+
+

Duplicate newline here.

Powered by Dreditor.

This looks great, I read through and wasn't able to find any issues that weren't in sun's review.

With tracking on revisions - if we always add to the usage count when a revision is added, and always decrement it when a revision is deleted, then it seems like it might take care of itself. A single file_usage entry accounts for all the revisions. However if that's so, then at least additional comments would be good. However it feels like it's more likely to get out of sync than tracking a usage per revision.

If we did want to switch to per-revision tracking, since revision IDs are unique I'm not sure we need a new column though, would just be changing the tracking to use vid instead of id? Although if you wanted to find what's referencing what it'd make the information less useful.

The revision vs. entire node tracking is intentional, since when you delete an entire node you don't want to have to do additional queries to find all the applicable vids and then do extra queries to delete each revision's files individually. Revision usages are still tracked within the larger node as a whole.

I'm not sure I understand why $type and $id can be optional, and, if they can be omitted, what that effectively means to the deletion, and, when I'm allowed to do that.

$type and $id are optional because if you delete an entire file, it doesn't matter where it's in use, you just want to delete all usages of it that your module is responsible for.

So from what I can tell the necessary changes are:
- Add new lines before @returns.
- Fix things so that if a count > than the current count is passed in the row is deleted.
- Change a watchdog error to an info.
- Remove a duplicate new line.

Entire node tracking makes sense to me. Only substantive issue then is the count.

Fixes issues as noted in #109.

Urg, forgot to remove the extra newline in user.install. Here we are.

Thanks, Nate! That's one giant awesome patch. You deserve some cool Danish beer.

I can haz summary, plz?

Especially for developers taking part in #D7CX and what they need to do?

tag per #114

Summary:

This API change affects three operations of managing files:

1. Deleting managed files
2. Saving managed files
3. Getting a list of where a file is in use

Deleting managed files:
Currently we require modules to implement hook_file_references() to report usage of files. This hook is currently called when a file is deleted and must return no references in order for the file to be deleted.

Before (this code is actually so complicated it's hard to summarize, but here's the gist):

  // Before deleting a file, you must set special flags on your $file object:
  $file->file_field_name = $field_name;
  $file->file_field_id = $field_id;
  file_delete($file);

  // Then in hook_file_references() for your module you return the file usage count *excluding* the object that's about to be deleted:
  if (!(isset($file->file_field_name) && isset($file->file_field_id) && $file->file_field_name == $field_name && $file->file_field_id == $field_id)) {
    $count += db_query("SELECT COUNT(*) FROM {mytable} WHERE fid = :fid", array(':fid' => $file->fid));
  }

After:

  // Remove your module's usage before calling file_delete():
  file_usage_delete($file, 'mymodule', 'node', $node->nid);
  file_delete($file);

The previous approach requires extremely convoluted code (about 80 lines from file.module) and one query per field per file. The new approach requires a single line in implementing modules and two queries to check and decrement usage per file being deleted. So rather than having a O^n algorithm we're now O*n (where n == number of files to delete and number of fields on the site).

Saving managed files:
This new approach means that when adding a new file, you need to record a usage in the file_usage table. So when permanently saving a file you now need to add file_usage_add($file, $module_name, 'node', $node->nid) in your submit handlers. Previously you did not have to do anything other than save the file.

Before:

$file->status = FILE_STATUS_PERMANENT;
file_save($file);

After:

file_usage_add($file, 'mymodule', 'node', $node->nid);
$file->status = FILE_STATUS_PERMANENT;
file_save($file);

Note that most of the time (if you're using the #type => 'managed_file' FAPI element), the file will actually already be saved for you as a temporary file. The submit handler will be responsible for setting the status to permanent and adding the file usage.

Getting a list of where a file is in use:
This appears to be a simple change from the calling side.

Before:

$usages = module_invoke_all('file_references', $file);

After:

$usages = file_usage_list($file);

The previous approach required, at minimum, a query per module. In the case of file.module, this was one query per field. The new approach requires a single query and is in fact much more useful, because previously hook_file_references() would only tell you the number of times a file was in use, it wouldn't tell you *where* the file was in use at all. Previously it was impossible to get this information through a centralized location.

In addition to quicksketch's excellent summary:

• It was debated and investigated whether file_usage_delete() could be embedded into file_delete(), but that would not contain the required information passed via
  

    file_usage_delete($file, 'mymodule', 'node', $node->nid);
  

• It was further investigated whether a permanent file status as in
  

  file_usage_add($file, 'mymodule', 'node', $node->nid);
  $file->status = FILE_STATUS_PERMANENT;
  

  could not be assumed always, i.e., for every file_usage_add(). However, since we are not 100% sure about this relationship and it's very late for D7, we just keep the manual flag and simply find out in the real-world, potentially to be changed for D8.

While working on this: #768090: drupal_static performance improvement, I uncovered a pretty nasty bug in file_get_file_references() where it is erroneously calling drupal_static(). Here is the problem in a nutshell:

Current:
  $references = drupal_static(__FUNCTION__, array());

Fixed:
  $references = &drupal_static(__FUNCTION__, array());

I'm not familiar with your file handling end of the drupal code base so I have no idea what the ramifications of this fix might be, but it could likely resolve some issues. I posted a patch on the #846296: file_file_download() only implements access checks for nodes and users issue which fixes this error.

#118 is irrelevant to this issue because this issue removes the get references hook.

Sorry for the interruption. BTW, it turns out that this drupal_static() call error has no real ill effects anyway. Carry on.

Rerolled against HEAD.

Another reroll, not sure if this is going to have the same test fail as #121, but it's just an update for HEAD. Perhaps something changed that we'll need to accommodate for.

doh, guess something was changed.

I was going to look into this but I can't reproduce the failing test, what gives?

#123: file_usage.patch queued for re-testing.

Note that if quicksketch does not come along to further explain that , drewish's request is a followup because that error message is already in HEAD, it's a straight conversion from the current state. Further, because just a few lines up it explains nicely as

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.

The comment mentions a "unmanaged files" which by definition will not have records in {files} and therefore not be loadable by file_load().

It's possible that a file could be added to the file_managed table (via file_save()), but no usages yet added. Say if a module like IMCE is adding a file that is not in use anywhere, there are no usages but the file would exist in the database. Arguably this case should never occur if modules manage their usages properly. But on the other hand, if this situation never occurs then this message won't show up anyway. In Drupal 6, IMCE inserted it's files into the database but did not implement hook_file_references(), which is why this check was added in the Drupal 6 version of FileField.

Or another scenario, a sly user might re-use a file that is in the temporary state. Say two users are creating content at the same time, one user uploads a file and then the other user re-uses that file before the node is saved. The second user then saves the node and then deletes it before the first user has saved their node. Now the first user's file has been deleted before they even saved the node, since the second user's node was the only "usage" reported (usages aren't added until the node is saved).

I should also note that we're not changing the functionality of this line at all with this patch. It's a security mechanism (albeit for edge-cases) to prevent users from deleting files that exist in the file_managed table but do not have any usages yet.

So in other words, there's a difference between "managed files" and files that are "in use". If we want to be accurate about it we can change the phrase "unmanaged files" to "files that are not in use". But really I think we're splitting hairs here.

#139: drupal_353458.patch queued for re-testing.

Reroll of #139 updated for changes in user.module.

drewish and I did discuss this one day, without reaching any particular conclusion. The hold-up is over whether or not the file usage check is necessary when re-using a file. The basic arguments are:

A) Keeping the check has no negative side-effect (other than one SQL query per file uploaded) and provides a safety check that prevents accidental (or malicious) deletion of files that are stored in the "file_managed" table but do not have any usages reported.

vs.

B) The check is unnecessary because any file that is in the "file_managed" table should have usage reported. Any module that is saving files but not recording usage is at fault for allowing accidental or malicious deletion of its uploaded files.

I should note that the check already existed before this patch and isn't a change at all in behavior from the current code, just the mechanism for getting the list of file usages has changed and so the check needed to be updated. I personally feel this is still RTBC, though I'll defer to others whether the check should be removed. Right now it's just a little silly this is being held up over a philosophical issue of whether core should provide security safety checks or not.

If the check was there previously then that should be a separate issue and not hold this one back to rot, I think.

reroll to fix PHP Fatal error: Cannot redeclare user_update_7012()

This is the same test fail we got before that I (and others) wasn't able to reproduce locally. mikey_p still hasn't told us how he was able to get it to pass testbot :)

I'll take another look and see if I can get a pass.

Okay this should do the job. SimpleTest is STILL broken and impossible to run through the Drupal UI. Seriously, does no one EVER test patches before uploading them? :P

This patch was broken by #668058: Resubmitting a user picture does show the first uploaded picture, though overall an improvement. The change removes some funny file handling we had to do because file names were identical, which is different than most other places throughout Drupal.

Sorry the last patch included my hacks to the tests to run them manually.

This was marked RTBC by chx in #130 and I've been running Drupal with this patch for quite awhile without any problems. drewish's concern with code comments appear to be resolved. The file usage check stuff described by quicksketch in #142 which already exists in Drupal w/o this patch really belongs in a separate issue for discussion, if necessary.

Back to RTBC.

This is close:

+++ includes/file.inc	10 Aug 2010 05:02:42 -0000
@@ -542,7 +543,134 @@ function file_save(stdClass $file) {
+ * @return
+ *   TRUE for success, or FALSE if no usages remain but the file still could
+ *   not be deleted.

This is not working as advertised. The function currently returns nothing. Probably needs tests.

+++ modules/file/file.module	10 Aug 2010 05:02:43 -0000
@@ -520,11 +512,8 @@ function file_managed_file_validate(&$el
-        if ($reference_count == 0) {
+        $references = file_usage_list($file);
+        if (empty($references)) {
           form_error($element, t('Referencing to the file used in the !name field is not allowed.', array('!name' => $element['#title'])));
         }

Technically not part of this patch but that error message is a bit wonky. It should probably read 'References' instead of 'Referencing'.

Needs work

This is not working as advertised. The function currently returns nothing. Probably needs tests.

Nice catch. The return value here was left over from when file_usage_delete() actually called file_delete() automatically for you. I separated these two several patches ago because the automatic calling of file_delete() was not always wanted and we needed more control over when it was called. To fix the problem I've simply removed the PHPdoc on the return value, since this function doesn't return anything or need to (just like file_usage_add()).

It should probably read 'References' instead of 'Referencing'.

I changed this to "The file used in the !name field may not be referenced.", which sounds less aggressive than "is not allowed". No other changes from the last patch, no functional differences at all.

Just a note we've been using this on ex2 for a couple of months with no negative side-effects (in fact had to run it because there were serious issues without it). Marking back to RTBC since I think Dries' comments are dealt with, please wait for the bot to come back green.

Green! Committed to CVS HEAD. Thanks.

Looks like #116 is the summary, and that this definitely needs an announcement. @quicksketch, hoping to catch you today for a rundown on this.

This seems to be missing documentation. It was a bit of the WTF moment when trying to save a form that had a managed file element, only to have a validation error "The file used in the xxxx field may not be referenced.".

The example seems to be also lacking an example of this too @ http://api.drupal.org/api/drupal/developer--examples--image_example--ima...

Removing beta blocker tag. This is now a documentation issue.

Given that this issue has 160 comments already, could you maybe file a separate issue explaining what needs documentation? Those of us not familiar with this issue, who might actually add documentation, may not have the time or inclination to read through the whole thing and figure out the problem.

After filing a separate issue, could you perhaps add a link here and set back to the status it had before #158?

thanks...

Maybe just the usage of the managed_file would be a good start. The code is well documented, maybe to well as there is a lot to read. I actually gave up on using both managed_file & a custom fieldable bundle as the process was leading to issue after issue, some were real known bugs, others were just missing function / classes in my code. So I got a third party TTS library and dynamically generated the mp3's from text, much easier!

I can not remember the exact things, but relevant to this thread was the need to update two tables with the file information, the managed_file table and the file_usage table. Missing the latter table causes validation errors on the second save. Some of the other known bugs made me drop this one in the end.

In relation to the fieldable bundle, I gave up when I was getting errors due to default field classes not being able to correctly render the defaults. I had not actually written any classes for this, and I was relying on the core base classes. I can not remember the exact details of the error, blaming it on some missing functionality in my code.

There appears to be strong coupling here between the two, so mimicking the File field code was the best doco that I could find at the time, as I agree, it takes hours to parse old issue threads. So maybe just the bare bones functionality required to generate a managed_element and to save it would be about it. I do not know how many contributed modules are using the files/managed_file table, but the file usage step will effect all of these.

If you don't have anything definite that needs to be documented, can you please re-close this issue (with the old attributes)? If you have something definite, can you please open a new documentation issue? Thanks.