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;
}
Comment | File | Size | Author |
---|---|---|---|
#5 | file-usage-list-defective-1222576-5.patch | 2.56 KB | jox |
#3 | file-usage-list-defective-1222576-3.patch | 478 bytes | jox |
Comments
Comment #1
jox CreditAttribution: jox commentedI 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?
Comment #2
aaron CreditAttribution: aaron commentedneeds a patch before it can be reviewed.
Comment #3
jox CreditAttribution: jox commentedHere is a patch for D8.
Comment #5
jox CreditAttribution: jox commentedSome tests need adjustments as well. Here is a new patch with the following changes:
Comment #6
jox CreditAttribution: jox commentedComment #7
mirzu CreditAttribution: mirzu commentedThe original function is in fact defective and the patch does indeed fix the issue.
Comment #8
aaron CreditAttribution: aaron commentedworks great, thanks @jox!
Comment #9
Dries CreditAttribution: Dries commentedCommitted 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.
Comment #10
aaron CreditAttribution: aaron commented#5: file-usage-list-defective-1222576-5.patch queued for re-testing.
Comment #11
webchickaaron, quicksketch, pwolanin, etc: can you provide some insight as to the impact in D7 contributed modules if we made this change in core?
Comment #12
jox CreditAttribution: jox commentedOut 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)].
Comment #13
aaron CreditAttribution: aaron commented@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.
Comment #14
aaron CreditAttribution: aaron commentedand 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...
Comment #15
jox CreditAttribution: jox commentedNot 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:
Now, exactly this is not the default behaviour.
Reading further:
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...
...better be the following?
In my eyes one of the following could be improved:
Comment #16
aaron CreditAttribution: aaron commentedalso, 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.
Comment #17
jox CreditAttribution: jox commented@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.
Comment #18
webchickCool, 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.
Comment #19
catchComment #20
webchickAnd since adding that summary is relatively easy, tagging as Novice.
Comment #21
aspilicious CreditAttribution: aspilicious commentedNeeds review and probably a text update by a someone that can actually write english but it's a start http://drupal.org/node/1339434
Comment #22
bithin CreditAttribution: bithin commented#3: file-usage-list-defective-1222576-3.patch queued for re-testing.
Comment #23
aspilicious CreditAttribution: aspilicious commentedYou 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.
Comment #24
bithin CreditAttribution: bithin commented#5: file-usage-list-defective-1222576-5.patch queued for re-testing.
Comment #26
aspilicious CreditAttribution: aspilicious commentedSTOP retesting, its NOT needed.
Back to needs review for the notification...
Comment #27
xjmAdded more detail to the change notice, as well as references to the function's API documentation.
Comment #28
xjm