Wrong usage of hook_file_references

+++ modules/user/user.module	19 Mar 2010 19:25:16 -0000
@@ -811,7 +811,7 @@ function user_file_references($file) {
   $file_used = (bool) db_query_range('SELECT 1 FROM {users} WHERE picture = :fid', 0, 1, array(':fid' => $file->fid))->fetchField();
   if ($file_used) {
     // Return the name of the module and how many references it has to the file.
-    return array('user' => $count);
+    return array('user' => 1);

Wouldn't a query like db_result('select count(*) from {users} where...')); be better, and then actually return the result? Isn't the hook supposed to return the actual number of references?

I know that in core, it's always going to be one, given current assumptions. But there's no reason this shouldn't be a more "sane" query.

150 critical left. Go review some!

This looks correct to me. Marking RTBC so I can commit it later.

Committed to CVS HEAD.

This patch was an almost direct revert of #196862: COUNT(*) is an expensive query in InnoDB., please re-revert.

Sorry, @andypost :-(

@catch, shouldn't we change the signature of hook_file_references to return a bool, then? We aren't going to want to ever return a count are we? In PHP, this is really just a change to the documentation, I think, but we should do that. The hook should return TRUE if there are existing files.

err. cross posted with above comment ...

What is the appropriate way to do this with respect to InnoDB? Store a variable with the count? Select all rows and count them in PHP?

hook_file_references() says that you should return a count of the total number of uses of the file.

As @rfay points out in #1, given current assumptions in core just returning 1 or 0 will work but does that potentially break things for contrib?

That makes sense to me. I can't see a use-case for returning a count here.

// If file still user then 1 is enough to indicate.
This should be // If file is still used then 1 is enough to indicate this.
Otherwise RTBC.

This looks good. We should open a separate issue to change hook_file_references() documentation. Although there's also #353458: hook_file_references() was not designed for a highly flexible field storage.

I think we'll be accidentally rewriting this function forever if we don't change the signature of hook_file_references(). @catch, you're the keeper of this. Should hook_file_references() return a bool?

I think it should, but I have zero knowledge of what that field storage issue is proposing and how that'd interact with the changes here.

Sorry about that! I committed the updated patch to HEAD. Thanks.