Comments

timmillwood created an issue. See original summary.

larowlan’s picture

Status: Needs work » Active
timmillwood’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
4.77 KB

Initial patch for this.

Status: Needs review » Needs work

The last submitted patch, 3: blockcontent_should-2716081-3.patch, failed testing.

timmillwood’s picture

FileSize
30.59 KB

timmillwood’s picture

Title: BlockContent should have revision_user and revision_created fields » BlockContent should have revision_user and revision_created fields and implement RevisionLogInterface
timmillwood’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
9.1 KB

Added basic tests, and some of the fixes needed.

timmillwood’s picture

Adding update hook.

The last submitted patch, 7: blockcontent_should-2716081-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: blockcontent_should-2716081-8.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
11.17 KB

Fixed the test

dixon_’s picture

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?

dixon_’s picture

Issue tags: +Needs change record
Parent issue: » #2721129: Workflow Initiative
dixon_’s picture

Issue tags: +Workflow Initiative
timmillwood’s picture

@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.

Change record added https://www.drupal.org/node/2723393

timmillwood’s picture

Issue tags: -Needs change record
markdorison’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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_’s picture

@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 :)

dixon_’s picture

dawehner’s picture

Status: Needs review » Needs work

Given the latest feedback.

dawehner’s picture

Issue tags: +Needs tests

.

timmillwood’s picture

Priority: Normal » Major
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
13.23 KB
2.62 KB

Adding test to check revision_user and revision_created BaseFieldDefinitions are added in the update hook.

markdorison’s picture

Status: Needs review » Reviewed & tested by the community

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

jibran’s picture

+++ 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.

timmillwood’s picture

Taking @jibran's advice.

jibran’s picture

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

timmillwood’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed 4ac8f96 on 8.2.x
    Issue #2716081 by timmillwood, dixon_: BlockContent should have...
paranojik’s picture

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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.