After a file-entity is saved via the UI, fields that reference the file in other entities (e.g. node, user, taxonomy term) have stale data in the cache_field cache bin/table.

CommentFileSizeAuthor
#11 1431070-11-file-save-invalidate-field-caches.patch.txt2.67 KBBevan
#7 1431070-file-save-invalidate-field-caches.patch2.74 KBBevan
#10 1431070-file-save-invalidate-field-caches.patch2.74 KBdave reid
#8 1431070-8-file-save-invalidate-field-caches.patch2.67 KBBevan
#6 1431070-file-save-invalidate-field-caches.patch2.74 KBdave reid
#4 1431070-1-cache_field_for_entities_that_reference_file_is_not_cleared.patch938 bytesBevan
#1 1431070.patch0 bytesBevan

Comments

Bevan’s picture

Bevan commented
Status: Needs work » Needs review
StatusFileSize
new 1431070.patch0 bytes

This patch fixes the issue by looking for references of the file using file_get_file_references() and then clears the respective cached row for each entity that references the file. There may be more performant and/or simpler ways to achieve this.

Note that file_get_file_references()'s @return documentation is incorrect. See #993082: file_get_file_references() @return doc totally wrong.

In this patch I also pose the question of whether array('file', 'image') should be defined in a constant, variable or alterable info-hook, since file_entity_field_formatter_info() also uses this hard-coded array. Or is there already a better way of doing this which I am unaware of?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

I have a feeling that we should just be issuing a cache_clear_all() to clear block and page caches, in addition to the field caches since the file_usage table is not very accurage for keeping track of files (media embedded in wysiwyg).

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs review » Needs work

Note this is the solution used by node_form_submit(), comment_form_submit(), taxonomy_form_term_submit(), and taxonomy_vocabulary_save(), so yeah, I think just a wide-sweeping cache_clear_all() should be good here.

Bevan’s picture

Bevan commented
Status: Needs work » Needs review
StatusFileSize
new 1431070-1-cache_field_for_entities_that_reference_file_is_not_cleared.patch938 bytes

Dave,

cache_clear_all() clears cache_block and cache_page only. So even if we call cache_clear_all() after updating a File via it's File Entity form, the stale data remains cache_field.

This patch adds a call to cache_clear_all(), but does not remove the code introduced in the other patch.

I am not convinced this patch is any better than the first patch because File Entity properties are not displayed anywhere in a typical site and are actually non-trivial to add to pages and blocks (AFAIK) except via views which has it's own cache management. This patch may in fact may be worse than the first patch due the (possibly unnecessary) toll on performance.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Version: 7.x-2.0-unstable3 » 7.x-2.x-dev

Yeah I see what you mean. I've filed #1446464: Get rid of file_field_load() - shouldn't load all file object data with field load - only file ID to fix this in D8 core since adding this type of data to the field cache is absolutely dumb, and since node reference and user reference fields don't call entity load in hook_field_load() this isn't a problem for those modules.

We might actually be able to fetch a list of entity types and IDs from the {file_usage} table rather than using file_get_file_references() - which should help speed this up since we don't have to query the field tables and will support anything that adds itself into the usage table.

File Entity properties are not displayed anywhere in a typical site

We're commonly recommending that people use the 'Rendered file' formatter for file and image reference fields, which would include field data output as well, so it would still be handy to clear the page and block caches.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
StatusFileSize
new 1431070-file-save-invalidate-field-caches.patch2.74 KB

Revised patch which queries file_usage directly, and gathers an array of CIDs to pass to cache_clear_all() to make the operation faster with lots of entities.

Bevan’s picture

Bevan commented
StatusFileSize
new 1431070-file-save-invalidate-field-caches.patch2.74 KB

The patch references a variable which is never set $cached_entity_types. I assume it should be $entity_types. The attached patch makes that change and that change only. Otherwise the patch looks good and works great!

Bevan’s picture

Bevan commented
StatusFileSize
new 1431070-8-file-save-invalidate-field-caches.patch2.67 KB

This patch is the same but for 2.0-unstable3

Status: Needs review » Needs work

The last submitted patch, 1431070-8-file-save-invalidate-field-caches.patch, failed testing.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review
StatusFileSize
new 1431070-file-save-invalidate-field-caches.patch2.74 KB

Thanks, fixed that one thing against 7.x-2.x. Patches against tags really aren't useful at all.

Bevan’s picture

Bevan commented
StatusFileSize
new 1431070-11-file-save-invalidate-field-caches.patch.txt2.67 KB

I think #10 was the same as #7.

This is the same as #10 but for 2.0-unstable3, which is mostly only useful as a reference for a project I am working on which does not want to chase HEAD of File Entity. I think if I give it a file extension other than ".patch" it won't get tested.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs review » Reviewed & tested by the community

Oh thanks, I didn't see you had rolled it against 7.x-2.x as well. I'll get this committed shortly.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Reviewed & tested by the community » Fixed

Committed this to 7.x-2.x with http://drupalcode.org/project/file_entity.git/commit/4db2744 and I gave you the credit for commit Bevan.

Status: Fixed » Closed (fixed)

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