BlockContent should have revision_user and revision_created fields and implement RevisionLogInterface

Patch looks really good.

I think it would be worth adding some kind of test just ensuring that block_content_update_8003() works as expected for sites with existing content. However, I'm not sure how we easily can test update hooks in core?

Tested the patch in #11 and it is working as expected.

+++ b/core/modules/block_content/block_content.install
@@ -39,3 +39,25 @@ function block_content_update_8002() {
+/**
+ * Add 'revision_created' and 'revision_user' fields to 'block_content' entities.
+ */
+function block_content_update_8003() {
+  $revision_created = BaseFieldDefinition::create('created')
+    ->setLabel(t('Revision create time'))
+    ->setDescription(t('The time that the current revision was created.'))
+    ->setRevisionable(TRUE);
+
+  \Drupal::entityDefinitionUpdateManager()
+    ->installFieldStorageDefinition('revision_created', 'block_content', 'block_content', $revision_created);
+
+  $revision_user = BaseFieldDefinition::create('entity_reference')
+    ->setLabel(t('Revision user'))
+    ->setDescription(t('The user ID of the author of the current revision.'))
+    ->setSetting('target_type', 'user')
+    ->setRevisionable(TRUE);
+
+  \Drupal::entityDefinitionUpdateManager()
+    ->installFieldStorageDefinition('revision_user', 'block_content', 'block_content', $revision_user);
+}
\ No newline at end of file

Is this explicitly tested anywhere?

@dixon_ - There are already over 40 tests that check for schema changes. So this kinda covers the block_content_update_8003() update hook. If the update hook breaks / fails then all these schema tests will fail.

Schema hooks might be tested, but not the actual data update we perform this this specific update hook. And as we are touching people's data structure we need at least some kind of basic test to ensure it's doing what we think it does :)

Given the latest feedback.

.

Tested #23. Working as expected for both a fresh install and update.

+++ b/core/modules/block_content/src/Tests/BlockContentUpdateEntityFields.php
@@ -0,0 +1,44 @@
+    $pre_revision_created = \Drupal::entityDefinitionUpdateManager()->getFieldStorageDefinition('revision_created', 'block_content');
+    $pre_revision_user = \Drupal::entityDefinitionUpdateManager()->getFieldStorageDefinition('revision_user', 'block_content');
+    $this->assertEqual(null, $pre_revision_created, "Revision created field not found");
+    $this->assertEqual(null, $pre_revision_user, "Revision user field not found");

These asserts are not needed. Adding asserts to update tests before running updates turned out to be problematic in other issues.

We need a follow up to update BlockContentViewsData with revision_user relationship to users table.

Committed 4ac8f96 and pushed to 8.2.x. Thanks!

diff --git a/core/modules/block_content/src/Entity/BlockContent.php b/core/modules/block_content/src/Entity/BlockContent.php
index 2640d74..9e0531e 100644
--- a/core/modules/block_content/src/Entity/BlockContent.php
+++ b/core/modules/block_content/src/Entity/BlockContent.php
@@ -6,7 +6,6 @@
 use Drupal\Core\Entity\EntityChangedTrait;
 use Drupal\Core\Entity\EntityStorageInterface;
 use Drupal\Core\Entity\EntityTypeInterface;
-use Drupal\Core\Entity\RevisionLogInterface;
 use Drupal\Core\Field\BaseFieldDefinition;
 use Drupal\block_content\BlockContentInterface;
 use Drupal\user\UserInterface;
@@ -284,4 +283,5 @@ public function setRevisionLogMessage($revision_log_message) {
     $this->set('revision_log', $revision_log_message);
     return $this;
   }
+
 }

Couple of nits fixed on commit.

I notices something that looks like an inconsistency introduced by this patch. On the schema level the two new fields 'revision_user' and 'revision_created' are added to the 'block_content_field_data' and 'block_content_field_revision' tables, whereas the existing 'revision_log' field remained in the 'block_content_revision' table.
I don't understand the schema details but still it seems really strange and also not consistent with the way revision metadata is handled for nodes. In the node's case all three revision metadata fields 'revision_log', 'revision_uid' and 'revision_timestamp' reside in the 'node_revision' table.
There's already an issue #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table where this is being discussed.