Several places in Field-related code need to know if a object update is creating a new revision.
- file field's implementation of hook_field_update()
- patch RTBC in #629252: field_attach_form() should make available all field translations on submit

The code currently hardcodes a check on $object->revision.

2 options:

1) Nodes being the only 'revisionable' entity so far, we do have the latitude to state that the 'revision' property has a fixed conventional meaning, and that entity types with revisions need to make sure $object->revision is TRUE when a new revision is being saved.
If so, this needs to be documented - in hook_entity_info() ?

2) Like we do for 'properties with legacy' (object id = nid, uid, cid, tid...), we ask entity types to declare the name of the property with the 'new revision' semantic, through hook_entity_info()'s 'object keys' entry.

'object keys' being a way to workaround legacy differences that I still kind of hope we'll be able to attenuate in a D8 iteration of the entity API, I'd tend to lean towards 1) and not add another abstraction where it's not strictly needed.

No strong bias, though, opinions welcome.

CommentFileSizeAuthor
#5 drupal.entity-revision.5.patch1.84 KBsun

Comments

yched’s picture

yched commented
Component: field system » base system

bumping out of the 'Field API' bucket.

damien tournoud’s picture

damien tournoud commented

'object keys' being a way to workaround legacy differences that I still kind of hope we'll be able to attenuate in a D8 iteration of the entity API, I'd tend to lean towards 1) and not add another abstraction where it's not strictly needed.

I don't follow you here.

Let's take a step backward: SQL doesn't mandate any particular name for the primary key of a table, but that doesn't mean that the "concept" of a table is not abstracted. I don't see how letting each entity pick the names of those properties to match their own concept will make the entity API harder to implement.

yched’s picture

yched commented

I don't see how letting each entity pick the names of those properties to match their own concept will make the entity API harder to implement.

Well, having to go through code like 

list($id, $vid, $bundle) = entity_extract_ids($object)

or 

$info = entity_get_info('node');
$new_revision_key = $info['object keys']['new_revision'];
if (!empty($object->$new_revision_key)) {

to work with any object property where you want generic semantics attached (id, bundle name, language...) is convoluted (euphemism).

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

I'm slightly in favor of a new object key, mainly for consinstency reasons.

If we decide to go this way, we should keep in mind that

currently we have 'revision key' which seems off wrt 'id key'; we might want to introduce a new 'revision id key' which would correspond to 'vid' and use the current 'revison key' for 'revision'.
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Needs work
StatusFileSize
new drupal.entity-revision.5.patch1.84 KB

Whatever we do, please continue from this patch. (@see http://api.drupal.org/api/function/entity_extract_ids/7)

catch’s picture

catch
he/him
Primary language English
commented

subscribing.

casey’s picture

casey commented

While this isn't addressed in D7 I certainly hope any contrib module that implements revision-able entity types uses option 1 to indicate a entity will we saved as a new revision; that is using the $entity->revision key.

casey’s picture

casey commented

An $entity->age (ENTITY_CURRENT|ENTITY_REVISION) property would also be handy.

catch’s picture

catch
he/him
Primary language English
commented
Version: 7.x-dev » 8.x-dev

Moving to D8.

fago’s picture

fago
Primary language German
Location Vienna
commented

I do think for D8 every entity should be based upon a class implementing the "EntityInterface", which provides some simple methods to easily access identifiers, revision ids, labels, uris as well as save(), delete(), .. methods.

So, while we would have simple access to something like the revision identifier then, I do still think that we might want to standardize things like the revision property name. If it's easy to unify things, it helps us to avoid needless abstraction.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

@fago:

totally agreed

skwashd’s picture

skwashd commented

subscribing

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Have a look at #1723892: Support for revisions for entity save and delete operations, which adds revision CRUD support to the default storage controller and also adds a isNewRevision() method to EntityInterface. Haven't updated existing code yet, could be done here once that is in. Needs review :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Closed (duplicate)

Closing, entities have methods for this now.