This is a follow-up to #2144631: Add a revisionable key to field definitions, which got committed without having all field definitions updated. Let's mark this as novice - maybe someone wants to pick up here?

What's left:
- Go through all content entities and update the base field definitions to set all fields that are stored revisionable as revisionable
- To know whether a base field is revisionable, one can lookup the entity type's database table schema. Everything in the data or revision table should be marked revisionsable.

CommentFileSizeAuthor
#25 core-revisionable_fields-2225699-22.patch3.79 KBjessebeach
#25 interdiff.txt1.45 KBjessebeach
#21 block-language.png26.96 KBjessebeach
#19 interdiff-10-19.txt442 bytesczigor
#19 core-revisionable_fields-2225699-19.patch2.89 KBczigor
#10 interdiff-5-10.txt2.12 KBczigor
#10 core-revisionable_fields-2225699-10.patch3.62 KBczigor
#5 core-revisionable_fields-2225699-5.patch3.03 KBczigor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

Core revisionable entity types are:

  • custom_block
  • node
  • entity_test_rev
  • entity_test_mulrev

The actual tables to check are named ENTITY_TYPE_revision.

czigor’s picture

czigor CreditAttribution: czigor commented

Working on this.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Parent issue: » #2144263: Decouple entity field storage from configurable fields
czigor’s picture

czigor CreditAttribution: czigor commented
Assigned: Unassigned » czigor
czigor’s picture

czigor CreditAttribution: czigor commented
Status: Active » Needs review
FileSize
core-revisionable_fields-2225699-5.patch3.03 KB

Added the ->setRevisionable(TRUE); call for the above entities' revisionable fields. I have not found any other revisionable entities.

setRevisionable(TRUE) is not called for id and revision id fields.

czigor’s picture

czigor CreditAttribution: czigor commented
Assigned: czigor » Unassigned

Status: Needs review » Needs work

The last submitted patch, 5: core-revisionable_fields-2225699-5.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review

5: core-revisionable_fields-2225699-5.patch queued for re-testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Needs work

Thanks for the patch! There are just a couple of issues to fix:

  1. Also node.langcode should be revisionable. 
  2. +++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTestMulRev.php
@@ -59,6 +59,8 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    $fields['langcode'] = $fields['langcode']->setRevisionable(TRUE);

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTestRev.php
@@ -60,6 +60,10 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    $fields['langcode'] = $fields['langcode']->setRevisionable(TRUE);
+    $fields['name'] = $fields['name']->setRevisionable(TRUE);
+    $fields['user_id'] = $fields['user_id']->setRevisionable(TRUE);

    There is no need to reassign field definitions, enabling the revisionable property is enough.

czigor’s picture

czigor CreditAttribution: czigor commented
FileSize
core-revisionable_fields-2225699-10.patch3.62 KB
interdiff-5-10.txt2.12 KB

Thanks for the review! Fixes are in the attachment.

czigor’s picture

czigor CreditAttribution: czigor commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: core-revisionable_fields-2225699-10.patch, failed testing.

czigor’s picture

czigor CreditAttribution: czigor commented

10: core-revisionable_fields-2225699-10.patch queued for re-testing.

The last submitted patch, 10: core-revisionable_fields-2225699-10.patch, failed testing.

czigor’s picture

czigor CreditAttribution: czigor commented

10: core-revisionable_fields-2225699-10.patch queued for re-testing.

czigor’s picture

czigor CreditAttribution: czigor commented
Status: Needs work » Needs review
jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented
Related issues: +#2224549: Simplify checking whether an entity type is revisionable
fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Needs review » Needs work

Patch looks good to me.

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTestMulRev.php
@@ -59,6 +59,8 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    $fields['langcode']->setRevisionable(TRUE);

That should be already revisionable from the parent. Else patch looks good to me.

czigor’s picture

czigor CreditAttribution: czigor commented
Status: Needs work » Needs review
FileSize
core-revisionable_fields-2225699-19.patch2.89 KB
interdiff-10-19.txt442 bytes
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Reviewed & tested by the community

Thanks!

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented
Status: Reviewed & tested by the community » Needs work
FileSize
block-language.png26.96 KB

Node sets langcode to be revisionable.

$fields['langcode'] = FieldDefinition::create('language')
  ->setLabel(t('Language code'))
  ->setDescription(t('The node language code.'))
  ->setRevisionable(TRUE);

but CustomBlock does not:

$fields['langcode'] = FieldDefinition::create('language')
  ->setLabel(t('Language code'))
  ->setDescription(t('The custom block language code.'));

Seems like CustomBlock should also set langcode to revisionable? They can be varied by language and revisioned by language.

Screenshot of a block type edit form. The language switcher select box is indicated.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Reviewed & tested by the community

Let's stick to the currently implemented schema for now so we don't introduce inconsistencies: custom block language is not revisionable atm, we can fix it when we have entity schema generation in place :)

We are merely describing the current schema here.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

yep, #21 is a good point - but as this issue is just about adding metadata for what's there it should be fixed in its own issue.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented
Status: Reviewed & tested by the community » Needs review
FileSize
interdiff.txt1.45 KB
core-revisionable_fields-2225699-22.patch3.79 KB

Ok, I can live with that since #2144263: Decouple entity field storage from configurable fields is also a beta blocker, so it will get done.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented
Status: Needs review » Reviewed & tested by the community

xpost

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented
Priority: Normal » Critical

If this is a beta blocker, it is then by definition also Critical.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

Sorry, I was going to post a patch with the changed schema, but decided against it after plach's comment in #22. But my form still wanted to upload the files. So I'm hiding them now. The patch in #19 is the one to commit.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: core-revisionable_fields-2225699-22.patch, failed testing.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented
Status: Needs work » Reviewed & tested by the community
Related issues: +#2226445: The custom_block entity should include langcode in the set of properties included in a revision entry

A related issue for custom_block revisioning by langcode.

#2226445: The custom_block entity should include langcode in the set of properties included in a revision entry

  • Commit 64cd121 on 8.x by catch: 
    Issue #2225699 by czigor, jessebeach: Set all revisionable fields to...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Thanks!

Status: Fixed » Closed (fixed)

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