The current UUID implementation for nodes doesn't support revision UUIDs. This means that when an entity supports revisions, those revisions can't be tracked across environments. There are situations where it is reasonable to expect to be able to update a revision of a node in one environment with the contents from another.

The UUID contrib module supports UUIDs for both the entity (and where supported) revisions. It would be good to see core support this in D8.

Adding the Revision UUID column as a not null required column poses a problem with setting the default values. Previously we've got around this problem by using initial or initial_from_field but that is a little trickier with UUID which need to be unique per row. We have two options:

  1. Use initial_from_field as is shown in #48 and then provide a UUID() function for each database layer.
  2. Solve #2346019: Handle initial values when creating a new field storage definition so that we can support many different schema migrations.
CommentFileSizeAuthor
#148 1812202-148.patch29.05 KBthursday_bw
#140 1812202-140.patch32.94 KBtimmillwood
#138 1812202-138.patch33.35 KBtimmillwood
#136 1812202-136.patch34.94 KBlammensj
#134 1812202-134.patch34.89 KBdagmar
#132 1812202-132.patch35.22 KBtimmillwood
#125 1812202-125.patch35.61 KBtimmillwood
#125 interdiff.txt1.91 KBtimmillwood
#124 interdiff.txt4.41 KBtimmillwood
#124 1812202-124.patch35.72 KBtimmillwood
#122 interdiff.txt4.59 KBtimmillwood
#122 1812202-122.patch31.84 KBtimmillwood
#120 interdiff.txt7.27 KBtimmillwood
#120 1812202-120.patch31.95 KBtimmillwood
#113 interdiff.txt2.77 KBtimmillwood
#113 1812202-113.patch34.06 KBtimmillwood
#111 1812202-111.patch33.87 KBtimmillwood
#111 interdiff.txt5.12 KBtimmillwood
#109 interdiff.txt6.16 KBtimmillwood
#109 1812202-109.patch30.16 KBtimmillwood
#107 interdiff.txt6.39 KBtimmillwood
#107 1812202-107.patch25.24 KBtimmillwood
#102 interdiff.txt671 bytestimmillwood
#102 1812202-102.patch25.5 KBtimmillwood
#100 interdiff.txt3.93 KBtimmillwood
#100 1812202-100.patch24.85 KBtimmillwood
#98 interdiff.txt7.02 KBtimmillwood
#98 1812202-98.patch24.05 KBtimmillwood
#93 interdiff.txt1.51 KBtimmillwood
#93 1812202-93.patch25.84 KBtimmillwood
#91 interdiff.txt783 bytestimmillwood
#91 1812202-91.patch25.2 KBtimmillwood
#88 interdiff.txt1.94 KBtimmillwood
#88 1812202-88.patch25.08 KBtimmillwood
#86 interdiff.txt1.57 KBbenjy
#86 1812202-86.patch23.83 KBbenjy
#84 interdiff.txt1.71 KBtimmillwood
#84 1812202-84.patch25.4 KBtimmillwood
#81 1812202-81.patch25.04 KBtimmillwood
#78 interdiff.txt932 bytestimmillwood
#78 1812202-78.patch40.67 KBtimmillwood
#76 interdiff.txt1.04 KBtimmillwood
#76 1812202-76.patch39.52 KBtimmillwood
#74 interdiff.txt2.64 KBtimmillwood
#74 1812202-74.patch39.11 KBtimmillwood
#73 interdiff.txt1.52 KBtimmillwood
#73 1812202-73.patch39.64 KBtimmillwood
#65 add_uuid_support_for-1812202-65-interdiff.txt1.79 KBBerdir
#65 add_uuid_support_for-1812202-65.patch38.38 KBBerdir
#60 1812202-60.patch38.48 KBbenjy
#60 interdiff.txt704 bytesbenjy
#58 interdiff.txt13.24 KBbenjy
#57 1812202-57.patch38.5 KBbenjy
#56 1812202-56.patch49.62 KBbenjy
#48 interdiff.txt1.49 KBbenjy
#48 1812202-48.patch49.41 KBbenjy
#45 interdiff.txt17.79 KBbenjy
#45 1812202-45.patch47.91 KBbenjy
#42 interdiff.txt3.08 KBbenjy
#42 1812202-42.patch35.48 KBbenjy
#40 interdiff.txt4.9 KBbenjy
#40 1812202-40.patch34.43 KBbenjy
#38 interdiff.txt8.29 KBbenjy
#37 1812202-37.patch32.79 KBbenjy
#35 interdiff.txt14.95 KBbenjy
#35 1812202-35.patch28.27 KBbenjy
#33 1812202-33.patch13.94 KBbenjy
#26 1812202-26.patch10.43 KBdixon_
#20 interdiff.txt6.96 KBamateescu
#20 1812202-20.patch13.88 KBamateescu
#16 interdiff.txt2.36 KBamateescu
#16 1812202-16.patch16.37 KBamateescu
#14 interdiff.txt10.05 KBamateescu
#14 1812202-14.patch14 KBamateescu
#11 1812202-11.patch8.83 KBamateescu
#7 drupal-1812202-revision-uuids.patch7.34 KBtimmillwood
#2 drupal-1812202-revision-uuids.patch7.34 KBskwashd
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mitchell’s picture

Issue tags: +revision, +UUID

(tagging)

skwashd’s picture

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

This is a first cut. I want some feedback to make sure I am heading in the right direction with this. If people are happy with it I'll add some tests.

The last submitted patch, drupal-1812202-revision-uuids.patch, failed testing.

skwashd’s picture

Status: Needs work » Needs review

#2: drupal-1812202-revision-uuids.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1812202-revision-uuids.patch, failed testing.

effulgentsia’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue summary: View changes

#2304849: Stop excluding local ID and revision fields from HAL output was committed recently, and in the course of that issue, we lamented not having revision UUIDs.

Bumping this to 8.1 for now, but per https://groups.drupal.org/node/508968, this will likely need to be bumped to 8.2 after March 2.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
7.34 KB

Requeuing patch from #2 with DrupalCI, just for fun!

Status: Needs review » Needs work

The last submitted patch, 7: drupal-1812202-revision-uuids.patch, failed testing.

dawehner’s picture

Thank you for the reroll, but of course in the meantime some thing changed.

