file_field_update() (file field's implementation of hook_field_update()) checks if a value has been removed from the previous values, to keep track of file references.

It hardocodes a check on $object->revision. See #644332: Do we need to abstract $object->revision ('new revision') property about that.

More problematic, to compare new values with previous values, it does a full load the current object.

a) I'm not sure the current code is the correct way of doing that - could use the opinion of an entity API guru.

  $load_function = $obj_type . '_load';
  $original = $load_function($id);

(this is the main reason I'm marking this 'critical')

b) entity_save() triggering an entity_load() doesn't sound too great. Reacting on values being removed is a legitimate use case for 'resource reference' field types, I'm wondering if we can make this better.

CommentFileSizeAuthor
#7 file_field_load_on_update-644338-7.patch1.08 KByched
#5 file_field_load_on_update.patch1.06 KByched

Comments

Anonymous’s picture

Anonymous (not verified) commented

subscribe. still getting my head around the fields code, came across this function here #618654: File and image fields are treated as temporary files and automatically deleted after six hours.

sun.core’s picture

sun.core commented
Priority: Critical » Normal

Not critical. Please read and understand Priority levels of Issues, thanks.

catch’s picture

catch
he/him
Primary language English
commented

#733756: image_field_insert() makes no sense and causes a memory leak with devel_generate() was a memory leak on inserts caused by this, although that was fixed by completely removing image_field_insert().

Subscribing to this one.

yched’s picture

yched commented

I think that instead of a full entity load, the code could use field_attach_load() with $options = array('field_id' => $field['id']) to get the current values of the field for the object. This is guaranteed to work whatever the entity type, and would avoid triggering a full entity load() during an entity update().

yched’s picture

yched commented
Status: Active » Needs review
StatusFileSize
new file_field_load_on_update.patch1.06 KB

Quick patch, not tested - I don't know if this part is tested in the current test suite, for that matter.

Status: Needs review » Needs work

The last submitted patch, file_field_load_on_update.patch, failed testing.

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new file_field_load_on_update-644338-7.patch1.08 KB

Oops, f_a_load() works by altering the entity by ref.

catch’s picture

catch
he/him
Primary language English
commented

That works for me, do we want to defer the $revision stuff to #644332: Do we need to abstract $object->revision ('new revision') property?

yched’s picture

yched commented
Title: fragile code in file_field_update() » file_field_update() triggers a full entity load durung entity_save()

Yes, that other issue has more discussions on that topic.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

RTBC, then.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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