file_field_insert() and file_field_update() both use the same code, which relies on $object->is_new to differentiate between insert and update.
$object->is_new is not standard across entity types. Works for nodes and users, but comments and taxo terms don't have that 'is_new' pattern. See #622534-21: Cleanup in hook_field_attach_*() for a related discussion.

+ minor: in the same line, $object->revision is also node specific. Unlike 'id key' or 'revision id key', the entity API sets no abstraction on a 'is this a new revision' key. Not *that* bad since we have no legacy here. Nodes are currently the only entities with revisions, it's not sure more will come up in contrib, and if so they'd need to use 'revision' for this key... Might deserve a note in hook_entity_info() that this $object->revision key is promoted to a standard on revisionable entities.

CommentFileSizeAuthor
#3 627626-file-field-is-new.patch1.14 KBdamien tournoud

Comments

jim0203’s picture

jim0203 commented

subscribbling

damien tournoud’s picture

damien tournoud commented
Title: Node-specific code » Node-specific code in file field

Here is the warning you get when adding a file field to a taxonomy term, to try to limit duplicate issues by alpha testers:

Notice: Undefined property: stdClass::$is_new in file_field_update() (line 260 of modules/file/file.field.inc).
damien tournoud’s picture

damien tournoud commented
Status: Active » Needs review
StatusFileSize
new 627626-file-field-is-new.patch1.14 KB

The fact is, $object->is_new is exactly the same as "I was called from hook_field_insert()" now (it wasn't the same in D6), so the call from hook_field_insert() is actually not needed.

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community

Sounds reasonable.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

An issue I had while making privatemsg.module fieldable was that the same function assumes that there is a load function called $obj_type . '_load'. I'm not sure if it's save to assume that...

$load_function = $obj_type . '_load';
$original = $load_function($id);
if (!empty($original->{$field['field_name']}[$langcode])) {

I was able to work around it be renaming my object type from privatemsg to privatemsg_message which is probably better anyway, I'm just unsure if there will always be a _load function that works like that function assumes. From what I could see, that function is the only place where such an assumption is made. If yes, we should document it somewhere.

EDIT: Ok, this is already discussed in #644338: file_field_update() triggers a full entity load durung entity_save()

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.