Create an interface for revisionable entities

With the summary from above I do like (b), alhough I'm biased because I suggested something like that in the linked issue. This can be a fairly simple patch.

Any objections against b?

1) After looking at the exact meaning/german translation of revision/version, I agree that it might make sense to use version.

b) sounds good to me as well. Maybe we can at some point (9.x), when we can rely on traits, revisit a) as it would be easy to include default implementations of the revision methods as a trait.

I guess to replace entity_extract_ids(), it's enough to being able to extract the current revision id, which this issue could do, a entity-generic revision implementation can be a follow-up. So this should be rather trivial?

I need more help for the node_revision stuff. How do you implement that method?

Probably not there yet and we need some tests for this. Thats everything I can do. The build query stuff looks a bit like black magic :)

Fetching the current revision id in buildQuery() was my suggestion, it doesn't cost us anything (well, a tiny but, but imho not a problem).

A function that currently does a is current revision check is node_delete_revision(). And it does a second node_load() to figure it out. I think we could already replace that check with a call to isCurrentVersion() to have some real usage of the new API.

Agreed that we need some tests here but other than that, this looks quite ok to me.

New try with tests.

When creating the tests I noticed that when doing a normal node_load without a vid key that node version id is always the current version id. I discussed this with Berder and we concluded this is ok.

<berdir> aspilicious: but you have old objects
<berdir> aspilicious: at the time the object was loaded, it *was* the active revision
...
<berdir> aspilicious: if you change the label but call $node->labe(), it is outdated
..
<berdir> aspilicious: you have the concurrency problem anyway, unless you do locking
<berdir> aspilicious: another process could change it between your check and the action that you are doing based on that check

To clarify this a bit, $nodes contained the original node objects after they were created. So current_vid always equals vid, because by the time it was loaded, it *was* the current vid.

IMHO, that behavior is ok, because it's the same for all method calls. If the second revision would change the label, the first object would still return the old label when calling $node->label(), so that's why we need to load them again.

Also, what we need to do, I guess in NodeStorageController for now, is making sure that current_vid/vid are correct after doing a save. Right now, if you would do a $entity->save() either for the first time or if a new revision is created, it would return FALSE.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionsTest.phpundefined
@@ -75,6 +75,9 @@ class NodeRevisionsTest extends NodeTestBase {
 
+    // Confirm that this is the current version
+    $this->assertTrue($node->isCurrentVersion(), 'Third node revision is the active one.');
+
     // Confirm that revisions revert properly.
     $this->drupalPost("node/$node->nid/revisions/{$nodes[1]->vid}/revert", array(), t('Revert'));
     $this->assertRaw(t('@type %title has been reverted back to the revision from %revision-date.',
@@ -83,6 +86,10 @@ class NodeRevisionsTest extends NodeTestBase {

@@ -83,6 +86,10 @@ class NodeRevisionsTest extends NodeTestBase {
     $reverted_node = node_load($node->nid);
     $this->assertTrue(($nodes[1]->body[LANGUAGE_NOT_SPECIFIED][0]['value'] == $reverted_node->body[LANGUAGE_NOT_SPECIFIED][0]['value']), t('Node reverted correctly.'));
 
+    //Confirm that this is not the current version

Second comment needs a space, both need a . :)

Another idea:

If we make the current version id property name static (e.g. current_version_id), then we could have a default implementation that returns TRUE if there is no revision key, and if not, compares $this->getVersionid() to $this->current_version_id. Then we don't need a custom implementation of that for every entity that supports revisions.

Thinking about that, if we go with the revision => version rename, we also need to update the corresponding hook_entity_info() properties. Can of course happen in a follow up, or while writing a generic Version save/delete implementation.

Ok, here's a re-roll.

- Fixed the comments.
- Went back to getRevisionId(). I agree that version is the better term but it makes *no* sense to have getVersionId() and 142 references to revision in *entity.module alone*. Changing all of them would make the page *huge*, which is not necessary at this point IMHO.
- Updated the sql magic to an expression that automatically sets isCurrentRevision.
- See @todo in NodeStorageController, is it really so that saving a new revision always forcefully makes this the active one. I thought that in 7.x contrib it's possible to have newer, non-active revisions. Do they reset the current revision after save or did we introduce a regression there?

Revision munging in D7 regressed compared to D6, there's some background at #1184318: Avoid using vid munging for pending revisions. However we should worry about that in #218755: Support revisions in different states I think.

#10: version-id-interface-1612014-10.patch queued for re-testing.

Simple re-roll, git++.

Changed all comments to current and replaced the @todo with a comment that explains what it does.

#16: revision-id-interface-1612014-16.patch queued for re-testing.

As catch mentioned in #11, the patch in #218755: Support revisions in different states deals with similar functionality and uses slightly different property names. Currently that patch is dealing directly with an "isLive" property. I like that the interface here uses methods to determine isCurrentRevision(). Perhaps there needs to be a separate method for "MakeCurrentRevision()" for use in entity saving. For instance, if a user loads an old revision into an edit form, makes changes, and wants to make those changes live/current, then isCurrentRevision() and MakeCurrentRevision() would return FALSE and TRUE respectively until the actual database writing happens. Does that make sense?

There is no point in discussion the same thing in two issues. Can we push this is and revise it in one issue if needed?

tags...

I reviewed this patch and it is a simple and handy extension to the API. Committed to 8.x. Thanks.