EntityNG: Support for revisions for entity save and delete operations

Is there a reason Entity::getRevisionId() always returns NULL instead checking the entity info for the the appropriate entity key?
Similar questions apply to Entity::id() and Entity::uuid().

t
That is because in D8 (in contrast to D7 Entity API) we do not always have the entity info available. We deliberately do not load the entity info unconditionally so it does not show up in every debugged entity. Also, if an entity overrides that method with

public function id() {
  return $this-uid;
}

(e.g. in the case of User) that is more performant than checking the entity info first.

Looks awesome except a few hunks

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -142,7 +142,7 @@ protected function attachLoad(&$queried_entities, $load_revision = FALSE) {
-  protected function mapFromStorageRecords(array $records) {
+  protected function mapFromStorageRecords(array $records, $load_revision = FALSE) {

Suppose doc-block should be changed too

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -211,13 +225,60 @@ public function save(EntityInterface $entity) {
+   * @return integer
+   *   The revision id.
+   */
+  protected function saveRevision(EntityInterface $entity) {

Not sure that we always have id() as integer

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -211,13 +225,60 @@ public function save(EntityInterface $entity) {
+    // @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';
+    }

We need a separate issue for that

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityRevisionsTest.phpundefined
@@ -0,0 +1,101 @@
+    // Basis settings.
+    $settings = array(
+      'name' => 'foo',
+      'user_id' => $this->web_user->uid,
+    );
+    $entity = entity_create('entity_test', $settings);

$settings is not used latter. better $entity = entity_create('entity_test', array('name' => 'foo','user_id' => $this->web_user->uid))

Sorry for not reviewing this earlier :) I think this is quite close, some clean-up stuff and then I'm happy to RTBC this.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -181,13 +186,27 @@ public function save(EntityInterface $entity) {
+        else {
+          // @todo, should a different value be returned when saving an entity
+          // with $isDefaultRevision = FALSE?
+          $return = FALSE;

Note to reviewers.

This @todo is just taken over from the existing DatabaseStorageController revision implementation which this patch is based on.

Can we maybe have a follow-up issue for this linked in the issue summary? Which would introduce SAVED_REVISION or something like that.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -211,13 +230,60 @@ public function save(EntityInterface $entity) {
+   * @param Drupal\Core\Entity\EntityInterface $entity
+   *   The entity object.

Should be @param \Drupal\Core\...

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -211,13 +230,60 @@ public function save(EntityInterface $entity) {
+    // When saving a new revision, set any existing revision ID to NULL so as to
+    // 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 ($entity->isNewRevision() && isset($record->{$this->revisionKey})) {
+      $record->{$this->revisionKey} = NULL;

The comment is unfortunately outdated (the second part). I'm quite that I had fixed this at some point but messed up a re-roll :(

Can we remove that part here and in the default implementation?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -211,13 +230,60 @@ public function save(EntityInterface $entity) {
+    $record = (array) $record;
+    $this->preSaveRevision($record, $entity);
+    $record = (object) $record;

You're doing this right now to make as few changes as possible.

This will become the implementation that will actually stay, so I'd suggest to update preSaveRevision() and the existing implementation to use an object here. Otherwise, we need a follow-up to fix it once we removed the old controller.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -211,13 +230,60 @@ public function save(EntityInterface $entity) {
+    // @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';

Same here, this is an existing @todo. And same request, can we have a follow-up for this?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -239,4 +305,16 @@ protected function mapToStorageRecord(EntityInterface $entity) {
+  protected function mapToRevisionStorageRecord(EntityInterface $entity) {
+    $record = new \stdClass();
+    foreach ($this->entityInfo['schema_fields_sql']['revision_table'] as $name) {
+      $record->$name = $entity->$name->value;
+    }

Wondering if we should introduce an internal helper function for this.

I'm also still not convinced that the special case support for date fields introduced in the comments issue is what we should be doing.

The problem is that the comment patch introduces special support for date fields here. And nodes will require the same, so once we convert nodes to use field type date for created/changed, we'll also need to support date here, which results in duplicated code.

Doing it right now would however unecessarly conflict with the comments patch, so.. follow-up? :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityRevisionsTest.phpundefined
@@ -0,0 +1,100 @@
+use Drupal\simpletest\WebTestBase;
+
+class EntityRevisionsTest extends WebTestBase {
+  protected $entities;
+  protected $names;

Missing documentation for the properties and the class.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityRevisionsTest.phpundefined
@@ -0,0 +1,100 @@
+    // Create and login user.
+    $web_user = $this->drupalCreateUser(array(
+      'view revisions',
+      'revert revisions',
+      'delete revisions',
+      'administer entity_test content',
+    ));
+    $this->drupalLogin($web_user);
+    $this->web_user = $web_user;

We can save an unecessary line of code if you directly do a $this->web_user = $this->drupalCreateUser().

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityRevisionsTest.phpundefined
@@ -0,0 +1,100 @@
+    // Create initial entity.
+    // Basis settings.
+    $entity = entity_create('entity_test', array(
+      'name' => 'foo',
+      'user_id' => $this->web_user->uid,

The second comment here doesn't really feel like proper english and we can probably just leave it away.

Some small nitpicks, looks RTBC to me otherwise if the bot agrees.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -552,13 +552,14 @@ protected function saveRevision(EntityInterface $entity) {
+    $record = (object) $record;
     $this->preSaveRevision($record, $entity);
+    $record = (array) $record;

Maybe add a comment here to explain why we are doing this? Maybe something like "preSaveRevision() excepts an object now to be compatible with the NG storage controller".

Completely changing to an object here would probably require quite a few changes more :(

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -613,12 +614,12 @@ protected function postDelete($entities) { }
-  protected function preSaveRevision(array &$record, EntityInterface $entity) { }
+  protected function preSaveRevision(\stdClass &$record, EntityInterface $entity) { }

The & is no longer necessary now.

Same for the implementations.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -557,6 +557,8 @@ protected function saveRevision(EntityInterface $entity) {
+    // Caste to object as preSaveRevision() expects one now to be compatible
+    // with the upcoming NG storage controller.
     $record = (object) $record;

Cast to .. no e :) Also, I think the "now" is unecessary.

Yes, looks good to me. This makes sure that the NG storage controller provides the same features as the old one. This adds test coverage and revision support for entity_test, which means that we can remove test_entity in favor of this as soon as this is in. Yay!

RTBC.

Hu, what happened with that tag?

All of this looks great to me, apart from:

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -552,13 +552,16 @@ protected function saveRevision(EntityInterface $entity) {
+    // Cast to object as preSaveRevision() expects one to be compatible with the
+    // upcoming NG storage controller.
+    $record = (object) $record;
     $this->preSaveRevision($record, $entity);
+    $record = (array) $record;

@@ -613,12 +616,12 @@ protected function postDelete($entities) { }
-   * @param array $record
-   *   The revision array.
+   * @param \stdClass $record
+   *   The revision object.
...
-  protected function preSaveRevision(array &$record, EntityInterface $entity) { }
+  protected function preSaveRevision(\stdClass $record, EntityInterface $entity) { }

Holy heavens, that is ugly. :(

Why can't we just keep this an array throughout?

We discussed this in IRC and I think I convinced webchick that this is ok like that for now, so back to RTBC.

In short, http://api.drupal.org/api/drupal/core!lib!Drupal!Core!Entity!DatabaseSto... already uses stdClass, we decided to implement this in a way that is consistent with the existing save() implementation and make only minimal adjustments to the existing saveRevision implementation because it will be removed anyway.

Yep, thanks Berdir!

We're over thresholds atm, but since we were under this weekend when this was originally RTBCed, I am cashing a raincheck. ;)

Committed and pushed to 8.x. :) YAY!