file_get_file_references() is completely broken

Subscribe, remembering to take a look at this later.

It looks to me like the problem is line #973 of file.module

$references[$field_name] = field_attach_query($file_field['id'], array(array('fid', $file->fid)), array('limit' => FIELD_QUERY_NO_LIMIT, 'cursor' => &$cursor, 'age'=> $age));

Which occurs inside of file_get_file_references() which is called in a round about way when updating the term as it goes through and checks for file references.

Removing the $age parameter from the line above makes the problem go away. The documentation for field_attach_query() states that this parameter is for internal use only and that field_attach_query_revisions() should be used instead. Changing to that function doesn't fix anything however.

This is where my knowledge of what is going on here starts to break down.

Also, possibly worth noting is that changing the $age parameter from FIELD_LOAD_REVISION of FIELD_LOAD_CURRENT fixes the problem as well.

file_get_file_references() is never called in core using FIELD_LOAD_CURRENT though that is the default value of the $age parameter. Possible cleanup task here.

Finally, looks like media module ran into this problem as well. #739310: File References error

Check the relevant revision tables in your database. They're most likely empty, but they shouldn't be empty?

FIELD_LOAD_CURRENT works because it queries the base table. FIELD_LOAD_REVISION queries the revision table, which is empty, so field_attach_query() rightly returns an empty result and therefore the reference count ends up equaling 0 triggering the error tanoshimi is getting.

I hypothesis that the image module, more specifically image.field.inc, needs to write out to the revision table when new/existing image fields are saved/updated. I have yet to write any proof of concept code to demonstrate it does indeed solve the problem.

    // Execute the query if we have values to insert.
    if ($do_insert) {
      $query->execute();
      if (isset($vid)) {
        $revision_query->execute();
      }
    }

field_sql_storage.module doesn't write to the revision table unless the entity supports revisions, so this is completely broken. file.module
likely needs to query first the current storage, then the revisions table, to see if either have references to the file.

If this wasn't such a nasty bug it ought to be a duplicate of or postponed on #353458: hook_file_references() was not designed for a highly flexible field storage or #780154: There is no listing API for field API.

+1 catch. After I looked into it further I realized my post in #4 was totally wrong.

catch's suggestion seems reasonable to me, should be an easy patch?

I think we should actuall save revisions for all entities all the time for consistency, node revisions are a bit broken if you look at the details etc., but that's all Drupal 8 work, so for now doing the current query first ought indeed to be a fairly quick patch.

catch, here's an idea. Revision tables seem to be always created (see below), even for entities that don't support revisions. So why not just write out a revision like you said, for consistency. Either way is going to be a kludge. I imagine if we change file_get_references we'd be ignoring the $age argument. Plus querying two tables (current + revision) which really sucks.

I'm not sure if this patch is going to work, but I figure I'd offer it up as an alternative. I mean it does fix the issue for me and it passes the few tests I've ran against it. But I have no idea if there are unintended side-effects with this approach. We'll see if it passes testbot.

snippet:

 function _field_sql_storage_schema($field) {

 // Construct the revision table. The primary key includes
  // revision_id but not entity_id so that multiple revision loads can
  // use the IN operator.
  $revision = $current;
  $revision['description'] = "Revision archive storage for {$deleted}field {$field['id']} ({$field['field_name']})";
  $revision['revision_id']['description'] = 'The entity revision id this data is attached to';
  $revision['primary key'] = array('etid', 'revision_id', 'deleted', 'delta', 'language');

  return array(
    _field_sql_storage_tablename($field) => $current,
    _field_sql_storage_revision_tablename($field) => $revision,
  );
}

I think that could cause issues for entities which we subsequently add revisions for (which I'm hoping to do in Drupal 8). And saving the field query doesn't justify the extra writes here IMO. Since hook_file_references() is going to be refactored in another, also critical issue, I'd suggest we just add the field query here, which is an easier change to revert in a refactor than changing field_sql_storage.module

Ah ok, well in that case shouldn't this wait until #780154: There is no listing API for field API is resolved?

Works for me.

This bug still exists after commited #780154: There is no listing API for field API

Also duplicates
with patch #718048: file_managed_file_validate does not throw error on correct count
#793386: file_get_file_reference_count() very broken, probably untested

Retitling. It took me some time to find the correct function.

I agree with #8: the storage should always store an entry in the revision table even if the entity doesn't support revisions. Sounds like a no-brainer to me.

If we don't store an entry in the revision table for entities that doesn't support revision, you cannot possibly get all the fields that have or had a value, and Views will be affected by such a madness.

I really think the only possible route here is to change the storage and always store revisions. After all, we already do store two copies when the entity supports revisions.

I concur. field sql storage as far as i can see has no notion of 'supporting revisions' or not, it always acts on both, storage_schema, update field, delete field, create field, the works. the only distinction is in storage write -- no vid, no record. that's easy to fix and hurts noone.

reroll.

+++ modules/field/modules/field_sql_storage/field_sql_storage.module
@@ -420,22 +419,18 @@ function field_sql_storage_field_storage_write($entity_type, $entity, $op, $fiel
+    } ¶

trailing whitespace :)

Powered by Dreditor.

I missed that one so far. Subscribe for now...

Added a low-level test that fields can be reached via FIELD_LOAD_REVISION even if they are attached to entities that don't support revisions.

Edit: and of course we cannot use 0 for the revision_id, because that's global to the entity type.

Added a high level test in file.test: we try to attach the file to a user (an entity that doesn't support revisions) and see if it is works properly. This test properly fail when the patch to the SQL storage is not applied.

Looks good to me. I'd RTBC but I'm probably not the right person to RTBC this.

No sure what I was smoking in #9, different storage based on revision support makes no sense anyway, patch looks good, adds and passes tests, RTBC.

Looks good. Committed to CVS HEAD.