file_usage_list() (file.inc) is supposed to return a list of items that use a given file. But it will in fact never return more then one usage of a certain module and type even if several exist.

This is because if multiple items of a certain module and type, use the same file, they get all keyed by the same values (module and type) and overwrite each other. Only the last item in the loop will be returned:

// Excerpt from the original file_usage_list()
foreach ($result as $usage) {
  $references[$usage->module][$usage->type] = array('id' => $usage->id, 'count' => $usage->count);
}
return $references;

One example where this occurs is a node that is translated and whose images are synchronized to the other languages.

To avoid this, the id could be used as another key.

// Modification
foreach ($result as $usage) {
  $references[$usage->module][$usage->type][$usage->id] = $usage->count;
}
return $references;

This would of course change the API. However the only use of this function in core, from what I have investigated, is to determine if a file is used by any entity at all. (This might be the reason why this has not been discovered so far.)

There might be modules relying on the current behaviour as well (and doing more than just checking in a boolean way). But then they're possibly faulty.

Relying on the current behaviour can lead to inconsistencies. Suppose a file_managed record is changed programatically and nodes that refer to that file (determined with file_usage_list()) need to get updated. Only one (random) node of multiple referring nodes would get updated. The others would be left with broken references.

I don't know how this would get fixed in compliance with the core development workflow/policies (regarding the API change).

One compromise would be to extend the existing file_usage_list() with an additional parameter (e.g. $keyed_id) which defaults to the old behavior. This way the old functionality is preserved and the new one could be enabled when needed. Like this:

// $keyed_id is the new parameter
function file_usage_list(stdClass $file, $keyed_id=false) {
  $result = db_select('file_usage', 'f')
    ->fields('f', array('module', 'type', 'id', 'count'))
    ->condition('fid', $file->fid)
    ->condition('count', 0, '>')
    ->execute();
  $references = array();
  foreach ($result as $usage) {
    if ($keyed_id) {
      // This builds the proper structure
      $references[$usage->module][$usage->type][$usage->id] = $usage->count;
    } else {
      $references[$usage->module][$usage->type] = array('id' => $usage->id, 'count' => $usage->count);
    }
  }
  return $references;
}

I leave it like this for discussion. If desired I can provide a patch for whatever solution that might be chosen.

For reference, here is the original file_usage_list() (file.inc):

/**
 * Determines where a file is used.
 *
 * @param $file
 *   A file object.
 *
 * @return
 *   A nested array with usage data. The first level is keyed by module name,
 *   the second by object type, the third has 'id' and 'count' keys.
 *
 * @see file_usage_add()
 * @see file_usage_delete()
 */
