Support for revisions for entity save and delete operations

...coming from #1082292: Clean up/refactor revisions

+++ b/core/modules/entity/lib/Drupal/entity/DatabaseStorageController.php
@@ -479,6 +493,42 @@ class DatabaseStorageController implements EntityStorageControllerInterface {
+    $revision = clone $entity;

Having a clone here looks awkward. If the object is just for preparing the storage of the revision, it should be named like that and probably be a plain array of values ora stdclass object. E.g. $record or $storage_record?

+++ b/core/modules/entity/lib/Drupal/entity/DatabaseStorageController.php
@@ -479,6 +493,42 @@ class DatabaseStorageController implements EntityStorageControllerInterface {
+    // ensure that a new revision will actually be created, then store the old
+    // revision ID in a separate property for use by hook implementations.
+    if ($revision->isNewRevision() && $revision->{$this->revisionKey}) {
+      $revision->{'old_' . $this->revisionKey} = $revision->getRevisionId();
+      $revision->{$this->revisionKey} = NULL;
+    }

I don't think we still need this old_revisionkey. For that $entity->original should work, shouldn't it? Or doesn't that contain the right revision? If so, I'd say that's what we want to fix: $original should be the previous version of whatever we are saving.

+++ b/core/modules/entity/lib/Drupal/entity/EntityInterface.php
@@ -46,6 +46,26 @@ interface EntityInterface {
+  /**
+   * Enforces an entity to be saved as a new revision.
+   *

Why is that enforcing something? Isn't it just setting the new revision flag? So maybe, it could be
just $entity->setNewRevision(); ?

+++ b/core/modules/node/lib/Drupal/node/NodeStorageController.php
@@ -250,13 +97,42 @@ class NodeStorageController extends DatabaseStorageController {
+    if (!empty($node->revision)) {
+      $node->enforceNewRevision(TRUE);
+    }

I do think this should be part of the form code. If the revision checkbox is set, use the method. But I don't think we should still have $node->revision as the API is the method.

+++ b/core/modules/node/node.admin.incundefined
@@ -471,7 +471,6 @@ function node_admin_nodes() {
-    ->addTag('node_access')

Seems to me as an unintentional deletion.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -321,6 +321,11 @@ class NodeFormController extends EntityFormController {
+    // Save as a new revision if requested to do so.
+    if (!empty($node->revision)) {
+      $node->setNewRevision();

Use $form_state['values'] instead

/modules/file/file.field.inc file_field_update() still uses !empty($entity->revision) instead of $entity->isNewRevision().

Needs a re-roll, I'm working on that right now.

Okay, here we go. I re-rolled the patch, let's see if it passes the tests.
I fixed some coding standard violations at the same time.

For now the only part I'm a bit concerned about is this:

...
    elseif (!isset($record['log']) || $record['log'] === '') {
      // If we are updating an existing node without adding a new revision, we
      // need to make sure $node->log is unset whenever it is empty. As long as
...

At first it looks like the condition could be adjusted to simply use empty(), even the comment below the condition implicitly says that. But according to Sascha, who I bothered with this, using empty() would lead to failing tests.
Shall we improve the comment or just hope that nobody has the idea to clean this up?

Patch does not apply any more :/

Also, this does not cover deleting yet. Looks like this does not even use the controller yet:
http://api.drupal.org/api/drupal/core!modules!node!node.module/function/...

I've worked on the entity api module's revision support today, patch over at http://drupal.org/node/996696#comment-6433746. This incorporates already #218755: Support revisions in different states as well as code of this patch - maybe it's useful for this patch as well.

Please add a Summary. Thanks

If this is a new hook, it needs docs: $this->invokeHook('revision_delete', $revision);

// When saving a new revision, set any existing revision ID to NULL
This is not good for data migration between Drupal sites (e.g. D7 => D8). Can we think of a way to honor a revision_id if caller provided one? Nodes and Users currently let you specify a nid/uid so I'm hoping that we can do similar here. This is probably worthy of a follow-up, and shouldn't block this patch.

$entity->use_provided_revision_id. Looks like this is a new, magic key :). We should document it

typo: deleeted

I just looked for the equivalent logic when saving nodes/users with specific IDs and it might have been removed in D8 (including the tests for it) which really sucks. Anyway, what D7 does is honor a specified id when an id is passed AND an is_new flag is set. I don't know if it makes sense to do that here. If it doesn't, your approach looks fine to me.

Looks like all my concerns were addressed, and bot is happy.

thx

I agree that this is more of a task, since we're generalizing existing functionality from one entity type to all entities. The diffstat illustrates this. :)

Back to RTBC. This sat at RTBC for a week. Lets please get it in soon.

I'm leaving this RTBC but it'd be good to understand the following (apart from the initial nitpick) a bit better since it's not explained well in the patch.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -507,6 +520,47 @@ public function save(EntityInterface $entity) {
+   * Saves a node revision.
+   *
+   * @param Drupal\entity\EntityInterface $node

s/entity/node/ ?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -507,6 +520,47 @@ public function save(EntityInterface $entity) {
+    // Convert the entity into a array as it might not have the same properties
+    // as the entity, it is just a raw structure.
+    $record = (array) $entity;

I don't understand this comment tbh. Why won't it have the same properties as the entity? If it's got a different structure to the Entity then what about an EntityRevisionInterface or similar.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -540,6 +594,16 @@ protected function preDelete($entities) { }
   /**
+   * Act on a revision before being saved.
+   *
+   * @param array $record
+   *   The revision array.
+   * @param Drupal\entity\EntityInterface $entity
+   *   The entity object.
+   */
+  protected function preSaveRevision(array &$record, EntityInterface $entity) { }

So we have the revision being passed as an array, and the entity being passed as an entity. Why would those be different at all? Also I have no idea what the structure of $record might be reading the documentation here.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -321,6 +321,11 @@ public function submit(array $form, array &$form_state) {
diff --git a/core/modules/node/lib/Drupal/node/NodeStorageController.php b/core/modules/node/lib/Drupal/node/NodeStorageController.php

diff --git a/core/modules/node/lib/Drupal/node/NodeStorageController.php b/core/modules/node/lib/Drupal/node/NodeStorageController.php
index 02e694e..482d534 100644

index 02e694e..482d534 100644
--- a/core/modules/node/lib/Drupal/node/NodeStorageController.php

--- a/core/modules/node/lib/Drupal/node/NodeStorageController.php
+++ b/core/modules/node/lib/Drupal/node/NodeStorageController.phpundefined

The diffstat on this file is amazing.

Did I miss the point, why the abstracted entity_revision_delete() - which was there in patch #56 - disappeared again, so node_revision_delete() and other entity types still need to do a "manual" db_delete()?
Otherwise a nice enabler!

Hmm, seems we need to take a few steps back.
Here's a straight re-roll of #56 against HEAD, with just a few obvious typos and glitches fixed.
Now we'd need to catch up with the changes since #56. Wasn't completely sure about what was on purpose and what was erroneous. Probably Berdir knows best.
The supplied interdiff is against #73.

Hmm, #82 applied cleanly here. Another try with some more minor doc fixes (+ stats)...
If the testbot can't apply, I don't know.
The supplied interdiff is still against #73.

+++ b/core/includes/entity.incundefined
@@ -150,6 +150,18 @@ function entity_revision_load($entity_type, $revision_id) {
+ * Deletes a node revision.

Deletes an 'entity revision'

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -243,6 +243,23 @@ class DatabaseStorageController implements EntityStorageControllerInterface {
+      $this->invokeHook('revision_delete', $revision);

Let's change this to 'delete_revision'.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -548,7 +632,13 @@ class DatabaseStorageController implements EntityStorageControllerInterface {
+    $function = 'field_attach_' . $hook;
+    // @todo: field_attach_delete_revision() is named the wrong way round,
+    // consider renaming it.
+    if ($function == 'field_attach_revision_delete') {
+      $function = 'field_attach_delete_revision';
+    }
+    if (!empty($this->entityInfo['fieldable']) && function_exists($function)) {

See comment above.

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -43,6 +43,13 @@ class Entity implements IteratorAggregate, EntityInterface {
   /**
+   * Boolean indicating whether a new revision should be created on save.
+   *
+   * @var bool

"Indicates whether this is a new revision."

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.phpundefined
@@ -64,6 +64,26 @@
+   * Returns whether a new revision should be created on save.
+   *
+   * @return bool
+   *   TRUE if a new revision should be created.
+   *
+   * @see Drupal\Core\Entity\EntityInterface::setNewRevision()
+   */
+  public function isNewRevision();

That comment refers to what we are using it for, not what it actually does. What it does is, it returns whether an instance is a new revision of an entity.

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.phpundefined
@@ -64,6 +64,26 @@
+  /**
+   * Enforces an entity to be saved as a new revision.
+   *
+   * @param bool $value
+   *   (optional) Whether a new revision should be saved.
+   *
+   * @see Drupal\Core\Entity\EntityInterface::isNewRevision()
+   */
+  public function setNewRevision($value = TRUE);

Same here basically.

Still Needs Work?

Ah whatever. I guess I was just nit picking. I can live with it the way it is :)

Marked #120967: Separate revisions from node.module as a duplicate.

Just to clear up any misconceptions about #76, this sat for a week at RTBC because we were over thresholds during that time, and this was classified as a feature request. And we're currently over thresholds again. I agree with this issue's re-classification as a task, however, because as xjm mentions in #74, this is more refactoring than a new feature. And what a lovely refactoring indeed! :D

Committed and pushed to 8.x, along with a CHANGELOG.txt update. WOOHOO. This'll need a change notice.

de-tagging

This bitten me during EFQ v2 although figured out fairly quick:

      $entity = entity_create('test_entity', array(
        'ftid' => $i,
        'ftvid' => $i,
        'fttype' => $bundles[$i & 1],
      ));
      $entity->enforceIsNew();
// This is new. Your revisioned entity won't load without this! Because ftvid is set it tries to update a revision which doesn't exist. Without enforceIsNew it tries to update a whole non-existing entity so not a new pattern though.
      $entity->setNewRevision(); 
      $entity->save();

We're once again over the critical task threshold. Please try and find time to write up documentation for your API change ASAP so your fellow contributors' features aren't blocked. Thanks.

Yes, please write the Change Notice

But it is over the top to use Critical priority for this. IMO, the Needs change notification tag is sufficient. We will remember to close those out before release.

Looks good to me; thanks Berdir!

This is a small fix to entity_revision_delete()'s comment. (I thought better here than a new issue.)

#99: entity-revision-save-delete-1723892-99.patch queued for re-testing.

Committed and pushed to 8.x, but let's get follow-up issues for minor things like this, ok? :)