Another thing which would be nice to use autogenerate the base fields, see ContentEntityBase::baseFieldDefinitions()

  1. +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
    @@ -136,6 +136,14 @@ public function __construct($entityType) {
    +    if (!empty($this->entityInfo['entity_keys']['vuuid'])) {
    

    What about just using 'revision_uuid', to not shortcut but rather be really explicit.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -40,6 +40,17 @@ public function id();
    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 d855384..a7cd045 100644
    
    index d855384..a7cd045 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.php
    
    +++ b/core/modules/node/lib/Drupal/node/NodeStorageController.php
    +++ b/core/modules/node/lib/Drupal/node/NodeStorageController.php
    @@ -9,6 +9,7 @@
    
    @@ -9,6 +9,7 @@
     
     use Drupal\Core\Entity\DatabaseStorageController;
     use Drupal\Core\Entity\EntityInterface;
    +use Drupal\Component\Uuid\Uuid;
     
     /**
      * Controller class for nodes.
    

    We should be able to skip this change completly

  3. +++ b/core/modules/node/node.install
    @@ -717,6 +726,40 @@ function node_update_8012() {
    +  $spec = array(
    +    'description' => 'The universally unique identifier (UUID) for this version.',
    +    'type' => 'varchar',
    +    'length' => 128,
    +    'not null' => FALSE,
    +  );
    +  $keys = array(
    +    'unique keys' => array(
    +      'vuuid' => array('vuuid'),
    +    ),
    +  );
    +  // Account for sites having the contributed UUID module installed.
    +  if (db_field_exists('node_revision', 'vuuid')) {
    +    db_change_field('node_revision', 'vuuid', 'vuuid', $spec, $keys);
    +  }
    +  else {
    +    db_add_field('node_revision', 'vuuid', $spec, $keys);
    +  }
    +
    +  if (!isset($sandbox['progress'])) {
    +    $sandbox['progress'] = 0;
    +    $sandbox['last'] = 0;
    +    $sandbox['max'] = db_query('SELECT COUNT(vid) FROM {node_revision} WHERE vuuid IS NULL')->fetchField();
    +  }
    +
    +  $vids = db_query_range('SELECT vid FROM {node_revision} WHERE vid > :vid AND vuuid IS NULL ORDER BY vid ASC', 0, 10, array(':vid' => $sandbox['last']))->fetchCol();
    +  update_add_uuids($sandbox, 'node_revision', 'vid', $vids);
    +
    +  $sandbox['#finished'] = empty($sandbox['max']) ? 1 : ($sandbox['progress'] / $sandbox['max']);
    +}
    

    I'm wondering whether we could generalize this entire update function to make it available for other ones as well

  4. +++ b/core/modules/node/node.install
    @@ -717,6 +726,40 @@ function node_update_8012() {
    +  update_add_uuids($sandbox, 'node_revision', 'vid', $vids);
    

    This function is missing

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amateescu’s picture

Re-starting the patch from scratch, a lot of things moved around in the meantime :) Leaving at NW because the update function is not there yet.

Anishnirmal’s picture

Status: Needs work » Needs review

Moving it to needs review to take the automated test for patch at #11.

Status: Needs review » Needs work

The last submitted patch, 11: 1812202-11.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
14 KB
10.05 KB

Some progress.

+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
@@ -93,6 +93,10 @@ public function getOriginalClass();
+   *     @todo Should the revision_uuid key be populated by default if the
+   *       revision key is set?

This question is still outstanding.. the current patch makes it kinda mandatory to have a revision_uuid key set if revision_id is set.

Status: Needs review » Needs work

The last submitted patch, 14: 1812202-14.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
16.37 KB
2.36 KB

This is the problem outlined in #14.

Does anyone have an opinion on this matter? :)

timmillwood’s picture

I think revision_uuid should be populated be default if revision key is set.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -1023,6 +1039,18 @@ protected function saveRevision(ContentEntityInterface $entity) {
+        if (isset($entity->original) && $entity->original->{$this->revisionUuidKey} != $entity->original->{$this->revisionUuidKey}) {

When will this ever return true?

amateescu’s picture

When will this ever return true?

When someone sets $entity->original and/or $entity->original->revision_uuid manually. See the code comment just above that line :)

Status: Needs review » Needs work

The last submitted patch, 16: 1812202-16.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
13.88 KB
6.96 KB

Discussed with @timmillwood on IRC and I finally got the problem mentioned in #17. However, I cannot easily think of a way that an external source might pre-set the revision uuid, so I took out that part for now.

Status: Needs review » Needs work

The last submitted patch, 20: 1812202-20.patch, failed testing.

timmillwood’s picture

Issue tags: +Workflow Initiative
pwolanin’s picture

Listening to the core conversation, I feel like the parent UUID + revision hash is possibly more useful than a revision UIID, though I understand that will not uniquely identify a revision since you could have multiple revisions with exactly the same content

markdorison’s picture

Is there a compelling reason to not have both a UUID and a revision hash? It seems that they serve two distinct (and valuable) purposes.

dixon_’s picture

As part of #2721129: Workflow Initiative I would like to propose a change in direction of this issue. Since 2012 (when skwashd posted this issue) the Multiversion has successfully evolved in contrib. And in Multiversion we are borrowing heavily from how revisions are done in other systems, namely Git and CouchDB.

A very important design decision in both Git and CouchDB is that if exactly the same change (time, content etc) is made in two different environments to the same entity it should result in the same revision hash. The point is that this hash should NOT be unique in the case just described. It's better if we can identify exactly the same change in two environments as the same revision hash to avoid having the same change marked as conflicts (once these two changes make it onto the same environment).

Having a revision UUID alongside this proposed revision hash would cause the hash to always be unique (which is not good), unless we exclude it from the hash algorithm. So I would propose we only move forward with implementing this revision hash since that is what #2721129: Workflow Initiative needs.


Structurally I think this patch can stay the same pretty much, we just need to change revision_uuid into revision_hash and use a different field type than uuid.

Stub code for the bit that generates the hash:

  $array = $entity->toArray();
  // Don't include local IDs to keep hash consistent across multiple environments.
  foreach (['id', 'revision_id'] as $key) {
    unset($array[$key]);
  }
  $entity->revision_hash->value = md5(serialize($array));
dixon_’s picture

FileSize
10.43 KB

Work in progress patch...

markdorison’s picture

Having a revision UUID alongside this proposed revision hash would cause the hash to always be unique (which is not good), unless we exclude it from the hash algorithm. So I would propose we only move forward with implementing this revision hash since that is what #2721129: Workflow Initiative needs.

@dixon_: You show in your stub code removing id and revision_id before hashing. If the goal is for identical changes in two different environments to have the same hash then we would also need to exclude changed (possibly others?). I only bring this up because I think getting the right value for the revision hash and whether or not we want to add a revision UUID are two different topics. Your code demonstrates that no matter what, we are going to need to exclude some data to get the revision hash to function as desired. With that in mind:

  1. Should this issue be renamed to focus on the revision hash?
  2. Would adding a revision UUID be valuable as well? If so, should that be a separate issue?
sime’s picture

You show in your stub code removing id and revision_id before hashing. If the goal is for identical changes in two different environments to have the same hash then we would also need to exclude changed (possibly others?).

There is no way to define for every use case what constitutes a 'unique change" between two environments. Whatever fields are excluded from the hash, it will require making assumptions, and assumptions need to be overridable.

acbramley’s picture

I agree with @sime here, I also think that hijacking this issue and focusing on the revision hash should be avoided. Or if that is the roadmap that is taken, this issue should be closed and a new one opened since a hash and a uuid are achieving slightly different things.

amateescu’s picture

a hash and a uuid are achieving slightly different things

I also agree with that and I believe that a revision UUID is still useful for REST purposes, as discussed in #2304849-25: Stop excluding local ID and revision fields from HAL output and the comments below.

So I propose to keep the original purpose of this issue and open a new one for adding the revision hash.

dixon_’s picture

Issue tags: -Workflow Initiative

So I propose to keep the original purpose of this issue and open a new one for adding the revision hash.

Yep, no problem. There are ways to work around having both revision UUID and revision hash. So I've moved the hash part into #2727511: WI: Add revision hash base field to all revisionable entities and instead made that issue part of #2721129: Workflow Initiative.

larowlan’s picture

However, I cannot easily think of a way that an external source might pre-set the revision uuid, so I took out that part for now.

Default content et-al would use that feature if we had a normalizer for revisions.

benjy’s picture

Status: Needs work » Needs review
FileSize
13.94 KB

re-roll.

Status: Needs review » Needs work

The last submitted patch, 33: 1812202-33.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
28.27 KB
14.95 KB

Couple of test fixes, added the update hooks and fixed a bug in SqlContentEntityStorageSchema

Status: Needs review » Needs work

The last submitted patch, 35: 1812202-35.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
32.79 KB

OK, i've fixed a couple of tests but we have an issue with setting the default value. The core issue for that is #2346019: Handle initial values when creating a new field storage definition which probably needs to leverage a Migrate based solution although this has promise #2721313: Upgrade path between revisionable / non-revisionable entities

I came up with a workaround using initial_from_field which I think works for MySQL and chx tells me we can solve it for the other database engines. That is now a schema change for all UUID fields though so I've left some TODO's to clean-up the reflection I was using and bake it into SqlContentEntityStorageSchema which will allow the entity updates to run.

benjy’s picture

FileSize
8.29 KB

Here's the interdiff

Status: Needs review » Needs work

The last submitted patch, 37: 1812202-37.patch, failed testing.

benjy’s picture

The change in entity_schema_test.module shows an API break, if other modules are enabling revisions via hook_entity_type_alter() then they'll also need to add revision_uuid otherwise getEntitySchema() will fail trying to get the field storage definition for revision_uuid. We could fix this by enforcing the entity key defaults logic in __set for entity_keys?

A couple more test fixes but still not managed to get rid of the reflection.

I've also removed the schema check in requiresFieldStorageSchemaChanges to see what breaks on the bot, I feel like the schema comparison is being handled below with $this->getSchemaFromStorageDefinition($storage_definition) != $this->loadFieldSchemaData($original)

Status: Needs review » Needs work

The last submitted patch, 40: 1812202-40.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
35.48 KB
3.08 KB

Patch is green, removing the reflection. Still need to try get this green on postgres and sqlite.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -288,10 +288,18 @@ public function __construct($definition) {
    +    if (!empty($this->entity_keys['revision'])) {
    +      $this->entity_keys['revision_uuid'] = 'revision_uuid';
    +    }
    

    IMHO we should also check whether 'revision_uuid' was not set yet. Users might override it, for whatever reason

  2. +++ b/core/lib/Drupal/Core/Entity/RevisionableInterface.php
    @@ -40,6 +40,15 @@ public function setNewRevision($value = TRUE);
       /**
    +   * Gets the revision universally-unique identifier of the entity.
    +   *
    +   * @return string
    +   *   The revision UUID of the entity, or NULL if the entity does not
    +   *   have a revision UUID.
    +   */
    +  public function getRevisionUuid();
    

    From a reader of this documentation I would be excited if this method would not just tell the information, which is already given by the function name, but rather also some actual additional information, for example an example where a revision UUID could be useful or so.

    Note: The return value is string, even getEntityKey returns NULL, if it doesn't exist.

    One thing which could be interesting to document is also whether this UUID is unique across translations or not.

  3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -154,11 +165,14 @@ public function getFieldStorageDefinitions() {
    +   * @param \Drupal\Component\Uuid\UuidInterface $uuid_service
    +   *   The UUID service.
    ...
    +    $this->uuidService = $uuid_service;
    

    I kinda dislike this variable name. What about name it $uuidGenerator, as this is exactly what this is doing? Note: This new property is also not documented yet.

  4. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -1033,7 +1054,10 @@ protected function saveRevision(ContentEntityInterface $entity) {
    -          ->fields(array($this->revisionKey => $record->{$this->revisionKey}))
    +          ->fields([
    +            $this->revisionKey => $record->{$this->revisionKey},
    +            $this->revisionUuidKey => $record->{$this->revisionUuidKey},
    

    Maybe some documentation could be added why this is needed. Why should keying by revision ID not be unique enough already?

Status: Needs review » Needs work

The last submitted patch, 42: 1812202-42.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
47.91 KB
17.79 KB
  1. Done
  2. I've done my best on the comments. Yes, translations are all contained within the one revision and therefore share a UUID.
  3. Agreed, renamed as suggested.
  4. This is just updating the values, nothing to do with keying?

Fixed all of dawehner's feedback, I think the only real way to solve the Postgres and SQLite fails is going to be a proper schema migration API from #2346019: Handle initial values when creating a new field storage definition

dawehner’s picture

Thank you @benjy

tstoeckler’s picture

+    // If the entity type is revisionable, provide a default value for the
+    // 'revision_uuid' key.
+    if (!empty($this->entity_keys['revision']) && empty($this->entity_keys['revision_uuid'])) {
+      $this->entity_keys['revision_uuid'] = 'revision_uuid';
+    }

This is a novelty in core, we currently do not provide any default entity keys, not even for uuid itself. Also there is no way to opt-out from this as far as I can tell if you want to avoid this for some reason.

Therefore, I would propose dropping that. Out entity annotations are already very verbose which independently is a problem in its own right, but one more line shouldn't be a problem I think.

benjy’s picture

@tstoeckler, will take a look at that tomorrow.

Here's a fix for SQLite.

benjy’s picture

@tstoeckler, I looked into your suggestion but the outcome was, it's more effort than just that one line to make it optional and in the end become much messier. I think, if you have revisions enabled then you get revision_uuid as well and that's it. There is a small API break for those who don't call ContentEntityBase::baseFieldDefinitions() that will need documenting in the change record.

benjy’s picture

Issue summary: View changes
Issue tags: +Needs framework manager review

Adding tags for review and updating IS.

timmillwood’s picture

Very nitty nit picks.

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
    @@ -100,16 +100,16 @@ class ConfigEntityStorage extends EntityStorageBase implements ConfigEntityStora
    -   * @param \Drupal\Component\Uuid\UuidInterface $uuid_service
    

    Not sure I see the need of changing this property name.

  2. +++ b/core/lib/Drupal/Core/Database/Schema.php
    @@ -305,7 +305,7 @@ public function fieldExists($table, $column) {
    +   *   Alternatively, the 'initial_from_field' key may be used, which will
    

    Sneaking in a typo fix? ;)

  3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -154,11 +165,14 @@ public function getFieldStorageDefinitions() {
    +    $this->uuidGenerator = $uuid_service;
    

    uuid generator, or uuid service?
    I think service everywhere would reduce the amount of changes in this patch.

larowlan’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -100,16 +100,16 @@ class ConfigEntityStorage extends EntityStorageBase implements ConfigEntityStora
-  public function __construct(EntityTypeInterface $entity_type, ConfigFactoryInterface $config_factory, UuidInterface $uuid_service, LanguageManagerInterface $language_manager) {
+  public function __construct(EntityTypeInterface $entity_type, ConfigFactoryInterface $config_factory, UuidInterface $uuid_generator, LanguageManagerInterface $language_manager) {

As mentioned on IRC - this is an API break. We need to make it optional and put all access behind a protected accessor that falls back to the container singleton if not set. Joys of BC

larowlan’s picture

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -154,11 +165,14 @@ public function getFieldStorageDefinitions() {
-  public function __construct(EntityTypeInterface $entity_type, Connection $database, EntityManagerInterface $entity_manager, CacheBackendInterface $cache, LanguageManagerInterface $language_manager) {
+  public function __construct(EntityTypeInterface $entity_type, Connection $database, EntityManagerInterface $entity_manager, CacheBackendInterface $cache, LanguageManagerInterface $language_manager, UuidInterface $uuid_service) {

Gah, not that one, this one :)

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

acbramley’s picture

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -1022,6 +1038,11 @@ protected function saveRevision(ContentEntityInterface $entity) {
+      // Generating a new revision should generate a new revision UUID.
+      if ($this->revisionUuidKey) {
+        $record->{$this->revisionUuidKey} = $this->uuidGenerator->generate();
+      }

This code block is causing issues when importing normalized entities.

I'm trying to develop normalization for entity_reference_revisions using revision_uuids over at #2780395: ERR fields are not deployable but my revision_uuids were always being overwritten on import which I tracked down to this point.

benjy’s picture

Straight re-roll against 8.3.x, looking at some of the feedback now.

benjy’s picture

This patch tries to detect is the previous revision has the same Revision UUID as the one we're creating, this show allow imports although I haven't tested as suggested in #55

I've renamed uuid_generator back to uuid_service in all existing code to try and reduce the amount of noise and made the UUID Service optional to preserve BC as per #52

benjy’s picture

FileSize
13.24 KB

Interdiff

Status: Needs review » Needs work

The last submitted patch, 57: 1812202-57.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
704 bytes
38.48 KB
benjy’s picture

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -1053,8 +1057,9 @@ protected function saveRevision(ContentEntityInterface $entity) {
+      $generate_uuid = $this->revisionUuidKey && ((!$entity->original) || $entity->original->getRevisionUuid() === $record->revision_uuid);
+      if ($generate_uuid) {
+        $record->{$this->revisionUuidKey} = $this->uuidService->generate();

Looks like the tests are failing for reverting revisions, also this won't work for importing the very first revision.

We probably need a flag to indicate that we're doing an import, or a way to force the use of the existing UUID, thoughts on that? Could even be a new issue given core doesn't have a way currently to import revisions.

Status: Needs review » Needs work

The last submitted patch, 60: 1812202-60.patch, failed testing.

dixon_’s picture

Just had a quick chat with dawehner and timmillwood on IRC regarding UUID vs revision hashes (e.g. #2727511: WI: Add revision hash base field to all revisionable entities) as part of #2721129: Workflow Initiative...

Personally I see use cases for having both UUID and revision hashes. But if we want to keep the number of fields down, one possible solution would be to first commit this UUID issue as-is and then later down the line change how we generate revision UUID so that the UUID is deterministic (like described in #2727511: WI: Add revision hash base field to all revisionable entities).

If I recall correctly version 5 of the UUID spec allows for UUIDs to be deterministic based on namespaces and user data. This could be something we look into in a follow-up.

skwashd’s picture

UUIDv5 allows you to generate UUIDs using a URI. While it would be possible for Drupal to generate a UUID from a data URI, there would be quite a bit to the logic.

Firstly Drupal would need to convert the entity object to a "simple" object that contains the properties to be used for generating the data URI. For example the revision id, update timestamp and some other properties would need to be removed. These properties prevent the same change being made on different systems at different times from having the same UUID.

In order to keep the data consistent across platforms the simple object keys would need to be sorted. Some platforms order keys inconsistently. An example of this is the common dict object in Python.

The "simple" object would need to be serialised, using a cross platform serialisation format. JSON is probably the most portable choice here. A decision would need to be made about base64 encoding and the mime type for the uri. I propose we use data:application/vnd.drupal+json;base64,[base64-encoded-data] with base64 encoding being mandatory.

A namespace UUID would need to be generated for Drupal to use when generating the UUIDv5 UUIDs.

Any client that wanted to interoperate with Drupal would need to implement this logic if deterministic UUIDs was a requirement for that implementation. Others could use UUIDv4 knowing that revision matching couldn't be used.

This is my long way of saying that I think revision hashes and revision UUIDs should be decoupled, at least for now. Such an approach would allow work on this issue continue. The feature could be implemented as originally proposed with v4 UUID, like it is done today for the entity UUIDs. The value of data uri v5 UUIDs can be compared to continuing to use their v4 counterparts in a later stage of the workflow initiative.

Berdir’s picture

Reroll and moved generating the uuid to setNewRevision(), because the current place had a second problem. It actually only generated the new uuid on saveRevision(), at that point, it had already saved the old uuid to the {node} table (do we actually need the revision_uuid there, I'm not sure..)

Status: Needs review » Needs work

The last submitted patch, 65: add_uuid_support_for-1812202-65.patch, failed testing.

dawehner’s picture

+++ b/core/modules/system/system.install
@@ -1730,3 +1730,35 @@ function system_update_8201() {
+ * Install new revision_uuid schema.
+ */
+function system_update_8300() {
+  foreach (\Drupal::entityTypeManager()->getDefinitions() as $entity_type_id => $entity_type) {
+    /** @var \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface $manager */

Just a question: is adding a field and by that changing the schema fast enough to be executed for every entity type, or would adding a sandbox make sense?

benjy’s picture

I did run that upgrade hook a few times locally from the UI, it has been a few weeks so I can't remember how long it took but it never timed out. I'll give it another test if I can get the patch green.

Berdir’s picture

One problem with the update function that I wanted to bring up is that now that we don't have a default value for revision_uuid, it doesn't make too much sense to run it on all entity types, as we can't assume that they have one. It might make more sense to have a helper function or so that each module that adds it to its entity types can call in its own update function. That would also solve the "timeout" problem.

You might not have problems on small sites, but I have no idea what will happen if you try to run that on on a site with 600k or so nodes.

benjy’s picture

Issue tags: +Dublin2016
Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
    @@ -126,6 +126,7 @@ public static function open(array &$connection_options = array()) {
         $pdo->sqliteCreateFunction('regexp', array(__CLASS__, 'sqlFunctionRegexp'));
    +    $pdo->sqliteCreateFunction('UUID', array(__CLASS__, 'sqlFunctionUUID'));
    

    are we sure that the uuid method of the databases creates UUID's that are compatible with ours?

    Also makes me wonder if that could possibly be seen as a API change, as we now rely on a method that we didn't before, for backends not supported by core..

  2. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
    @@ -318,6 +318,17 @@ public static function sqlFunctionLikeBinary($pattern, $subject) {
    +    $generator = new \Drupal\Component\Uuid\Php();
    +    return $generator->generate();
    

    using the service is not an option here?

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -1138,6 +1156,11 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +    if ($entity_type->hasKey('revision_uuid')) {
    +      $fields[$entity_type->getKey('revision_uuid')] = BaseFieldDefinition::create('uuid')
    +        ->setLabel(new TranslatableMarkup('Revision UUID'))
    +        ->setReadOnly(TRUE);
    +    }
    

    shoudn't the field be marked as revisionable or something?

  4. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -288,10 +288,18 @@ public function __construct($definition) {
    +
    +    // If the entity type is revisionable, provide a default value for the
    +    // 'revision_uuid' key.
    +    if (!empty($this->entity_keys['revision']) && empty($this->entity_keys['revision_uuid'])) {
    +      $this->entity_keys['revision_uuid'] = 'revision_uuid';
    +    }
    

    Hum, I actually thought we'd removed that already.

    I'm not sure if we can do this. We need to be as BC compatible as possible, so I think entity types need to opt-in to this feature, possibly with a @todo for D9. Also we might end up adding this for entity types that aren't even revisionable and that's wrong too.

  5. +++ b/core/lib/Drupal/Core/Entity/RevisionableInterface.php
    @@ -40,6 +40,21 @@ public function setNewRevision($value = TRUE);
    +  public function getRevisionUuid();
    

    for other revision metadata, we added separate interfaces, we might need to do that here as well. will be annoying, but this is a fairly generic interface that might be implemented by something that isn't a content entity.

  6. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -126,7 +136,8 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
           $container->get('cache.entity'),
    -      $container->get('language_manager')
    +      $container->get('language_manager'),
    +      $container->get('uuid')
         );
    

    We actually did not inject this service so far on purpose. \Drupal\Core\Entity\EntityStorageBase::create() generates a uuid if the service is inject, content entities currently do that through the default value.

    might not be needed anymore now, now that I moved it to the entity class.

acbramley’s picture

The latest patch seems to fix my issues described in #55, nice work!

timmillwood’s picture

Status: Needs work » Needs review
FileSize
39.64 KB
1.52 KB

This should fix broken tests in #65.

timmillwood’s picture

FileSize
39.11 KB
2.64 KB

Trying to address items from #71.
1) Not sure
2) Updated
3) Aren't entity keys assumed revisionable? For example revision_id isn't even a revisionable base field, but still, we should make this a revisionable field.
4) Removed
5) Added RevisionUuidInterface
6) Not really sure I understand the issue here, will ping @Berdir.

Status: Needs review » Needs work

The last submitted patch, 74: 1812202-74.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
39.52 KB
1.04 KB

Forgot to add the RevisionUuidInterface.
This also removes the injected service as mentioned in #71.6.

Status: Needs review » Needs work

The last submitted patch, 76: 1812202-76.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
40.67 KB
932 bytes

It seems like #71.4 started causing issues because we assume revisionable entities types have a revision_uuid entity key but the core revisionable entity types (Node and BlockContent) don't. This patch fixes that, but we need to be backwards compatible for those that don't have a revision_uuid entity key (and tests for that).

Status: Needs review » Needs work

The last submitted patch, 78: 1812202-78.patch, failed testing.

timmillwood’s picture

This seems to have blown up many entity types. Think we need more checks in place, and to allow any entity type (revisionable or not) to not need a revision_uuid.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
25.04 KB

Removing the special case of revision_uuid from SqlContentEntityStorage and SqlContentEntityStorageTest.

timmillwood’s picture

After looking at the changes in #81 I'm thinking that maybe we should make revision_uuid a revision_metadata_key, which will mean committing #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table first.

Status: Needs review » Needs work

The last submitted patch, 81: 1812202-81.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
25.4 KB
1.71 KB

Making revision_uuid a revision metadata field.

Status: Needs review » Needs work

The last submitted patch, 84: 1812202-84.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
23.83 KB
1.57 KB

Fixed the two remaining files which were changes in the test made earlier in this issue that I've reverted.

timmillwood’s picture

@amateescu, @dixon_, and I discussed which table the revision_uuid field should be in. We came to the conclusion it should be the revision table (eg node_revision) and not the revision data table (eg node_field_revision).

This is what happens when revision_uuid is set as revision metadata, but it shouldn't really be revision metadata. So we need to rework the storage schema to make sure it is in the right place.

timmillwood’s picture

Here's an update which makes revision_uuid a "key" field but not a "metadata" field.

mysql> select * from node;
+-----+------+---------+--------------------------------------+----------+
| nid | vid  | type    | uuid                                 | langcode |
+-----+------+---------+--------------------------------------+----------+
|   1 |    3 | article | 0cc3be04-be9e-4015-9e7b-993da5386f69 | en       |
+-----+------+---------+--------------------------------------+----------+
1 row in set (0.00 sec)

mysql> select * from node_revision;
+-----+-----+----------+--------------------+--------------+--------------+--------------------------------------+
| nid | vid | langcode | revision_timestamp | revision_uid | revision_log | revision_uuid                        |
+-----+-----+----------+--------------------+--------------+--------------+--------------------------------------+
|   1 |   1 | en       |         1478705619 |            1 | NULL         | 5c211086-3391-418c-b895-2ca64a22d575 |
|   1 |   2 | en       |         1478705626 |            1 | NULL         | a5a4dabb-6bf6-4bb9-9668-a2e903b97391 |
|   1 |   3 | en       |         1478705634 |            1 | NULL         | 072ca9a5-6cd5-42f2-ac23-3b8981444cc4 |
+-----+-----+----------+--------------------+--------------+--------------+--------------------------------------+
3 rows in set (0.00 sec)

mysql> select * from node_field_revision;
+-----+-----+----------+--------------------------------------+--------+-------+-----+------------+------------+---------+--------+-------------------------------+------------------+
| nid | vid | langcode | revision_uuid                        | status | title | uid | created    | changed    | promote | sticky | revision_translation_affected | default_langcode |
+-----+-----+----------+--------------------------------------+--------+-------+-----+------------+------------+---------+--------+-------------------------------+------------------+
|   1 |   1 | en       | 5c211086-3391-418c-b895-2ca64a22d575 |      1 | test  |   1 | 1478705614 | 1478705619 |       1 |      0 |                             1 |                1 |
|   1 |   2 | en       | a5a4dabb-6bf6-4bb9-9668-a2e903b97391 |      1 | test1 |   1 | 1478705614 | 1478705626 |       1 |      0 |                             1 |                1 |
|   1 |   3 | en       | 072ca9a5-6cd5-42f2-ac23-3b8981444cc4 |      0 | test2 |   1 | 1478705614 | 1478705634 |       1 |      0 |                             1 |                1 |
+-----+-----+----------+--------------------------------------+--------+-------+-----+------------+------------+---------+--------+-------------------------------+------------------+
3 rows in set (0.00 sec)

mysql> select * from node_field_data;
+-----+-----+---------+----------+--------------------------------------+--------+-------+-----+------------+------------+---------+--------+-------------------------------+------------------+
| nid | vid | type    | langcode | revision_uuid                        | status | title | uid | created    | changed    | promote | sticky | revision_translation_affected | default_langcode |
+-----+-----+---------+----------+--------------------------------------+--------+-------+-----+------------+------------+---------+--------+-------------------------------+------------------+
|   1 |   3 | article | en       | 072ca9a5-6cd5-42f2-ac23-3b8981444cc4 |      0 | test2 |   1 | 1478705614 | 1478705634 |       1 |      0 |                             1 |                1 |
+-----+-----+---------+----------+--------------------------------------+--------+-------+-----+------------+------------+---------+--------+-------------------------------+------------------+
1 row in set (0.00 sec)

Status: Needs review » Needs work

The last submitted patch, 88: 1812202-88.patch, failed testing.

timmillwood’s picture

Think we need to change the key on the revision_uuid column, or special case it so it doesn't appear in data tables.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
25.2 KB
783 bytes

Looking at the option of non-unique UUIDs. Using the revisionable flag to manage this. So revisionable UUIDs are not unique, and non-revisionable ones are unique.

Status: Needs review » Needs work

The last submitted patch, 91: 1812202-91.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
25.84 KB
1.51 KB

Well... that was harder to track down that I thought it would be.

In \Drupal\Core\Entity\Sql\SqlContentEntityStorage::getTableMapping revision_uuid was getting added twice. Once as a revision key field, and once as a revisionable field. Wrapping the arrage_merge() for these in array_unique() seemed to do the trick.

acbramley’s picture

I'm trying to get this integrated with the Paragraphs and using Entity Pilot to export and import a node's full history.

Other patches in play are:
#2826237: Add support for revision_uuids
#2780395: ERR fields are not deployable
#2774463: Allow transportation of revisions

We were having issues with importing existing content, where it was trying to insert an existing revision (I tracked this down to SqlContentEntityStorage::saveRevision) which resulted in an Integrity constraint violation due to the revision_uuid existing already.

The latest patch seems to have fixed that for nodes but I'm still getting the same error with the Paragraph entities.

The interesting thing is the same (or similar?) issue seems to be causing my paragraphs patch to fail some tests (https://www.drupal.org/pift-ci-job/527174) so I'm not sure if this is due to the core patch or something I'm missing in the paragraphs patch. Any ideas?

acbramley’s picture

It seems that I missed the change to set the revision_uuid field as revisionable.

The bi-product of that now seems to be that existing revisions are duplicated on import instead of updating the existing revision.

In other words, there is no integrity constraint on the revision_uuid any more. I can continue importing existing content and generating duplicate revisions with the same revision_uuid.

EDIT: SQL Output:

I have 2 nodes with several revisions each.

mysql> select  r.nid, r.vid, d.revision_uuid, moderation_state from node_revision r join node_field_revision d on d.vid = r.vid;
+-----+-----+--------------------------------------+------------------+
| nid | vid | revision_uuid                        | moderation_state |
+-----+-----+--------------------------------------+------------------+
|   1 |   1 | 9828fbe9-76a9-4f90-9d29-fe9d8e57dff7 | draft            |
|   1 |   2 | cd921c37-4cca-4813-9afc-ef5dc9456ee2 | needs_review     |
|   1 |   3 | 0b743bf5-cd23-4767-9b87-53c34f4d25a8 | published        |
|   1 |   4 | 3d494d6b-c8db-4804-a149-6d3c990d96b1 | draft            |
|   1 |   5 | 4743711d-a1af-421a-ae3c-85b4c2ba7e1a | needs_review     |
|   1 |   6 | acbca334-a390-4b55-b50f-29e4105c4cda | published        |
|   2 |   7 | 8447869e-d07b-4ba6-96a5-158d0e6287f1 | needs_review     |
|   2 |   8 | eb5a346d-6513-481a-852c-76f240791902 | published        |
|   2 |   9 | b6840541-abd3-4737-b097-1fcc87bfc1d3 | draft            |
+-----+-----+--------------------------------------+------------------+
9 rows in set (0.00 sec)

I then import those same nodes again expecting to see no change. But:

mysql> select  r.nid, r.vid, d.revision_uuid, moderation_state from node_revision r join node_field_revision d on d.vid = r.vid;
+-----+-----+--------------------------------------+------------------+
| nid | vid | revision_uuid                        | moderation_state |
+-----+-----+--------------------------------------+------------------+
|   1 |   1 | 9828fbe9-76a9-4f90-9d29-fe9d8e57dff7 | draft            |
|   1 |   2 | cd921c37-4cca-4813-9afc-ef5dc9456ee2 | needs_review     |
|   1 |   3 | 0b743bf5-cd23-4767-9b87-53c34f4d25a8 | published        |
|   1 |   4 | 3d494d6b-c8db-4804-a149-6d3c990d96b1 | draft            |
|   1 |   5 | 4743711d-a1af-421a-ae3c-85b4c2ba7e1a | needs_review     |
|   1 |   6 | acbca334-a390-4b55-b50f-29e4105c4cda | published        |
|   1 |  10 | 9828fbe9-76a9-4f90-9d29-fe9d8e57dff7 | draft            |
|   1 |  11 | cd921c37-4cca-4813-9afc-ef5dc9456ee2 | needs_review     |
|   1 |  12 | 0b743bf5-cd23-4767-9b87-53c34f4d25a8 | published        |
|   1 |  13 | 3d494d6b-c8db-4804-a149-6d3c990d96b1 | draft            |
|   1 |  14 | 4743711d-a1af-421a-ae3c-85b4c2ba7e1a | needs_review     |
|   1 |  15 | acbca334-a390-4b55-b50f-29e4105c4cda | published        |
|   2 |   7 | 8447869e-d07b-4ba6-96a5-158d0e6287f1 | needs_review     |
|   2 |   8 | eb5a346d-6513-481a-852c-76f240791902 | published        |
|   2 |   9 | b6840541-abd3-4737-b097-1fcc87bfc1d3 | draft            |
|   2 |  16 | 8447869e-d07b-4ba6-96a5-158d0e6287f1 | needs_review     |
|   2 |  17 | eb5a346d-6513-481a-852c-76f240791902 | published        |
|   2 |  18 | b6840541-abd3-4737-b097-1fcc87bfc1d3 | draft            |
+-----+-----+--------------------------------------+------------------+
18 rows in set (0.00 sec)
amateescu’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
    @@ -88,6 +88,9 @@ public function getOriginalClass();
    +   *     revision UUID of the entity. Defaults to "revision_uuid" if the
    +   *     'revision' key is set.
    

    This is not true anymore :)

  2. +++ b/core/lib/Drupal/Core/Entity/RevisionUuidInterface.php
    @@ -0,0 +1,25 @@
    +   * Gets the revision universally-unique identifier of the entity.
    

    -> of the entity revision.

  3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -298,11 +298,11 @@ public function getTableMapping(array $storage_definitions = NULL) {
    -      $revision_metadata_fields = array_intersect(array(
    +      $revision_metadata_fields = array_intersect([
    ...
    -      ), $all_fields);
    +      ], $all_fields);
    

    This change is out of scope :)

  4. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -317,7 +317,10 @@ public function getTableMapping(array $storage_definitions = NULL) {
    -        $table_mapping->setFieldNames($this->revisionTable, array_merge($revision_key_fields, $revisionable_fields));
    +        if ($this->entityType->hasKey('revision_uuid')) {
    +          $revision_key_fields[] = $this->entityType->getKey('revision_uuid');
    +        }
    +        $table_mapping->setFieldNames($this->revisionTable, array_unique(array_merge($revision_key_fields, $revisionable_fields)));
    

    Why do we need to add revision_uuid to $revision_key_fields?

  5. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -347,11 +350,14 @@ public function getTableMapping(array $storage_definitions = NULL) {
    +        if ($this->entityType->hasKey('revision_uuid')) {
    +          $revision_base_fields[] = $this->entityType->getKey('revision_uuid');
    +        }
    ...
    -        $table_mapping->setFieldNames($this->revisionDataTable, array_merge($revision_data_key_fields, $revision_data_fields));
    +        $table_mapping->setFieldNames($this->revisionDataTable, array_unique(array_merge($revision_data_key_fields, $revision_data_fields)));
    

    Same question here.

  6. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
    @@ -176,7 +176,6 @@ public function requiresFieldStorageSchemaChanges(FieldStorageDefinitionInterfac
    -      $storage_definition->getSchema() != $original->getSchema() ||
    

    We shouldn't remove this check.

  7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/UuidItem.php
    @@ -44,7 +44,10 @@ public function applyDefaultValue($notify = TRUE) {
    -    $schema['unique keys']['value'] = array('value');
    +    if (!$field_definition->isRevisionable()) {
    +      $schema['unique keys']['value'] = array('value');
    +    }
    +    $schema['columns']['value']['initial_from_field'] = 'UUID()';
    

    Why is this change needed?

    Also, using a db-level function as 'initial_from_field' is kind of a hack IMO :)

  8. +++ b/core/modules/comment/src/CommentStorage.php
    @@ -43,9 +44,11 @@ class CommentStorage extends SqlContentEntityStorage implements CommentStorageIn
    +   * @param \Drupal\Component\Uuid\UuidInterface $uuid_service
    +   *   The UUID service.
    ...
    -  public function __construct(EntityTypeInterface $entity_info, Connection $database, EntityManagerInterface $entity_manager, AccountInterface $current_user, CacheBackendInterface $cache, LanguageManagerInterface $language_manager) {
    -    parent::__construct($entity_info, $database, $entity_manager, $cache, $language_manager);
    +  public function __construct(EntityTypeInterface $entity_info, Connection $database, EntityManagerInterface $entity_manager, AccountInterface $current_user, CacheBackendInterface $cache, LanguageManagerInterface $language_manager, UuidInterface $uuid_service) {
    +    parent::__construct($entity_info, $database, $entity_manager, $cache, $language_manager, $uuid_service);
    
    @@ -59,7 +62,8 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    -      $container->get('language_manager')
    +      $container->get('language_manager'),
    +      $container->get('uuid')
    

    I don't see anything in the comment storage that uses the new service.

  9. +++ b/core/modules/system/system.install
    @@ -1768,3 +1768,35 @@ function system_update_8201() {
    +function system_update_8300() {
    +  foreach (\Drupal::entityTypeManager()->getDefinitions() as $entity_type_id => $entity_type) {
    +    /** @var \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface $manager */
    +    $manager = \Drupal::entityDefinitionUpdateManager();
    +
    +    // Install the new revision_uuid field.
    +    if ($entity_type->hasKey('revision_uuid')) {
    +      $field_manager = \Drupal::service('entity_field.manager');
    +      $field_storage_definitions = $field_manager->getFieldStorageDefinitions($entity_type_id);
    +      $manager->installFieldStorageDefinition($entity_type->getKey('revision_uuid'), $entity_type_id, $entity_type->getProvider(), $field_storage_definitions[$entity_type->getKey('revision_uuid')]);
    +    }
    +
    +    // Update the existing uuid field which has a new schema.
    +    if ($entity_type->hasKey('uuid') && $definition = $manager->getFieldStorageDefinition('uuid', $entity_type_id)) {
    +      $manager->updateFieldStorageDefinition($definition);
    +    }
    +  }
    +}
    

    - we need to hardcode the definition of the revision_uuid field instead of getting it from the field manager.

    - we need individual update functions for each entity type

    - the same individual update functions need to set the value of the revision_uuid field in the database instead of relying on the 'initial_from_field' stuff

timmillwood’s picture

#96.4 & #96.5 - So the column is added to the revision table (as well as the revision data tables).

#96.7 - So that revision_uuid isn't set as unique. A unique revision_uuid in the data tables causes issues for multilingual sites.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
24.05 KB
7.02 KB

Addressing items from #96 apart form 7 & 9.

Status: Needs review » Needs work

The last submitted patch, 98: 1812202-98.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
24.85 KB
3.93 KB

- Moved the update hook to node.install and block_content.install.
- Prevented the uuid field update by only adding the initial_from_field on revisionable uuid fields.

Can't think of a good solution to removing the use of initial_from_field.

Status: Needs review » Needs work

The last submitted patch, 100: 1812202-100.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
Issue tags: +Workflow Initiative
FileSize
25.5 KB
671 bytes

Fixing REST test failures.

amateescu’s picture

Can't think of a good solution to removing the use of initial_from_field

Is there anything wrong with generating the uuids in PHP and populating the reivision_uuid table column in update functions?

timmillwood’s picture

After speaking to @amateescu we agreed that UUID() in SQL is not the best option for anything, let alone initial_from_field. The biggest realization for me was that we need the same revision_uuid for each language of a revision.

The solution we are looking at now is:

  1. Set the initial_from_field to uuid, so that all revisions get the revision_uuid from the uuid of the entity.
  2. Within hook_update run a batch process to loop through all revisions and do a bulk update based on revision id.
Berdir’s picture

Doing that in hook_update_N() would mean hours of downtime for a site with 100k's of nodes. That's a serious problem.

We once discussed in IRC the idea of doing this as some kind of background job. file_entity has a similar thing where it converts files to have a bundle. That doesn't scale well either though as it saves each ID into a queue.

acbramley’s picture

Can I get a response to #95, the idea of having non unique revision_uuids is going to completely break the use case for us.

timmillwood’s picture

FileSize
25.24 KB
6.39 KB

Just a progress update
- Revision uuid is unique again
- Revision uuid is only in the revision table and not the data table
- The initial revision uuid is the revision id

Todo
- Update the revision uuid field with a uuid
- Run the update on cron
- Add a status page to show update progress and manually run it
- Write tests

Status: Needs review » Needs work

The last submitted patch, 107: 1812202-107.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
30.16 KB
6.16 KB

Before I sign off for the week
- Implemented EntityFieldValueUpdater class to be a general place we can put updaters in the future.
- updateRevisionUuid class loops through all [revision_id => entity_id] items in the array and updates the revision_uuid
- if there are less than 100 to update this is called from hook_update, otherwise it's stored in state
- hook_cron gets the array from state and loops through 100 at a time.

Todo:
- Add a status page to show update progress and manually run it
- Write tests

Status: Needs review » Needs work

The last submitted patch, 109: 1812202-109.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
5.12 KB
33.87 KB

- Added a hacky fix for the views issue
- Setup upgrade tests for node and block_content
- Introduced a status report item to show if there are "field value updates"

Todo:
- Add a route / controller to run updates manually
- Add a test with more than 100 node / block_content entities

#2721313: Upgrade path between revisionable / non-revisionable entities adds a database dump with more entities, which would be good to test this.

Status: Needs review » Needs work

The last submitted patch, 111: 1812202-111.patch, failed testing.

timmillwood’s picture

FileSize
34.06 KB
2.77 KB
timmillwood’s picture

Status: Needs work » Needs review
timmillwood’s picture

Status: Needs review » Needs work

Todo:
- Add a route / controller to run updates manually
- Add a test with more than 100 node / block_content entities

Berdir’s picture

+++ b/core/modules/block_content/block_content.module
@@ -105,3 +106,15 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
+ */
+function block_content_cron() {
+  $revisions = \Drupal::state()->get('block_content:revision_uuid_update');
+  if (!empty($revisions)) {
+    $entity_field_value_updater = new EntityFieldValueUpdater(\Drupal::entityTypeManager()->getStorage('block_content')->getEntityType());
+    $remaining_revisions = $entity_field_value_updater->updateRevisionUuid($revisions);
+    \Drupal::state()->set('node:revision_uuid_update', $remaining_revisions);
+  }

This also won't scale to 600k entries. We can query revisions without a uuid, all we need is a list of entity_types to process IMHO. Then we fetch N revisions without a UUID and if we're done (less than 100 found), we remove that key.

Also doesn't need to be entity type specific, could be in system_cron().

hchonov’s picture

So if I've disabled cron my entities would never be updated or if I do the update and want to make use of the feature it would be not possible right away but I might need to wait days depending on how many revisions I have and how often my cron is running? Why not doing this instead in a batch job during the update?

timmillwood’s picture

@hchonov - @berdir suggested that running all this in an update hook, even as a batch job will be slow for hundreds of thousands or millions of entities, therefore taking a live site down for a while. I'm planning on adding a route linked from the status report to run the update on all entity.

Berdir’s picture

Because a site is in maintenance mode while updates are running and *down*.

We can always add a drush command or even a UI to force-execute it.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
31.95 KB
7.27 KB

Reworked based on #116.

Still need to build a manual way of running the update.

(Don't think it works, but need to head out, so kinda a work in progress)

Status: Needs review » Needs work

The last submitted patch, 120: 1812202-120.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
31.84 KB
4.59 KB

Fixed the tests, and added a manual way to run the updates.
Not fully happy with the implementation, also need to switch the manual updater to run as a batch.

Could do with a test of more than 100 entities.

Status: Needs review » Needs work

The last submitted patch, 122: 1812202-122.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
35.72 KB
4.41 KB

Missed some file from the patch.

timmillwood’s picture

timmillwood’s picture

Been looking into the best way to batch process the field value updates. I guess in the controller, but then there are dependency injection issue. Are there any good examples of how to batch process things like this?

Status: Needs review » Needs work

The last submitted patch, 125: 1812202-125.patch, failed testing.

amateescu’s picture

If you need to run a method in a batch process, it's usually easier if you make it static and access services through \Drupal:: and friends :)

larowlan’s picture

<plug class="shameless">I wrote a bit about that https://www.previousnext.com.au/blog/refactoring-drupal-batch-api-callba...</plug>

Berdir’s picture

That's what I did in simplenews:

/**
 * Batch callback to dispatch batch operations to a service.
 */
function _simplenews_batch_dispatcher() {
  $args = func_get_args();
  list($service, $method) = explode(':', array_shift($args));
  call_user_func_array(array(\Drupal::service($service), $method), $args);
}

// This is how it is used:
      $operations = array();
      for ($i = 0; $i < $num_operations; $i++) {
        $operations[] = array('_simplenews_batch_dispatcher', array('simplenews.mailer:sendSpool', $throttle, $conditions));
      }

      // Add separate operations to clear the spool and update the send status.
      $operations[] = array('_simplenews_batch_dispatcher', array('simplenews.spool_storage:clear'));
      $operations[] = array('_simplenews_batch_dispatcher', array('simplenews.mailer:updateSendStatus'));

Could be a static method too. But of course, we could just simply add support for the controller resolver in batch API, should be easy enough :)

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
35.22 KB

re-roll of #125.

Status: Needs review » Needs work

The last submitted patch, 132: 1812202-132.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
34.89 KB

Rerrolled.

Status: Needs review » Needs work

The last submitted patch, 134: 1812202-134.patch, failed testing.

lammensj’s picture

I could not apply the latest patch due to a faulty path to FilterUidRevisionTest.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

timmillwood’s picture

Status: Needs review » Needs work

The last submitted patch, 138: 1812202-138.patch, failed testing. View results

timmillwood’s picture

Status: Needs work » Needs review
FileSize
32.94 KB

Helps if I properly resolve conflicts.

Status: Needs review » Needs work

The last submitted patch, 140: 1812202-140.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dawehner’s picture

Just a early in the morning review ...

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -18,7 +18,7 @@
    @@ -291,6 +291,20 @@ public function setNewRevision($value = TRUE) {
    
    @@ -291,6 +291,20 @@ public function setNewRevision($value = TRUE) {
    +
    +      if ($this->getEntityType()->hasKey('revision_uuid')) {
    +        // Also generate a new revision uuid.
    +        $this->set($this->getEntityType()->getKey('revision_uuid'), \Drupal::service('uuid')->generate());
    +      }
    

    Here is a question:

    $node->setNewRevison(TRUE);
    $node->setNewRevison(TRUE);
    

    Both times this call happens a new UUID is generated.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -291,6 +291,20 @@ public function setNewRevision($value = TRUE) {
    +      // Make sure that the flag tracking which translations are affected by the
    +      // current revision is reset.
    

    It would be great to not document what is happening, but rather why.

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -1094,6 +1115,12 @@ public function createDuplicate() {
    +      // Check if the entity type supports revision UUIDs and generate a new one
    +      // if so.
    +      if ($entity_type->hasKey('revision_uuid')) {
    +        $duplicate->{$entity_type->getKey('revision_uuid')}->value = $this->uuidGenerator()->generate();
    +      }
    

    Here its using $duplicate->{}, the other method above is using $this->set() ... maybe make those two lines more consistent.

  4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -1386,6 +1419,10 @@ public function hasTranslationChanges() {
    +          $field_name == $this->getEntityType()->getKey('revision_uuid')) {
    

    Let's be safe and use a triple equal

  5. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -53,6 +53,13 @@ class SqlContentEntityStorage extends ContentEntityStorageBase implements SqlEnt
    +   * @var string|bool
    ...
    +  protected $revisionUuidKey = FALSE;
    
    @@ -176,6 +183,7 @@ protected function initTableLayout() {
    +    $this->revisionUuidKey = NULL;
    

    So the revision uuid key can be NULL|bool|string ;) Just imagine if PHP would actually have generics, what a nice world this could be.

  6. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
    @@ -1370,7 +1370,9 @@ protected function createSharedTableSchema(FieldStorageDefinitionInterface $stor
    +                // Check if the unique key exists because it might already have
    +                // been created as part of the earlier entity type update event.
    +                $this->addUniqueKey($table_name, $name, $specifier);
    

    The documenation seems to document something which is not really visible in the code (anymore)?

  7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/UuidItem.php
    @@ -46,6 +47,19 @@ public function applyDefaultValue($notify = TRUE) {
    +    catch (ServiceNotFoundException $exception) {
    +      // Just a hack to get \Drupal\Tests\views\Unit\EntityViewsDataTest passing.
    +    }
    

    Why can we not mock the entity type manager?

hchonov’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -291,6 +291,20 @@ public function setNewRevision($value = TRUE) {
    +      if ($this->getEntityType()->hasKey('revision_uuid')) {
    +        // Also generate a new revision uuid.
    +        $this->set($this->getEntityType()->getKey('revision_uuid'), \Drupal::service('uuid')->generate());
    +      }
    

    In order to cover what Daniel has mentioned in #142.1 and don't regenerate the value each time setNewRevision is called and additionally don't loose it on
    1. setNewRevision(TRUE)
    2. setNewRevision(FALSE)
    we have to move this piece of code to the storage and create the revision UUID before the entity is saved. We should not make irreversible changes in setNewRevision() and this is why we've moved the logic for the revision translation affected flag away to the storage - see next point.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -291,6 +291,20 @@ public function setNewRevision($value = TRUE) {
    +      // Make sure that the flag tracking which translations are affected by the
    +      // current revision is reset.
    +      foreach ($this->translations as $langcode => $data) {
    +        // But skip removed translations.
    +        if ($this->hasTranslation($langcode)) {
    +          $this->getTranslation($langcode)->setRevisionTranslationAffected(NULL);
    +        }
    +      }
    

    We've recently moved this piece of logic to the storage. Is there a specific reason for bringing it back or something went wrong during re-rolling the patch? :)

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -1094,6 +1115,12 @@ public function createDuplicate() {
    +      // Check if the entity type supports revision UUIDs and generate a new one
    +      // if so.
    +      if ($entity_type->hasKey('revision_uuid')) {
    +        $duplicate->{$entity_type->getKey('revision_uuid')}->value = $this->uuidGenerator()->generate();
    +      }
    

    What about just setting it to null and letting the storage take care of it on entity save? I think that this way it will be consistent with 1.).

  4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -1386,6 +1419,10 @@ public function hasTranslationChanges() {
    +      if ($this->getEntityType()->hasKey('revision_uuid') &&
    +          $field_name == $this->getEntityType()->getKey('revision_uuid')) {
    +        continue;
    +      }
    

    If we put the revision uuid key to the revision metadata keys in the entity annotation it will get automatically excluded from CEB::hasTranslationChanges(). This happens in CEB::getFieldsToSkipFromTranslationChangesCheck().

  5. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -299,11 +299,13 @@ public function __construct($definition) {
    +      'revision_uuid' => '',
    

    I think we could consider this more of a revision metadata key instead of an entity key - see the previous point for advantages.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Issue tags: +API-First Initiative

Is this still intended to happen? This would have a huge impact on #2992833: Add a version negotiation to revisionable resource types.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

thursday_bw’s picture

I've had a go at updating this patch to work with 8.8.x-dev

I have a patch that applies, but it's buggy and definitely needs work. It's a step in the right direction though.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.