function file_usage_list(stdClass $file) {
  $result = db_select('file_usage', 'f')
    ->fields('f', array('module', 'type', 'id', 'count'))
    ->condition('fid', $file->fid)
    ->condition('count', 0, '>')
    ->execute();
  $references = array();
  foreach ($result as $usage) {
    $references[$usage->module][$usage->type] = array('id' => $usage->id, 'count' => $usage->count);
  }
  return $references;
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jox’s picture

Title: file_usage_list() might return incomplete results » file_usage_list() is defective (might return incomplete results)
Version: 7.x-dev » 8.x-dev

I changed the version to 8.x-dev since this applies to D8 as well.

Maybe it can be fundamentally fixed in D8 and the extended function could go into D7?

aaron’s picture

Status: Needs review » Needs work

needs a patch before it can be reviewed.

jox’s picture

Status: Needs work » Needs review
FileSize
478 bytes

Here is a patch for D8.

Status: Needs review » Needs work

The last submitted patch, file-usage-list-defective-1222576-3.patch, failed testing.

jox’s picture

Assigned: Unassigned » jox
Status: Needs work » Needs review
FileSize
2.56 KB

Some tests need adjustments as well. Here is a new patch with the following changes:

  • Fixed function file_usage_list() to return a proper usage list.
  • Changed the (doxygen) docs for file_usage_list() to reflect the new behaviour.
  • Altered tests: FileUsageTest, FileDeleteTest to adapt to the new behaviour.
jox’s picture

Assigned: jox » Unassigned
mirzu’s picture

The original function is in fact defective and the patch does indeed fix the issue.

aaron’s picture

Status: Needs review » Reviewed & tested by the community

works great, thanks @jox!

Dries’s picture

Version: 8.x-dev » 7.x-dev

Committed to 8.x. We should back-port this to 7.x so moving the version for webchick's consideration. It is an API change but one that may be inevitable.

aaron’s picture

webchick’s picture

aaron, quicksketch, pwolanin, etc: can you provide some insight as to the impact in D7 contributed modules if we made this change in core?

jox’s picture

Out of the 8029 projects from http://drupal.org/project/usage there is 2049 projects that have some kind of 7.x version.

2 of these 2049 D7-projects make use of file_usage_list(): media and imce

Both projects would not be affected by the change (they only check existence of usage, not the details).

[My own module File manager needs the fixed version of file_usage_list() but it's sandbox and still waiting for approval (it contains a fixed version of that function)].

aaron’s picture

@webchick: i found the same thing that @jox reported. if anything, this patch would improve functionality in contrib, as both of these modules are (from what i can tell at http://drupalcontrib.org/api/drupal/contributions--imce--inc--imce.page.... ) using the function as intended by its dox.

aaron’s picture

and actually, from looking at that imce usage, it looks like this would pro-actively fix a bug just waiting for an issue in its queue...

jox’s picture

Not to lessen the need for my fix. But from what I can tell it would not make any difference for these modules.

However, by looking at file_usage_delete() I noticed that it has some potential to produce inconsistencies.

The docs say:

Removes a record to indicate that a module is no longer using a file.
The file_delete() function is typically called after removing a file usage to remove the record from the file_managed table and delete the file itself.

Now, exactly this is not the default behaviour.

Reading further:

$count (optional) 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.

This tells that to accomplish the preceding, you actually have to pass 0 as $count.

Now, in the case of imce, shouldn't the line...

file_usage_delete($file, 'imce');

...better be the following?

file_usage_delete($file, 'imce', NULL, NULL, 0);

In my eyes one of the following could be improved:

  • The docs of file_usage_delete() (e.g. say "Removes references of a record (or optionally all)..." instead of "Removes a record..." etc.)
  • The default behavior of file_usage_delete() (default $count to 0)
aaron’s picture

also, views currently makes use of the {file_usage} table, which means that it's actually using the system the way it currently is meant. we need this to go in, to ensure the correct functionality is present for any other future needs.

jox’s picture

@aaron: I took a look at it. It seems like this will not be a problem. The change only affects the return value of file_usage_list(), it does not change the way the data is stored or handled elsewhere.

Even though Views touches the file_usage table, it does not use the file_usage_list() function (yet?). So there should be no risk.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7, +Needs change record

Cool, thanks for the feedback, folks. I'll go ahead and commit this early, so there's time to catch any fall-out issues before the next point release of D7.

Committed and pushed to 7.x.

We need an API change notification for this, to alert module developers on what to do. Tagging and marking needs work.

catch’s picture

Title: file_usage_list() is defective (might return incomplete results) » API change notification for file_usage_list() is defective (might return incomplete results)
Category: bug » task
Priority: Normal » Critical
Status: Needs work » Active
webchick’s picture

Issue tags: +Novice

And since adding that summary is relatively easy, tagging as Novice.

aspilicious’s picture

Status: Active » Needs review

Needs review and probably a text update by a someone that can actually write english but it's a start http://drupal.org/node/1339434

bithin’s picture

aspilicious’s picture

You don't need to retest the patch... I'm asking a review for the text in http://drupal.org/node/1339434. This retest will fail as the patch is alrdy applied to head.

bithin’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +Needs backport to D7, +Needs change record

The last submitted patch, file-usage-list-defective-1222576-5.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

STOP retesting, its NOT needed.

Back to needs review for the notification...

xjm’s picture

Status: Needs review » Fixed

Added more detail to the change notice, as well as references to the function's API documentation.

xjm’s picture

Title: API change notification for file_usage_list() is defective (might return incomplete results) » file_usage_list() is defective (might return incomplete results)
Issue tags: -Novice, -Needs change record

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