file_get_file_references() is rather bogus

+++ b/core/modules/file/file.field.incundefined
@@ -930,6 +930,26 @@ function file_field_formatter_view($entity_type, $entity, $field, $instance, $la
+ * Helper to determine whether a field references files.

...references a {file_managed}

+++ b/core/modules/file/file.field.incundefined
@@ -930,6 +930,26 @@ function file_field_formatter_view($entity_type, $entity, $field, $instance, $la
+ * @param array $field

Needs description, probably a field_instance

+++ b/core/modules/file/file.field.incundefined
@@ -930,6 +930,26 @@ function file_field_formatter_view($entity_type, $entity, $field, $instance, $la
+ * @return bool

blank line needed before

+++ b/core/modules/file/file.moduleundefined
@@ -1670,24 +1648,67 @@ function file_icon_map(File $file) {
+      elseif (!isset($file_fields[$entity_type][$bundle])) {
+        $instances = field_info_instances($entity_type, $bundle);
+        $file_fields[$entity_type][$bundle] = array();
+        foreach ($instances as $field_name => $instance) {

need more inline docs

@chx has been asking me to review this for ages. Upon initial inspection it all seems to make sense to me. It's a more logical way of collecting file usages than what we're doing currently, which is basically legacy code from before we had the file_usage table to reference.

Actually upon investigation (and trying to compare to the previous code), it looks like this entire patch was actually committed in #1801726: EntityFieldQuery v2. Doh.

Well the new approach may be more efficient but it certainly is really ugly. Any new cleanup to simplify the code would be great, but seems like it should be a different issue.

Added Entityreference to the summary.

As mentioned in #9 this was already committed to Drupal 8.

I've attached a backport of the current code for Drupal 7 assuming that's the remaining task here with cleanup done in another issue. Otherwise, please feel free to update the issue title/status.

So, I was debugging a site using Commons with a private filesystem. The root cause of the issue is quite irrelevant at this point, but it led me down the rabbit hole to realise that there is an issue with the latest patch. Let me elaborate.

$usage_list = file_usage_list($file);

file_usage_list, will return an array mapping (in the deepest level) IDs to counts https://api.drupal.org/api/drupal/includes%21file.inc/function/file_usag...

thus resulting in an array eg:

array { 
    ["file"]=> array { 
        ["node"]=> array { 
            [216]=>  "1" } 
        }
    }
}

Later on in the patch, there is this foreach:

foreach ($file_usage_list as $entity_type => $entity_ids) {

In which, entity_ids is an array eg:

array { 
    [216]=>  "1",
 }

Notice how the entity ID is actually the key to the array and not the value?

Further down, this array is "fed" into entity_load:

$entities = entity_load($entity_type, $entity_ids);

which does not comply with the array structure entity_load expects, loading node/1 in every case (for the site I was troubleshooting at least - other numbers may come up), which in turn propagates to the root issue I started investigating 3 hours ago :)

I'm attaching a patch that adds an

$entity_ids = array_keys($entity_ids);

That should ensure that the correct entities are loaded in every case.

12: 1805690_12.patch queued for re-testing.

Attempting to recreate the patch.

@chx, sorry if the patch is failing, it's my first attempt at committing an actual patch and not proposing the the chance in code blocks :/

Not at this point I'm afraid. Time limitations. Sorry.

It seems that this is already committed to core?

Re-roll for Drupal 7.43

A few nitpicks and it is OK.

1. +++ b/modules/file/file.module
   @@ -1042,27 +1022,84 @@ function file_icon_map($file) {
     * @return
   - *   An integer value.
   

   Let's add the type: array or even array[].

   In fact, let's specify the type of the params as well. it is not a must for D7, but it clarifies a lot (while looking at this code, all the time I was thinking that $field was a string...).

2. +++ b/modules/file/file.module
   @@ -1042,27 +1022,84 @@ function file_icon_map($file) {
   +     $entity_ids = array_keys($entity_ids); ¶
   

   space at end of line after ;

3. +++ b/modules/file/file.module
   @@ -1042,27 +1022,84 @@ function file_icon_map($file) {
   +     $entity_ids = array_keys($entity_ids); ¶
   +     $entity_info = entity_get_info($entity_type);
   +      // The usage table contains usage of every revision. If we are looking
   

   add 1 space of indentation. to the code lines.

Other remarks:

1) This patch solves bugs that are not described in the IS:
- If you call this function multiple times with different fids, you always get the same results.
- If you call this function multiple times with different values for $age, you always get the same results.
- You can't leave $field AND $field_type empty (hmmm, it is described but differently).
I updated the IS for this.

2) I closed #993082: file_get_file_references() @return doc totally wrong as a duplicate.

Fietserwin, I think you reviewed the Drupal 8 patch (which has already been committed), not the Drupal 7 patch.

I reviewed the patch in #25 ?!

That is the right patch, I need glasses.

Meanwhile I noticed a more serious problem with the patch:

    $usage_list = file_usage_list($file);
    $file_usage_list = isset($usage_list['file']) ? $usage_list['file'] : array();

If a file is referenced by something other than the file module, the references are not found.
In fact I have a custom module that references files. It's based on the file module.
I had been wondering why I'm spammed with errors 'Did not delete temporary file "foo" during garbage collection, because it is in use by the following modules: bar'

I'm not sure if it's a defect in the patch, or if I'm doing something weird?
I can' t test it directly without the patch because without it the function doesn't work with mongo_entities
because of the EntityFieldQuery that's missing an 'entity_type' entityCondition.

Yes: you can refer to images/files on the public/private file system directly or via a managed file. If you use the insert module to get inline images you even have both.

But I think that the function file_get_file_references() and thus this patch is about references to a managed file (a record in the table file_managed), not to the underlying file/image. So, for me, this patch is not incorrect because of this.

I am at this moment writing a module to find and remove duplicate images(/files) and there, besides searching in the table file_managed, yes, I also have to search through all long text fields and such.

But my custom module does write records to file_managed and file_usage. Other modules than 'file' can do that,
otherwise why would file_usage_list() be keyed by module?
Looking at the image module, it seems to do the same thing. In image_field_update_field() it calls

file_save($file_new);
 file_usage_add($file_new, 'image', 'default_image', $field['id']);

file_save() writes to file_managed and file_usage_add() writes to file_usage.
What is weird is that if I insert an image and then look at the file_usage table, the "module" and "type" are somehow overwritten: module becomes "file" and type becomes "node". In my custom module, no such overwriting takes place.
Anyway, file_get_file_references() for the image also returns an empty array, with or without the patch. Is it supposed to do that?

OK my tests with images were a bit confused - image_field_update_field() is only for default images, also to get the image with file_get_file_references() you have to pass the fourth argument. file_file_download() does that when called from image_file_download().

[EDIT: updated usages of file_usage_add() by contrib]

@Paul B: you are completely right with your remark in #29: only usages via the file module will be considered.

But what does this mean? I dove into how file_usage_add() is used:

• Image fields update usage via the file module, thus with 'file' for module.
• Image field settings defines a default image, this is stored under module 'image', but field settings are not an entity thus this usage should better be ignored if we want to return a multidimensional array keyed on (a.o.) entity type and id.
• Media updates usage under their own name, i.e. module='media'. These will be missed.
• File_entity does not call file_usage_add() itself but leaves that (correctly) up to the file module.
• Migrate correctly corrects user_save() that does not update usage on account creation.
• Feeds does add its own usage when importing a file because it keeps track of imported stuff (feeds_source table). However the type is the fetcher (class) type whereas the id is the source id. Thus not to be seen as an entity_type/id pair.
• Imce's use of file_usage_add() is bogus: if an image is uploaded via IMCE, a managed file is created and that managed file (type = 'file, id = $file->fid) is stored as a usage of ... itself. Thus this usage is also better ignored.
• Entityreference: if this module is used to create a field that can reference managed files, usage is not updated via file_usage_add(). But I guess we would like to get these usages. BTW: entityreference does specify foreign keys in the schemas of its fields.
• User pictures are correctly added as a usage. The field is named picture. However this is not specified as a foreign key in the schema, nor is it a field (it is a property of the user entity), so file_field_find_file_reference_column() will not find this.
• Webform records file submissions via webforms under 'webform' for module and, correctly, 'submission' as type with its submission id.
• I do not know about other modules, including Paul B's custom module.

What does this mean:

• file_usage_list() will return all references to managed files from file and image fields under the key 'file'.
• file_usage_list() will return references from media fields but these are not processed by the current patch.
• file_usage_list() will return references from user pictures but these are not processed by the current patch.
• file_usage_list() will return some other usages under module 'image', 'imce', 'webform', 'feeds' but these are better ignored.
• file_usage_list() will not return references from custom reference fields created via entityreference.
• file_get_file_references() is defined in file.module and should thus be considered a file field specific function. Remember that the file module does not define managed files (that is done in system), but a file field. In that sense this patch does what it should do.
• IMO, it should thus better be renamed to file_get_file_field_references(). At least it should be clearly documented that other usages than via file or image fields are not returned and these other usages include usages from the user module and contrib modules like media and entityreference.

I think it may be more of a private function, so it should be renamed to _file_get_file_references. Anyway, any bugfixes and improvements have to be added to Drupal 8 first, so I think they're out of scope for this issue. The fix added in #1805690-12: file_get_file_references() is rather bogus has already been added to Drupal 8, see #1997716: file_get_file_references() doesn't load entities correctly.

That would leave only the trailing whitespace issue I guess, however
if I understand #1452100: Private file download returns access denied, when file attached to revision other than current correctly, the new code doesn't work for revisions, and can't work in general, see #1452100-16: Private file download returns access denied, when file attached to revision other than current.

It shouldn't matter for file_file_download() since that function calls file_get_file_references() with FIELD_LOAD_CURRENT (but also see
#992674-22: Private file download returns access denied, but if the old code worked correctly that would still be a regression.

Yeah, let's do the remarks of #26 and update the function documentation that it is a file field module function and thus only returns file field references (and image field as that "extends" file field) and nothing else. Also renaming is out of scope for D7, so we also don't do that.

I've done the remarks of #26 and re-rolled the patch. Please review.

Re-rolled it again for 7.56

previous patch had an extraneous path component

Reroll. I'm not sure if SA-CORE-2017-003 affects this, but the tests should determine that.

Reroll against current 7.x

The entity field query in file_get_file_references() is unlimited and will bomb your site if you use a reference many times. Opened #3207669: file_get_file_references() can cause PHP out of memory errors for that small aspect.