Add a revision_parent field to revisionable entities

It looks like this is probably a blocker for #2992833: Add a version negotiation to revisionable resource types — i.e. for JSON API (but also core REST and GraphQL) to implement revision support. For revision support to be implementable, we need to be able to reliably determine what to link to. We need to implement RFC5829: Link Relation Types for Simple Version Navigation between Web Resources. That includes for example a working-copy-of link relation type. That means we need to know, given a revision, which other revision it was based on. Which of course leads me to this issue.

Also, this is actually part of phase B of the Workflow Initiative, based on the sign-offs given in #2725433. See #2786133: WI: Phase B: Extend the revision API with support for parents. Updating issue metadata accordingly.

It'd be very helpful if we could already start discussing what the upgrade path will look like. Because today, we do not have revision parent information (hence this issue), which means we'll have to construct a past.

Do we go with the simplest possible assumption? That'd be assuming that the previous revision by ID always is the revision parent. Which may be inaccurate. But it's a fair best-effort guess.

Do we go with the simplest possible assumption? That'd be assuming that the previous revision by ID always is the revision parent. Which may be inaccurate. But it's a fair best-effort guess.

The only time this assumption might be inaccurate is for revision reverts (i.e. when a new revision is being creating by "reverting" to the data of a past revision), but this case is impossible (read: complicated to the point that it's not even worth trying) to determine so, yes, we'll have to go with the assumption that the previous revision ID is the parent.

Perfect, thanks! Updated the IS.

Here's a patch for adding the revision_parent field :)

Nevermind those test fails for now, the patch needs architectural review so putting back to NR.

High-level review to kick off the review process :) Reviewing for high-level understandability. Reviewed the validation logic, but didn't review all test cases just yet.

1. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
   @@ -225,6 +225,14 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
   +      $field_name = $entity_type->getRevisionMetadataKeys(FALSE)['revision_parent'];
   +      $base_field_definitions[$field_name] = BaseFieldDefinition::create('revision_tree')
   +        ->setLabel($this->t('Revision parent'))
   
   +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidRevisionTreeReferenceConstraint.php
   @@ -0,0 +1,40 @@
   + *   id = "ValidRevisionTreeReference",
   + *   label = @Translation("Valid parent revision reference", context = "Validation")
   

   🤔 parent s tree vs parent vs tree … I'm confused!

2. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
   @@ -225,6 +225,14 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
   +        ->setInternal(TRUE)
   

   👍 Not 100% sure if this should be internal, but I think it makes sense to at least make it internal at first and then consider making it non-internal later. Perfect :)

3. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidRevisionTreeReferenceConstraintValidator.php
   @@ -0,0 +1,90 @@
   +class ValidRevisionTreeReferenceConstraintValidator extends ConstraintValidator implements ContainerInjectionInterface {
   ...
   +   * Constructs a ValidReferenceConstraintValidator object.
   

   Nit: mismsatch.

4. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidRevisionTreeReferenceConstraintValidator.php
   @@ -0,0 +1,90 @@
   +    /** @var \Drupal\Core\Field\FieldItemInterface $value */
   

   Can we instead do an assert()?

5. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidRevisionTreeReferenceConstraintValidator.php
   @@ -0,0 +1,90 @@
   +    /** @var \Drupal\Core\Entity\Plugin\Validation\Constraint\ValidRevisionTreeReferenceConstraint $constraint */
   

   Same here. And several more.

6. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidRevisionTreeReferenceConstraintValidator.php
   @@ -0,0 +1,90 @@
   +      if ($parent_revision_id == $merge_revision_id) {
   

   🤔 Can we use a strict comparison?

7. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidRevisionTreeReferenceConstraintValidator.php
   @@ -0,0 +1,90 @@
   +      // Third, check that the assigned parent and the merge parent actually
   

   Nit: s/actually//

8. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/RevisionTreeItem.php
   @@ -0,0 +1,148 @@
   +    // When the parent revision ID is assigned manually, we have to skip the
   

   👍

9. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/RevisionTreeItem.php
   @@ -0,0 +1,148 @@
   +    // auto-assign code from ::preSave().
   

   Nit: s/from//in/

Is the idea to have a field with two properties - one for the revision ID of the parent and one for the revision ID of an entity revision that has been merged into the current one?

Why do we need a field with two properties instead of two dedicated fields for each task?

When adding some reference by revision, then it would be better to keep the already established naming standard from entity_reference_revisions and refer to the revision property as target_revision_id and not target_id, because otherwise it will get confusing.

Also without having the entity ID of the parent as a dedicated property it becomes harder to prepare entity queries for searching for the children of a specific entity ID. Therefore it would be better to include this information as well.

I personally think that it would be great to include EntityReferenceRevisionsItem from the entity_reference_revisions module into core and use it instead. This is a desired feature for a long time now and this here would be a great opportunity for it.

But I guess that there are reasons for not taking this approach?

Thank you both for starting the review process :)

Re #18:

1. The field type (plugin) is called revision_tree, which has a "valid revision tree" constraint, and the name of the base field that's added to all revisionable entity types is revision_parent :) I changed the description of the constraint, maybe that was the confusing part?
2. Exactly my thought process.
3. Fixed.
4. We could, but since assert() has a runtime cost, however small that might be, and these comments are there purely for IDE autocompletion, I opted not to use it..
5. Same as above.
6. Same old Entity API story, we can't use strict comparison on field values :)
7. Fixed.
8. :)
9. Fixed.

Re #19:

Is the idea to have a field with two properties - one for the revision ID of the parent and one for the revision ID of an entity revision that has been merged into the current one?

Yup :)

Why do we need a field with two properties instead of two dedicated fields for each task?

Because conceptually this is a multi-value field, but, since we know that we can only store a maximum of two parents, having two columns for them is a performance optimization. It also allows us to store it in the shared tables, removing the need for joining a dedicated field table.

When adding some reference by revision, then it would be better to keep the already established naming standard from entity_reference_revisions and refer to the revision property as target_revision_id and not target_id, because otherwise it will get confusing.

Since this field type is not based on ERR on purpose (more on that below), I think it would be equally confusing to have the same property name as ERR. If we really want to differentiate somehow from both ER and ERR, how about using parent_revision_id and merge_revision_id for the property names?

Also without having the entity ID of the parent as a dedicated property it becomes harder to prepare entity queries for searching for the children of a specific entity ID. Therefore it would be better to include this information as well.

I don't get this point, you can add a condition on the entity ID field to the entity query just like on any other field.

I personally think that it would be great to include EntityReferenceRevisionsItem from the entity_reference_revisions module into core and use it instead. This is a desired feature for a long time now and this here would be a great opportunity for it.

But I guess that there are reasons for not taking this approach?

That's actually how I started to write this field type, but, after wasting quite some time fighting (overriding) the behavior of the parent class(es), I realized that we're much better off with a custom field type dedicated to the task at hand: building a tree/graph of revisions.

Here's a non-exhaustive list of reasons for not using ERR as a base class for this field type:

1. having an entity ID property would give the impression that this field type could reference the revision of an arbitrary entity, but we want to be able to reference revisions from a *single* entity
2. similar to above, having a target_type storage setting would give the impression that the target type is configurable, when in fact it is restricted (hardcoded) to the entity type that contains the field
3. we can't use *any* code related to ER: selection plugins, auto-create support, widgets, formatters, etc. because they are designed to work with entity IDs, not revision IDs
4. while writing the field type on top of ERR, I found that I had to override almost every method of the parent class, to the point where it wasn't using even 10% of its code

how about using parent_revision_id and merge_revision_id for the property names?

That would mean some funny column names though: revision_parent__parent_revision_id and revision_parent__merge_revision_id :/ IMO, the existing property names from the patch are clear enough.

And here's one of the most useful functionalities enabled by revision parents: the ability to find the lowest common ancestor (LCA) of two revisions, which is the stepping stone for proper conflict management :)

If the full patch gets too hard to read with this addition, maybe it should be posted in a followup issue. I'll let that up to reviewers to decide.

Fixed a typo so I'm not sending this updated patch to the testbot.

Méh..

having an entity ID property would give the impression that this field type could reference the revision of an arbitrary entity, but we want to be able to reference revisions from a *single* entity

Well, we already have exactly this use case, where we decouple an entity and keep a reference to its parent and the revision from which the entity was decoupled, which is a different entity. We use that to track changes on the parent and through conflict management update the child. Both entities are allowed to change.
I think this use case demonstrates, why it is important to not have a constraint on the parent. It sounds logical to derive an entity from another one and keep track of the origin through the parent field.
Currently we also update the parent revision reference when me merge a newer parent revision into the child. We don't use a merged revision field, because we don't see any reason for it. What would the core reason be for that? Our argument is that after a merge the entity has a new parent, because the parent changes have been merged into the derived entity and the initial parent revision doesn't anymore represent a real parent of the updated child. This is why if the initial parent revision was 5 and later we merged revision 7 into the child, then we update the parent field to point at revision 7.

And here's one of the most useful functionalities enabled by revision parents: the ability to find the lowest common ancestor (LCA) of two revisions, which is the stepping stone for proper conflict management :)

Hmmm, but we already have a proper and completely functioning conflict management provided by the 2.x branch of the conflict module as presented at Drupal Europe Darmstadt 2018. And here is the recorded session on YouTube - https://youtube.com/watch?v=ixlLskzT9c4

I am confused by your comment. Probably you weren't aware of my work on the conflict module? Should we arrange a call and talk about all this? Because at work we are in a pretty advanced state regarding parent tracking and conflict resolutions :).

we can't use *any* code related to ER: selection plugins, auto-create support, widgets, formatters, etc. because they are designed to work with entity IDs, not revision IDs
while writing the field type on top of ERR, I found that I had to override almost every method of the parent class, to the point where it wasn't using even 10% of its code

Actually by saying

I personally think that it would be great to include EntityReferenceRevisionsItem from the entity_reference_revisions module into core and use it instead. This is a desired feature for a long time now and this here would be a great opportunity for it.

I've only meant that we need EntityReferenceRevisionsItem. We don't need any other logic. So we don't have to build on top of it, but we can copy the relevant parts - like the properties, the data type, the schema, the setter and getter methods. This would be enough for start.

It sounds logical to derive an entity from another one and keep track of the origin through the parent field.

I'm sorry but this actually sounds very project-specific and not a use case for core.

Currently we also update the parent revision reference when me merge a newer parent revision into the child. We don't use a merged revision field, because we don't see any reason for it. What would the core reason be for that?

Let me quote from https://en.wikipedia.org/wiki/Merge_(version_control):

In some cases, the merge can be performed automatically, because there is sufficient history information to reconstruct the changes, and the changes do not conflict. In other cases, a person must decide exactly what the resulting files should contain.

The field type introduced by this patch provides a way to efficiently store the Directed Acyclic Graph of an entity's revision history, as well as the capability to retrieve the Lowest Common Ancestor of two revisions, as needed by the three-way merge process. Having the ability to do a three-way merge during conflict resolution is very much needed because it maximizes the chances for automatic merging, thus providing a better UX for content editors.

I've only meant that we need EntityReferenceRevisionsItem. We don't need any other logic.

Even a "stripped down" EntityReferenceRevisionsItem would still have to extend core's EntityReferenceItem, which comes with the unnecessary baggage and downsides as pointed out in #20.

Proper tracking of a revision's parent does not just allow for proper conflict management, but also for proper tracking and audit trails.

Tracking the parent revision also allows for full 3 way merges, and potentially automated conflict management. As well as revision graphs and the calculation of the lowest common ancestor. All of which makes Drupal increasingly powerful.

I fully support the current solution in this issue.

I'm sorry but this actually sounds very project-specific and not a use case for core.

Could you state your arguments please? Drupal is a CMS, which has an API for duplicating an entity. The duplicated entity has all the data from entity it was duplicated from, so the intensions are to use it as a starting point. For having a complete history the CMS would have to track that information, but Drupal currently doesn't. Therefore I think, that this makes it a core feature.
If for example we take a food magazine, then we create a duplicate of some recipe and want to offer different extended variants by the users, which create a duplicate and extend/modify the new recipe, but a link between the recipes is automatically created. This actually sounds like a really great out of the box feature for Umami and for presenting Drupal.
I think that this feature might be used by a lot of projects and as such, it would be good to be part of the Drupal core.

In some cases, the merge can be performed automatically, because there is sufficient history information to reconstruct the changes, and the changes do not conflict. In other cases, a person must decide exactly what the resulting files should contain.

In my previous comment, I've made a reference to the conflict module and to the session about it. I am quoting the following from the module's page - https://www.drupal.org/project/conflict :

Conflicts which will be auto merged:
-changes in translatable fields in non-edited content entity translations
-changes in fields to which the user does not have access
--fields with no edit access
--fields not part of the entity form display
-changes in entity metadata
--revision ID
--changed timestamp

And we already do that and it works without any core additions.

Having the ability to do a three-way merge during conflict resolution is very much needed because it maximizes the chances for automatic merging, thus providing a better UX for content editors.

I know what a three-way merge is, because I've already implemented it in the the conflict module. There it is currently limited to the use case of concurrent editing. It keeps track of the initial entity used to build the form (yes, this is something that we need to solve more efficiently there). The second entity that is required is the current entity resulting from the form values and the third one is the newest entity from the storage.

-----

I guess, that you are talking about merging two entities from two different revision trees/branches? In that case which one would become the new parent? Realistically there would be two parent revisions then.
What happens if we merge rev 17 and rev 35 from two different revision branches and through this create rev 41. Now we want to merge rev 41 with a revision from a third revision branch, which has a common ancestor with either rev 17 or rev 35, but not with both of them.

Beside that question, I think that you could use the conflict module out of the box, if you provide it with the 3 required entities. In your case the entity from the storage matches the entity from the first revision branch, the edited entity the one from the second revision branch and the initial entity matches the common ancestor.

The field type introduced by this patch provides a way to efficiently store the Directed Acyclic Graph of an entity's revision history

Is it more efficient than an approach that would be using EntityReferenceRevisionsItem instead? If so, then why?

Even a "stripped down" EntityReferenceRevisionsItem would still have to extend core's EntityReferenceItem, which comes with the unnecessary baggage and downsides as pointed out in #20.

I don't think so, because we can at first create the new field type without UI, exactly like the one from your patch. What downsides exactly do you see in this case?
I haven't checked exactly, but probably it might be even possible to extend our current EntityReferenceItem to support revisions through a setting. Having the setting by default turned off would not be a BC break. Do you know if anybody has given this approach a try?

@timmillwood, I am sorry, but I've somehow missed the intention of your comment. Nobody here is arguing against parent tracking and all of us want to have it. We are only discussing about the approach.

Could you state your arguments please?

That's my opinion, but it's up to framework and product managers to decide whether cross-entity revision graphs are a desirable feature for core.

I guess, that you are talking about merging two entities from two different revision trees/branches? In that case which one would become the new parent?

I am talking about merging two revisions of the same entity, for example edited by two users at the same time, possibly in different worskpaces as well. Both these cases mean that the revisions belong to different branches of the graph.

Realistically there would be two parent revisions then.

Not sure whether that's a question or not, but yes, that's why we need to track the "merge parent" in addition to the parent revision.

What happens if we merge rev 17 and rev 35 from two different revision branches and through this create rev 41. Now we want to merge rev 41 with a revision from a third revision branch, which has a common ancestor with either rev 17 or rev 35, but not with both of them.

I can answer that question only if you post the full revision graph.

Is it more efficient than an approach that would be using EntityReferenceRevisionsItem instead? If so, then why?

Because we store both parents in the same table (the entity type's revision table), instead of requiring a separate dedicated field table for each revisionable entity type.

I don't think so, because we can at first create the new field type without UI, exactly like the one from your patch. What downsides exactly do you see in this case?
I haven't checked exactly, but probably it might be even possible to extend our current EntityReferenceItem to support revisions through a setting. Having the setting by default turned off would not be a BC break. Do you know if anybody has given this approach a try?

I already said that this is how I started writing this field type, but realized that it's the wrong path to take and started with a fresh field type. Since you keep insisting on this point, here's the full list of methods from EntityReferenceItem that we would need to override:

• defaultStorageSettings() because we already know the target type
• defaultFieldSettings() because we can't use selection handlers
• propertyDefinitions() because we need to add the merge_target_id property and remove the entity property
• schema() same as above
• setValue() because we don't need to keep target_id in sync with entity
• getValue() because we don't support the auto-create feature (the hasNewEntity() call)
• onChange() same as setValue()
• isEmpty() because empty items are the roots of the tree, and we need to be able to store NULL values in the database for them
• preSave() same as getValue()
• generateSampleValue() because we need to generate sample parent IDs differently
• storageSettingsForm() because we don't have the target_type storage setting
• fieldSettingsForm() same as above and defaultFieldSettings()
• hasNewEntity() because we don't have the auto-create feature
• calculateDependencies() because we don't have default values or other dependencies
• calculateStorageDependencies() because we don't have a configurable target type
• onDependencyRemoval() same as above
• getPossibleValues(), getPossibleOptions(), getSettableValues() and getSettableOptions() because we don't expose any kind of UI widgets so we don't need to implement OptionsProviderInterface
• getPreconfiguredOptions() because we don't expose this field type in the UI

Which means that there are only two methods from EntityReferenceItem that we would use as-is: mainPropertyName() and getConstraints(), and the former might need to be overridden as well if we decide to not use target_id as the main property name.

It seems that my assessment from #20 was wrong, we wouldn't even use that 10% of the code from ER but more like 1%, and we would still have all the baggage that comes along with ER.

Adding the "Needs framework manager review" tag since #2786133: WI: Phase B: Extend the revision API with support for parents still has it.

Raising this to Major, since it sounds like it could potentially help with a Critical (#2950869-25: Entity queries querying the latest revision very slow with lots of revisions) and with API-First (per #10, but I'm not sure if that's still accurate).

Kicking in a review of this, for what it's worth. Looks like really solid work 👍

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -398,6 +409,90 @@ public function getLatestTranslationAffectedRevisionId($entity_id, $langcode) {
   +    $result = $this->getQuery()
   +      ->condition($this->idKey, $entity->id(), '=')
   +      ->sort($revision_key, 'ASC')
   +      ->allRevisions()
   +      ->execute();
   

   Should this skip access checking?

2. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
   @@ -225,6 +225,14 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
   +      $base_field_definitions[$field_name] = BaseFieldDefinition::create('revision_tree')
   

   Hm, the field isn't actually a whole tree is it? Just a reference to a single parent. Naming seems kind of confusing.

3. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
   @@ -225,6 +225,14 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
   +        ->setTranslatable(FALSE)
   

   Have we validated that the parent field should be non-translatable with some tests that demonstrate how the field is populated when translations are added?

   Just based on intuition, it feels like it should be translatable. Creating new revisions of a de translation, doesn't seem like it should change the parent of the en translation, if revision_translation_affected is FALSE.

4. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
   --- a/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueContentEntityStorage.php
   +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueContentEntityStorage.php
   

   We really should document somewhere that this entity storage is not really suitable for anything. I feel like I've seen it being used in the wild, where it really shouldn't be.

5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/RevisionTreeItem.php
   @@ -0,0 +1,148 @@
   +/**
   + * Defines the 'revision_tree' field type.
   + *
   + * @FieldType(
   + *   id = "revision_tree",
   + *   label = @Translation("Revision tree"),
   + *   description = @Translation("An entity field containing a revision tree item."),
   + *   no_ui = TRUE,
   + *   cardinality = 1,
   + *   constraints = {"ValidRevisionTreeReference" = {}}
   + * )
   + */
   +class RevisionTreeItem extends FieldItemBase {
   

   Thinking about the naming a bit more, is this actually a node in the tree? That is, maybe "Revision tree node" is more correct and the tree is actually the whole collection of nodes.

6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/RevisionTreeItem.php
   @@ -0,0 +1,148 @@
   +    $target_id_data_type = 'string';
   +    if ($target_type->entityClassImplements(FieldableEntityInterface::class)) {
   +      $id_definition = \Drupal::service('entity_field.manager')->getBaseFieldDefinitions($target_type->id())[$target_type->getKey('revision')];
   +      if ($id_definition->getType() === 'integer') {
   +        $target_id_data_type = 'integer';
   +      }
   +    }
   

   Migrate also has \Drupal\migrate\Plugin\migrate\destination\EntityContentBase::getDefinitionFromEntity, I might be a useful utility for entity_field.manager itself.
   
   Edit: looks like this does something else.

7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/RevisionTreeItem.php
   @@ -0,0 +1,148 @@
   +          'type' => 'int',
   ...
   +          'type' => 'int',
   ...
   +          'type' => 'varchar_ascii',
   ...
   +          'type' => 'varchar_ascii',
   

   I wonder if you could use FieldStorageDefinitionInterface::getSchema, on the storage definition of the original ID field.

8. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/RevisionTreeItem.php
   @@ -0,0 +1,148 @@
   +  public function isEmpty() {
   +    // An empty item is always a root.
   +    return FALSE;
   +  }
   

   Hm, so wouldn't isEmpty be TRUE for roots? The comment doesn't explain the hard-coded return value IMO.

@Sam152, thanks!

Re #34:

1. It sure does :)
2. Replied in 5. below
3. I discussed this point with @plach a while ago and he said that having it non-translatable is fine, mostly because we might be able to retire the whole "revision translation affected" stuff and provide a better replacement based on revision parents and conflict management.
4. Yeah, it gets in the way a lot :/
5. Wim raised a similar point in #18, so I guess we really need to do something about it. However, if we introduce the 'tree node' terminology, we might as well name it for what it really is, a graph :) So "Revision graph node" would be even more accurate. I'd like to get some more input on this point from other folks as well, maybe we can come up with something better.
6. That code is gone now, per the point below.
7. We definitely can, I cleaned up all that code which was mostly copied from ER.
8. I've expanded the comment, hope it's more clear now.

Also raising to Major, per #33.

Oops :)

Thanks for addressing each point :)

• #35.3: If there was conflict management on revisions, wouldn't that allow us to essentially have different branches of the graph for each translation, and the revision_parent field would have to be translatable to support that? I understand if this is something that isn't really a concern right now. I suppose translation doesn't affect the schema so it's an easy change later.
• #35.8: Ah, I see. By saying "this field is never empty", NULL becomes a valid value and gets stored for the root node of the graph.

I read the discussion above about the current approach: to be honest I can't say whether the use case @hchonov is describing about deriving entities from other entities should be supported natively. On one hand, given that this is the first time it is proposed AFAIK, despite entity cloning being a thing for quite a while, I'd tend to think that it must not be that common. OTOH it might be that once available to a widespread audience it could end up being a valuable solution.

If I understood correctly @hchonov's examples, it seems the main use case for this approach are entities composed by sub-entities, so I asked @Berdir to chime in here and provide us his view on how one approach or the other might affect Paragraph's ability to support conflict management.

I guess that implementing revision references via an EntityRevisionReferenceItem would support both use cases and thus would be more flexible, OTOH it would likely involve a more complex logic and might be harder to implement and support. In the latter case Conflict might be able to keep things working as they are now by just adding an ERR field, although integrating with core's built-in functionalities might require some workarounds at that point. Maybe the ability to support conflict management between different entities could be separated out to a submodule?

I wish we had had this discussion before the 8.x-2.x branch was opened, however luckily not many sites are using that yet.

Code review of the current patch, solid work indeed :)

1.             +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
               @@ -44,6 +45,13 @@
               +  protected $ancestorIds = [];
           

   I could not spot any invalidation logic for this: that's ok because we never update revision data, right?

2.             +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
               @@ -321,6 +329,9 @@ public function createRevision(RevisionableInterface $entity, $default = TRUE, $
               +    $new_revision->revision_parent->target_id = $entity->getLoadedRevisionId();
           

   What about adding accessor methods for parent / merge parent on RevisionableInterface?

3.             +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
               @@ -398,6 +409,91 @@ public function getLatestTranslationAffectedRevisionId($entity_id, $langcode) {
               +  protected function getAllRevisionParents(RevisionableInterface $entity) {
               ...
               +    $result = $this->getQuery()
               ...
               +    foreach ($this->loadMultipleRevisions(array_keys($result)) as $revision_id => $revision) {
           

   Technically, the base implementation and the SQL-specific (optimized?) version are not equivalent, because in the former we are invoking hooks. What about using an aggregate entity query to retrieve field data without having to load the entity objects in the base implementation?

4.             +++ b/core/lib/Drupal/Core/Entity/RevisionableStorageInterface.php
               @@ -65,4 +65,20 @@ public function deleteRevision($revision_id);
               +  public function getLowestCommonAncestor(RevisionableInterface $entity, $first_revision_id, $second_revision_id);
           

   This is returning a revision ID, should we rename it to ::getLowestCommonAncestorId()? Also, do we actually need an entity object here? Could we pass an entity ID for consistency?

5.             +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/RevisionTreeItem.php
               @@ -0,0 +1,122 @@
               + * Defines the 'revision_tree' field type.
           

   My initial reaction based on #38.5 was to propose to name this revision_parent, since it implements a very specific logic that fits only this particular use case, I believe. However this led me to think that @yched always used to say that field items should only be data buckets and NOT wrap any logic, so maybe we should implement a RevisionReferenceItem instead? We could then add revision_parent and revision_merge_parent fields to revisionable entity types and move the logic to ContentEntityBase::preSaveRevision()?

6.             +++ b/core/modules/system/system.install
               @@ -2173,3 +2174,146 @@ function system_update_8501() {
               +    $parent_revision_field_storage_definition = $entity_definition_update_manager->getFieldStorageDefinition($entity_type->getRevisionMetadataKey('revision_parent', FALSE), $entity_type_id);
           

   Extra param passed to getRevisionMetadataKey.

7.             +++ b/core/modules/system/system.install
               @@ -2173,3 +2174,146 @@ function system_update_8501() {
               +    $id_column = $table_mapping->getFieldColumnName($id_field_storage_definition, 'value');
               +    $revision_id_column = $table_mapping->getFieldColumnName($revision_id_field_storage_definition, 'value');
               +    $revision_parent_column = $table_mapping->getFieldColumnName($parent_revision_field_storage_definition, 'target_id');
           

   Were the property names hardcoded on purpose?

8.             +++ b/core/modules/system/system.install
               @@ -2173,3 +2174,146 @@ function system_update_8501() {
               +      ->range($sandbox[$entity_type_id]['current'], $sandbox[$entity_type_id]['current'] + $steps)
           

   To avoid issues with added/removed data, could we adopt an approach similar to SqlFieldableEntityTypeListenerTrait:227 here?
   Or hardcoding assumptions about revision IDs being integers would not be ok?

9.             +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityRevisionTreeTest.php
               @@ -0,0 +1,124 @@
               +   * @var \Drupal\Core\Entity\EntityStorageInterface|\Drupal\Core\Entity\RevisionableStorageInterface
           

   To be merged :)

10.             +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityRevisionTreeTest.php
                @@ -0,0 +1,124 @@
                +    // Create a complex revision graph:
                +    //
                +    //        3 -- 4 -- 5 -- 8
                +    //       /         /      \
                +    // 1 -- 2 -- 6 -- 7 ------ 9 --- 16
                +    //            \                 /
                +    //             10 -- 11 ------ 15
                +    //              \             /
                +    //               12 -- 13 -- 14
                +    //
            

    <3

11.             +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityRevisionTreeTest.php
                @@ -0,0 +1,124 @@
                +      5 => [4, 7],
            

    This looks weird: how can a parent ID be higher than the child ID? Shouldn't that be 8 => [5, 7]?

Edit: fixed some formatting issues.

Thanks for the review, @plach!

I've managed to rework the patch and make the current way of storing two revision parents in a single field item an implementation detail of the RevisionTree field type. Through the new EntityRevisionReferenceFieldItemListInterface added in this patch, the revision_parent field can now be swapped with an entirely different implementation like EntityReferenceRevision from contrib, assuming that it will implement this new interface.

Also extracted the code and logic for getting the lowest common ancestor into a service, so it's easier to swap with a custom implementation rather than replacing the entire entity storage class.

Re #40:

1. Yup, but this static cache is gone now because I think it was a premature optimization.
2. Done! Except that there's no distinction between the parent ID and the merge parent ID, they're both just parent IDs :)
3. Fixed because the new implementation only works with the SQL entity storage class.
4. Sure, renamed as suggested.
5. We discussed this a lot and I said I'll try to check if we can move the logic to ContentEntityBase::preSaveRevision(), but I don't think it's possible because we need the ability to set a default value for the parent ID only when it wasn't set manually, and we can't influence the onChange() method of the field type class from ContentEntityBase::preSaveRevision(). In any case, setting a default value is not what I would call logic, and ChangedItem does very similar things.
6. Fixed.
7. Nope, my bad :) Fixed.
8. We've tried very hard so far to not force revision IDs to be integers.. there's even an issue to codify the int|string expectation in the doxygen blocks of various methods that deal with revision IDs.
9. Done!
10. :D
11. You're right, since those are supposed to be revision IDs, the ancestry has to be sequential. Fixed :)

Those test failures are just a copy/pasta mistake in the update path function.

Aside from that, I changed my mind about introducing a new EntityRevisionReferenceFieldItemListInterface. ERR in contrib would have to depend on core 8.7 to be able to use it, and that's not very realistic in the short term. So we should simply use the existing EntityReferenceFieldItemListInterface, which is also used by ERR right now :)

That test fail will be fixed by #2975081: UpdatePathTestBase fails to re-initialize the test site (rebuild container, clear caches) after running the database updates. In the meantime, the patch is still good to review.

Nice work!

Just a side note:

Aside from that, I changed my mind about introducing a new EntityRevisionReferenceFieldItemListInterface. ERR in contrib would have to depend on core 8.7 to be able to use it, and that's not very realistic in the short term.

True, however the 9.0-dev branch will be opening soon, and if D9 is expected to have ERR in core at some point, we should get prepared as early as possible, particularly help ERR integrate with Core as tightly as possible.

Rerolled the patch to chase latest HEAD.

@Pancho, the interface that I mentioned was entirely new code added in #41 that doesn't exist in the contrib ERR module, so this patch is not making it any easier or harder to get ERR in core at some point :)

so this patch is not making it any easier or harder to get ERR in core at some point :)

I know. But if we could make it easier, we probably should.

Re #40:

I can't say whether the use case @hchonov is describing about deriving entities from other entities should be supported natively. On one hand, given that this is the first time it is proposed AFAIK, despite entity cloning being a thing for quite a while, I'd tend to think that it must not be that common. OTOH it might be that once available to a widespread audience it could end up being a valuable solution.

If I understood correctly @hchonov's examples, it seems the main use case for this approach are entities composed by sub-entities, so I asked @Berdir to chime in here and provide us his view on how one approach or the other might affect Paragraph's ability to support conflict management.

Actually not. What we do is to export our original content and then import it to another site. There we allow it to be modified - no matter if there are sub-entities involved or not. When a user wants to modify the content, then we create a duplicate of the entity and through a custom parent reference we make a link between both entities. Then we make it possible to merge changes between both entities, while they diverge. I do think that distributing content is a pretty common case. And allowing Drupal out of the box for tracking this would be super nice feature.

As you've said :

OTOH it might be that once available to a widespread audience it could end up being a valuable solution.

How can we make a decision on this?

I guess that implementing revision references via an EntityRevisionReferenceItem would support both use cases and thus would be more flexible, OTOH it would likely involve a more complex logic and might be harder to implement and support. In the latter case Conflict might be able to keep things working as they are now by just adding an ERR field, although integrating with core's built-in functionalities might require some workarounds at that point. Maybe the ability to support conflict management between different entities could be separated out to a submodule?

The request of the community for supporting reference by revision is there for quite some time as well and there is an issue for this - #2812595: Support revision tracking of entity references in core. I think that this will be amazing for the community, especially given the fact that we are doing more and more stuff with revisions. So many new features arise regarding revisions, so I think that it is time that we finally make it a core feature to reference by revision. If we are going to invest time and resources, then let us solve the problem in a much general form, where a bigger part of the community will benefit from. The effort will be higher, but the win will be even higher.

I wish we had had this discussion before the 8.x-2.x branch was opened, however luckily not many sites are using that yet.

What do you mean when you say "luckily"? I am completely open to talks about the further development of the branch and we still can adjust things to fit better for core. I've been developing the module with high quality and documentation as I can in order for it to become a proud core member some day :). I would be happy to cooperate with you on it, so that we make it better and suitable for a wide range of use cases before it enters core.
Actually it would have been great, if you would've had involved me in your conversations and discussions about the approach in this issue here, because I've already have some pretty good experiance in dealing with conflicts and providing conflict resolutions, because I've invested an enourmous amount of time and work on making conflict 2.x working out-of-the-box for all kind of content entities and I don't even require an entity to be revisionable in order to provide concurrent editing and conflict resolutions.

----------------------

Independently from the disccusion about the parent field and how we want to implement it, there is something I don't really understand about the merged tracking and why exactly it is considered when dealing with revisions. Could you please provide some more in-depth reasoning about this?

Consider the following graph:
     10 (A)
    /       \
 20 (A)    30 (B)
   \         /    \
    21 (A)      31 (B)
    /
   22 (A)

Here we have the revision 10 from the revision branch 1.x. Revision 10 has the value "A" on some random field. From revision 10 we create two revision branches - 2.x and 3.x - with the respective revision IDs 20 and 30.

On the revision branch 2.x the field value remains intact. On the revision branch 3.x the value is changed to "B".

Then we merge revision 30 into revision 20 and save the result as a revision 21 in the 2.x branch - still without changing the value of the field.

From 21 create 22 in the revision 2.x branch.
From 30 create 31 in the revision 3.x branch.

The next step is to merge 31 from revision branch 3.x into 22 from revision branch 2.x.

Now according to the latest patch the common ancestor for this merge will be 30 from revision branch 3.x. However this is not really true, because only some part of 30 has been merged into 21, but 30 doesn't represent all the common values they both shared, which are only truly and completely represented by 10.

When you perform a 3-way merge, then yes you need a common ancestor, however you need the one from which the two branches separated apart. One should ask the question why is it actually needed - and the answer is to find out what really has changed since the entities drove apart.

Here I would argue that it is completely enough to define 10 as the common ancestor. Why? Because if we merge an entity from the 3.x branch into the 2.x branch without taking the changed value from the 3.x branch and then later we want to merge the 3.x branch into the 2.x branch again and if this value has not yet changed since the last merge, then we have to detect this. We have have to detect, that the value has not changed in 2.x but only in 3.x. If we use 10 as common ancestor then we can detect this. If we use 30 as a common ancestor we cannot detect it.

In this case it is even wrong to define 30 as a common ancestor. Why? Because then we would consider the value of the 2.x branch as the one that has changed compared to the common ancestor of the 3.x branch. This however is not true - the value has not changed since we've initialy created the 2.x branch. It is wrong to assume that the value has changed on the 2.x branch. Why is this is wrong? Because if you take as an example concurrent editing then you need to know if the value has been changed in the current session (revision branch 2.x) or on the server (revision branch 3.x). If you define 30 as the common ancestor, then you will assume that the value has been changed by the current user - which however is not true. Assuming this might lead to overwriting and reverting values if using automatic merges. Even if you provide a conflict resolution UI to the user then you would show the local value as changed compared to the common ancestor - and the user will be confused.

Also if the user has decided to merge values of the 3.x branch into the 2.x branch, then those will not be identified as conflicts at all and therefore there is actually no need for having access to the "merged" revision.

A good example is:
1. The user of the 2.x branch tries to save.
2. Conflicts with the 3.x branch are detected.
3. A conflict resolution UI is provided to the user.
4. The user accepts some of the changes on the 3.x branch, but some not - the "A" values still remains intact.
5. After the conflict resolution is finished, then the user continues editing the form, as after the conflict resolution some adjustments might be needed.
6. The user tries to save again and a new conflict is detected, because there was an update on the 3.x branch in the meanwhile.
7. Now we should not detect the value "A" as changed by the user of the 2.x branch, but we should be able to identify it as only changed in the 3.x branch.

Based on this I think that a core solution for finding the common ancestor should be implemented as easy and transparent as possible and be defined as the entity at which point the revision branches were split. I think that core should stick to а general solution and if there is a need for something so specific like the merged revision, then this can be implemented in contrib and we could offer the API for exchanging the common ancestor. This general approach is also much more understandable and completely enough.

P.S. I'll be visiting Drupal Mountain Camp Davos 07.-10.03.2019 and there I will be working on #2812595: Support revision tracking of entity references in core.

@amateescu, @plach it would be great if we could meet there and talk further before this issue reaches 200+ comments, at it is already on that way :). Otherwise we could schedule a web conversation.

@hchonov, I'm not going to quote all that wall of text, but I'll answer on each main "topic":

Why not use the ERR field type for tracking revision parents: because we need to track multiple parents, and using ERR for that means we would have to add a dedicated field table for each revisionable entity type. Since we know that we can only have a maximum of two parent revisions, the field type introduced by this patch provides an optimized storage by keeping them in two properties of the same field item so they can be stored in the shared entity revision table.

Why do we need to track multiple parents: that's the definition of a merge revision. A merge revision has all of the parents which are the last revision of each involved branch. Specifically, the parent on the current branch is the first parent. The other ("merge") parent is the last revision of the merged branch.

Regarding your example graph: that's a problem that can happen in git as well, and it's really well described in this answer on SO. Git doesn't prevent you from working further in the 3.x branch after it was merged into 2.x (i.e. creating revision 31 with 30 as the parent), but, in our case, we could decide that we don't want to allow users to get into that situation and forbid parents that are part of a "closed branch".

In an autosave scenario, where the user continues editing revision 30 without knowing that the 3.x branch has been merged into 2.x, we can write some code in preSaveRevision() which detects that the parent revision (which was assigned automatically when the editor started working on it) is now part of a closed branch and re-assign a proper parent, 21 in this case. This allows the 2.x and 3.x branches to live separately as they were before the merge, and a subsequent merge would have to resolve the (same) conflict, there's really no other way around that.

As for the proposal to "define the common ancestor as the entity at which point the revision branches were split", please take a look at the example revision graph from the issue summary of #3023194: [PP-2] Add parallel revisioning support and clarify how would your solution work exactly.

In any case, I've done a few updates to the patch:

• added a getter and a setter for the parent revision IDs on RevisionableInterface, and a default implementation in ContentEntityBase which works regardless of the field type used to store the parents;
• moved the validation to be on the entity level instead of the field type level, which makes it work with any kind of revision reference field as well;
• removed the constraint for the parent revisions that needed to belong to the same entity, so you can now reference two arbitrary revisions of an entity type, which satisfies your use case for cross-entity parent tracking;
• the logic for determining the LCA of two revisions now lives in a entity.revision_tree service and it doesn't require an $entity_id argument anymore, which makes it easy to swap with a custom implementation (this was done in #41).

All this means that it's really easy to swap the optimized field type provided by core with an ERR field if needed, and also that your specific use case is completely supported by the current approach.

That fail will be fixed by #2975081: UpdatePathTestBase fails to re-initialize the test site (rebuild container, clear caches) after running the database updates, keeping at NR to get more eyes on this until that issue gets in.

While working on #2950869: Entity queries querying the latest revision very slow with lots of revisions I realized that we must set the (first) parent automatically to always be the active revision of the branch. Here's an update for that which also fixes @plach's point from #40.5.

Also posting a patch combined with #2975081: UpdatePathTestBase fails to re-initialize the test site (rebuild container, clear caches) after running the database updates to check if we're good on all supported database drivers.

Those test fails reminded me that we should keep the ability to set the revision parents manually if needed, so we need to do the same "enforce" dance as for the revision translation affected key. Also restored the test coverage for it.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -322,6 +329,58 @@ public function updateLoadedRevisionId() {
   +    }
   +
   +    $revision_parent_key = $entity_type->getRevisionMetadataKey('revision_parent');
   +    $revision_parent_field = $this->get($revision_parent_key);
   +
   

   I'm just jumping in without having read much of this issue except the issue summary.

   Thinking about paragraphs, I'm wondering if there should be a way for entity types to opt out of this. We already don't track our own changed date, uid as well as most of the revision metadata because each paragraph revision is part of the larger composite which is the parent and its referenced paragraphs.

   When we merge two host revisions with paragraphs, we don't really care about the parent of the paragraphs revisions but the paragraph revisions referenced by the host entity.

2. +++ b/core/lib/Drupal/Core/Entity/EntityRevisionTree.php
   @@ -0,0 +1,122 @@
   +   */
   +  protected function getAllRevisionParents($entity_type_id, $entity_id) {
   +    $entity_type = $this->entityTypeManager->getDefinition($entity_type_id);
   +    $storage = $this->entityTypeManager->getStorage($entity_type_id);
   +    assert($storage instanceof SqlEntityStorageInterface);
   

   This has probably been discussed, but I'm wondering what the reason for this being a separate service is. A site could theoretically have one entity type in the SQL and another in MongoDB. Or it could have a different implementation of an SQL backend that doesn't work with the same type of structure?

   This being a dedicated service, even if we'd tag it with backend_overridable, would not allow to provide different implementations for different entity types?

3. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidRevisionParentConstraintValidator.php
   @@ -0,0 +1,87 @@
   +  public function validate($entity, Constraint $constraint) {
   +    /** @var \Drupal\Core\Entity\RevisionableInterface $entity */
   +    if (!isset($entity)) {
   +      return;
   +    }
   

   what about doing this as an actual $entity instanceof RevisionableInterface check, then you get code inspection too without a @var.

   Also, if we'd make having a parent field optional then this validator would also need to be optional, as well as various places that define the field definition and so on.

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/RevisionTreeItem.php
   @@ -0,0 +1,89 @@
   +   */
   +  public static function schema(FieldStorageDefinitionInterface $field_definition) {
   +    $target_type = \Drupal::entityTypeManager()->getDefinition($field_definition->getTargetEntityTypeId());
   +
   +    /** @var \Drupal\Core\Field\BaseFieldDefinition $revision_field_definition */
   +    $revision_field_definition = \Drupal::service('entity_field.manager')->getBaseFieldDefinitions($target_type->id())[$target_type->getKey('revision')];
   +    $revision_column_schema = $revision_field_definition->getSchema()['columns'][$revision_field_definition->getMainPropertyName()];
   

   a field schema that is dynamically depending on the schema of another field on the same entity type is slightly scary :)

5. +++ b/core/lib/Drupal/Core/Field/RevisionTreeItemList.php
   @@ -0,0 +1,40 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function set($offset, $value) {
   +    parent::set($offset, $value);
   +
   +    // Store the second revision parent in its dedicated property.
   +    if ($offset === 1) {
   +      $this->list[0]->set('merge_target_id', $value, TRUE);
   +    }
   +
   +    return $this;
   

   Is there a specific reason (I actually wrote revision first, too much revision going on here) for this magic?

   It does seem to assume that $value is a scalar, I'm also not quite sure why it calls the parent *and* this?

6. +++ b/core/modules/system/system.install
   @@ -2238,3 +2239,146 @@ function system_update_8601() {
   +/**
   + * Fill values for the 'revision_parent' field.
   + */
   +function system_update_8702(&$sandbox = NULL) {
   +  $entity_definition_update_manager = \Drupal::entityDefinitionUpdateManager();
   +  $entity_type_manager = \Drupal::entityTypeManager();
   

   A bit worried about the execution time of these update functions, not just this but others that 8.7 is adding. Didn't review in detail yet, but if I see this correctly we're doing a select + ûpdate for each revision, plus a select to get the next batch of revisions.

   We are using the default entity_update_batch_size step size here, but there's a big difference between an entity save and a select + update, so possibly this could be considerably higher and a bit more efficient then. Not sure if it's worth it, it will be called for at least one second per run.

   Would be great if we could test this on a database with a million nodes or so. I did a somewhat similar update on paragraphs and tested that with 800k paragraphs. It was surprisingly fast but did take a while. I know that my laptop is also 3x faster than e.g. a MySQL Galera cluster due far less disk/network overhead and my MySQL being configured for speed and not secure data.

   I'm wondering if a Drupal site could technically work without all those revision parents being filled out and if we could do that in the background on cron/batch. But I suppose that's tricky as soon as we start to actually save a revisionable entity? And also other upcoming things depending on this.

   I'll try to test this on a larger site but I imagine it is a bit tricky as I will need to first update our distribution to 8.7 and then update the real site with that. And this patch likely doesn't apply to 8.6.

Wasn't that hard to get that working, but the update died :)

Executing system_update_8701 [15.36 sec, 35.56 MB]                                                                                                                                                                                                                               
Field storage definition for 'langcode' could not be found. [40.14 sec, 75.16 MB]                                                                                                                                                                                             

Seems to die on https://www.drupal.org/project/profile, which is revisionable but not translatable. Possibly caused by something else in 8.7, didn't debug further, but here's the exception backtrace:

Drupal\Core\Field\FieldException: Field storage definition for 'langcode' could not be found. in web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:870
Stack trace:
#0 web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php(159): Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->getEntitySchema(Object(Drupal\Core\Entity\ContentEntityType), true)
#1 web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php(344): Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->requiresEntityStorageSchemaChanges(Object(Drupal\Core\Entity\ContentEntityType), Object(Drupal\Core\Entity\ContentEntityType))
#2 web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(1466): Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->onEntityTypeUpdate(Object(Drupal\Core\Entity\ContentEntityType), Object(Drupal\Core\Entity\ContentEntityType))
#3 web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(1546): Drupal\Core\Entity\Sql\SqlContentEntityStorage->Drupal\Core\Entity\Sql\{closure}()
#4 web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(1467): Drupal\Core\Entity\Sql\SqlContentEntityStorage->wrapSchemaException(Object(Closure))
#5 web/core/lib/Drupal/Core/Entity/EntityTypeListener.php(92): Drupal\Core\Entity\Sql\SqlContentEntityStorage->onEntityTypeUpdate(Object(Drupal\Core\Entity\ContentEntityType), Object(Drupal\Core\Entity\ContentEntityType))
#6 web/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php(233): Drupal\Core\Entity\EntityTypeListener->onEntityTypeUpdate(Object(Drupal\Core\Entity\ContentEntityType), Object(Drupal\Core\Entity\ContentEntityType))
#7 web/core/modules/system/system.install(2280): Drupal\Core\Entity\EntityDefinitionUpdateManager->updateEntityType(Object(Drupal\Core\Entity\ContentEntityType))
#8 vendor/drush/drush/commands/core/drupal/update.inc(65): system_update_8701(Array)
#9 vendor/drush/drush/commands/core/drupal/batch.inc(163): drush_update_do_one('system', 8701, Array, Object(DrushBatchContext))
#10 vendor/drush/drush/commands/core/drupal/batch.inc(111): _drush_batch_worker()
#11 vendor/drush/drush/includes/batch.inc(98): _drush_batch_command('151137')
#12 vendor/drush/drush/commands/core/drupal/update.inc(235): drush_batch_command('151137')
#13 vendor/drush/drush/commands/core/core.drush.inc(1228): _update_batch_command('151137')
#14 vendor/drush/drush/includes/command.inc(422): drush_core_updatedb_batch_process('151137')
#15 vendor/drush/drush/includes/command.inc(231): _drush_invoke_hooks(Array, Array)
#16 vendor/drush/drush/includes/command.inc(199): drush_command('151137')
#17 vendor/drush/drush/lib/Drush/Boot/BaseBoot.php(67): drush_dispatch(Array)
#18 vendor/drush/drush/includes/preflight.inc(66): Drush\Boot\BaseBoot->bootstrap_and_dispatch()

Should be fairly easy to reproduce by installing profile.module, possibly create some data and then run the update...

Ok, just skipped profile. With 800k node revisions and a bit over 800k paragraph revisions, the update took about 780s, then went into some kind of loop and didn't finish. Seen that before when the sum query disagree over the amount of rows that are actually returned, could be some kind of cruft in the table or so. Usually I also add an exit condition if the last run returned less than 50 results.

And I'm sure this isn't the largest site by far :)

One thing I see that causes it to slow down is that LIMIT HUGE_NUMBER, 50 is inefficient:

SELECT t.revision_id AS revision_id FROM paragraphs_item_revision t WHERE (revision_id < '2822025') limit 797020, 50;
50 rows in set (0.15 sec)

It's just 0.15s at the end of the process, but still, that sums up quickly, 800k rows means we have to call the update function 16k times. It's much faster if you invert it and select the next 50 revision ids that are higher:

SELECT t.revision_id AS revision_id FROM paragraphs_item_revision t WHERE (revision_id > '2822025') limit 50;
50 rows in set (0.00 sec)

For this query, that would mean that you need to store the last updated revision_id in the sandbox, you don't even need the MAX because per above, you can simply stop if you get less than $batch_size results.

Re #51:

@hchonov, I'm not going to quote all that wall of text, but I'll answer on each main "topic":

If I did not wanted/expected an extended answer then I would not have put that much effort into writing it all down. This is why I've asked for a call where we can clarify all the things and prevent misunderstandings. Once again I propose that we schedule a call. What do you think?

Why not use the ERR field type for tracking revision parents: because we need to track multiple parents, and using ERR for that means we would have to add a dedicated field table for each revisionable entity type. Since we know that we can only have a maximum of two parent revisions, the field type introduced by this patch provides an optimized storage by keeping them in two properties of the same field item so they can be stored in the shared entity revision table.

Once again you introduce a specific field and invest work into a specific solution instead into a general one where the whole Drupal community will benefit. Why? Why does this feature have higher priority than the community feature request for referencing revisions out of the box? Why don't we first implement a general solution and then if really needed re-use it here or adjust it for a specific use case here?

What is the problem of having a dedicated table? When #2620980: ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load() gets in, then we don't have to worry about any optimizations regarding entity revisions, as nothing can outperform a revision cache, where we should not constantly join all the tables just for loading an entity revision. Therefore I don't think that this argument really counts. And if it does not count as an argument, then we can separate the parent and the merged revision problems and have two dedicated issues instead of this big one. We can then focus on two different problems. What about this?

Regarding your example graph: that's a problem that can happen in git as well, and it's really well described in this answer on SO. Git doesn't prevent you from working further in the 3.x branch after it was merged into 2.x (i.e. creating revision 31 with 30 as the parent), but, in our case, we could decide that we don't want to allow users to get into that situation and forbid parents that are part of a "closed branch".
In an autosave scenario, where the user continues editing revision 30 without knowing that the 3.x branch has been merged into 2.x, we can write some code in preSaveRevision() which detects that the parent revision (which was assigned automatically when the editor started working on it) is now part of a closed branch and re-assign a proper parent, 21 in this case. This allows the 2.x and 3.x branches to live separately as they were before the merge, and a subsequent merge would have to resolve the (same) conflict, there's really no other way around that.

This is not possible. This kills concurrent editing. If 2.x is the current user session and 3.x the default revision branch then you cannot close either of them.

1. The user of the 2.x branch works and wants to save at some point in time, but the default revision branch was updated and therefore there is a conflict now.
2. Then the user gets a conflict resolution UI, where the user can merge all the changes or just pick a few of them from the default revision branch 3.x.
3. After the conflict resolution the user might need some more time to adjust things because of the merged 3.x branch.
4. In the meantime it should be possible for other users to save in the default revision.
5. After some time the user of the 2.x branch decides to save, but now there might be a new conflict.
6. A new conflict resolution should occur.

We cannot disable such use cases just to "announce" the merged revision as the common ancestor of two entities instead of the one entity where both branches have split.

As for the proposal to "define the common ancestor as the entity at which point the revision branches were split", please take a look at the example revision graph from the issue summary of #3023194: [PP-2] Add parallel revisioning support and clarify how would your solution work exactly.

I have asked for an in-depth reasoning about the merge revision tracking and using it as a common ancestor. Could you please provide this information, as I cannot find it, but it would be really useful here. Without it it is still hard for me to understand why/what for exactly do you need the merged revision beside "history" tracking. Also regarding the referenced issue please state me a specific question and what exactly would not work if we use the real parent of both revision branches instead of the merged revision?

I've also made a statement that it is wrong to assume the merged revision as a common ancestor, because the entity revision where both branches have split is the real and truly ancestor of both revision branches and not the merged one. Could you please comment on that?
Please consider that if you merge 3.x into 2.x and don't modify the merged fields and then later want to merge 3.x into 2.x again then there will be no conflicts when using the entity from the 1.x branch as the common ancestor because the values didn't change.

P.S.
Please note that we already have a "parent" field in core, which is allowed to reference a different entity. I am talking about the taxonomy term's parent field. The taxonomy term entity type should be converted to revisionable in #2880149: Convert taxonomy terms to be revisionable and I've just posted there the question what is going to happen with the parent field. Because there it might make sense to track changes in the vocabulary and for that we would need an entity revision reference field to reference the target revision of the parent in order to be able to fully reconstruct a vocabulary at any certain time point.

This and the use case of decoupling (creating duplicate) entities and tracking changes between the original and the decoupled entity I've given earlier in the current issue just show how useful an entity revision reference field will be. We are even already doing so much things in core with revisions and still don't offer the ability to reference by revision.

@Berdir, thanks for the review and the thorough testing! That was very very useful :)

Re #57:

1. We discussed this further and agreed it's not really useful to have the ability to disable this functionality, mostly because a few other followups intend to use the revision tree introduced here for various purposes:
   • fix the critical performance problem from #2950869: Entity queries querying the latest revision very slow with lots of revisions
   • use the logic for finding the LCA of two revisions in #2867707: WI: Phase H: Conflict management and local workspace merging support
   • possibly re-write the "latest affected" functionality based on revision (translation) parents
2. Very good point. Converted it to be an entity handler instead.
3. Done :)
4. And yet it works surprisingly well, even for the ER field type: #2680571-41: Cannot reference an entity with a big int (size setting of ID field not propagated to reference field) (don't believe those test fails, they're just about "unexpected" schema changes)
5. The reason for that whole item list class was my attempt to make it very easy to swap this field type with an ERR one, but it's not really working and it increases the complexity for no good reason, so I dropped that approach.
6. Implemented your suggestion from #59, which also makes the logic of this update function very similar to the one used by SqlFieldableEntityTypeListenerTrait::copyData().

I still have to look into #58.

Edit: I installed the Profile module on my local 8.7.x codebase without this patch, created a profile and then applied the patch and ran the update function and everything worked fine. I know that for you it failed before this step, but the profile that I created had two revisions and the second one had the proper parent assigned by system_update_8702().

@hchonov:

Once again you introduce a specific field and invest work into a specific solution instead into a general one where the whole Drupal community will benefit. Why? Why does this feature have higher priority than the community feature request for referencing revisions out of the box? Why don't we first implement a general solution and then if really needed re-use it here or adjust it for a specific use case here?

Because I don't think this feature is a good use case for ERR, for the reasons explained in the part of my comment that you quoted just above these questions.

What is the problem of having a dedicated table? When #2620980: ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load() gets in, then we don't have to worry about any optimizations regarding entity revisions, as nothing can outperform a revision cache, where we should not constantly join all the tables just for loading an entity revision. Therefore I don't think that this argument really counts.

The optimizations provided by this field type are about storage: less tables, less data that needs to be stored, faster inserts, etc., it has nothing to do with loading revisions. Therefore I think the argument counts.

This is not possible. This kills concurrent editing. If 2.x is the current user session and 3.x the default revision branch then you cannot close either of them.
...
We cannot disable such use cases just to "announce" the merged revision as the common ancestor of two entities instead of the one entity where both branches have split.

I have to say that my initial reaction to "killing concurrent editing" was that it's just FUD, but I took the time to re-read everything a few times over and realized that this

3. After the conflict resolution the user might need some more time to adjust things because of the merged 3.x branch.

actually means that the user aborted the merge while keeping the local changes they made, including possible resolved conflicts. Which, in turn, means that revision 21 from your initial graph is not a merge revision after all, it only has 20 as a parent, so a subsequent merge attempt will still pick 10 as the common ancestor.

I have asked for an in-depth reasoning about the merge revision tracking and using it as a common ancestor. Could you please provide this information, as I cannot find it, but it would be really useful here. Without it it is still hard for me to understand why/what for exactly do you need the merged revision beside "history" tracking.

That's precisely the answer, the merged revision is used for history tracking. As I've said before, all this work is based on the git storage model, which tracks two parents for a commit that results from a non-fast-forward merge (the -no-ff option).

I've also made a statement that it is wrong to assume the merged revision as a common ancestor, because the entity revision where both branches have split is the real and truly ancestor of both revision branches and not the merged one. Could you please comment on that?

Per the explanation above, I don't think there's any disagreement here. A merge revision has no special meaning in the graph, it will only be a common ancestor when two branches are split from it, which is not the case in the example you gave earlier.

Also, I forgot to mention that the ability to specify a parent revision that belongs to a different entity was already implemented a while ago (in #41) by removing the validation constraint for this case, and by allowing $entity_id to be NULL in EntityRevisionTreeHandlerInterface::getLowestCommonAncestorId($revision_1, $revision_2, $entity_id = NULL). So your use case for cross-entity parent tracking is fully supported by this patch.

+++ b/core/modules/system/system.install
@@ -2371,10 +2373,18 @@ function system_update_8702(&$sandbox = NULL) {
 
-    $sandbox[$entity_type_id]['current'] += count($revisions);
-    $sandbox[$entity_type_id]['finished'] = ($sandbox[$entity_type_id]['current'] == $sandbox[$entity_type_id]['max']) || empty($revisions);
+    $missing = $database->select($revision_table, 't')
+      ->condition("t.$revision_id_column", $sandbox[$entity_type_id]['current_id'], '>')
+      ->orderBy($revision_id_column, 'ASC')
+      ->countQuery()
+      ->execute()
+      ->fetchField();
+    $sandbox[$entity_type_id]['finished'] = $missing ? $sandbox[$entity_type_id]['progress'] / ($sandbox[$entity_type_id]['progress'] + (int) $missing) : 1;

hm, when saying that we don't need the total, I forgot about progress tracking. It might be more efficient to still get the max revision id and use that to calculate a rough progress and then we can also use count($revisions) > $step_size as an indicator that we are done. That wouldn't be exact, but the progress is pretty random anyway here with multiple entity types (I'm not even sure how you currently calculate that into the progress).

About ERR, my thoughts as an entity system co-maintainer are that it does not seem like a good fit for this scenario. Especially with the current approach of storing possibly two revision parents, that would be very inefficient with ERR because you would need a multi-cardinality field, which means a dedicated field table for every revisionable entity type. ERR stores a reference to an entity id + entity revision, so even if we would only store one parent, we would have an extra column with useless data. That is only for this type of revision parent reference, the term parent field is quite a different use case, exactly because it is a reference to a different entity. I think there's an open issue already to make that revisionable.

And my thoughts about getting ERR into core as an ERR co-maintainer are that I'm currently not very fond of that idea, for a few reasons:

* Getting non-trivial stable functionality (this wouldn't be a module) into core is a very time consuming process, I think I can speak for Miro too when I say that neither of us could spend the necessary time & effort on that. And I think it would be important to have us on board, see following reasons.

* Only a relatively small part of the ERR module would be viable for core IMHO, a large part of the complexity is in the composite system that is heavily used by Paragraphs. But there are still too many moving parts there and it would not make sense to have this in core IMHO. That means the module would still need to provide that functionality, which would require a considerable amount of time for refactoring it. And further improvements would also be hindered by having parts of it in core, most of which we'd likely have to override again.

* ERR currently does not have a widget included that actually makes sense for it, the default ER widgets don't really work because there is nothing that allows to select a specific revision and the default widgets also update to the latest revision every time you save. There are also other incomplete parts, like views integration.

I can't really comment right now the other discussion points like merging strategies and if we need to track multiple parents, would need to spend more time thinking and reading through plans for that.

hm, when saying that we don't need the total, I forgot about progress tracking. It might be more efficient to still get the max revision id and use that to calculate a rough progress and then we can also use count($revisions) > $step_size as an indicator that we are done.

The problem with getting the max revision ID before applying the changes to the revisions processed by the batch run is that it doesn't know about revisions that might have been created in the meantime, that's why @plach and I opted for getting the "final" answer at the end of the batch run for both SqlFieldableEntityTypeListenerTrait::copyData() and the update function from this patch.

You raised this question in #57.6, and with a small addition of checking whether the parent ID is NULL, I think the update function will work fine in the scenario where revisions are created while the update runs.

That wouldn't be exact, but the progress is pretty random anyway here with multiple entity types (I'm not even sure how you currently calculate that into the progress).

We don't calculate individual progress for each revision, only for entity types :) That's what system_update_8400() did as well, when moving revision metadata fields to the revision table.

#2975081: UpdatePathTestBase fails to re-initialize the test site (rebuild container, clear caches) after running the database updates is in, so this is now unblocked! Re-uploading the 'for review' patch.

Just to say I'm +1 to a custom field type with two values for the different revision parents here.

ERR has a very specific use case (like linking commerce orders to the product revision so that the price reflects what was paid at the time, not later changes) so keeping ERR in contrib, and this field being very clearly for the special case we're dealing with will make that clear.

This looks very good! Personally I'd still prefer a slightly different approach to track parents: a new simple revision reference type (not an ERR!) and two base fields to track parent and merge parent, since parent and merge parent have very defined roles and we don’t foresee to have more parents in this specific context. Having this generic reference field would allow us to implement generic DAGs supporting multiple parents, and any component working with graphs could be adapted to work with both generic DAGs and this specific implementation in the same way. However, given that most people here seem to be onboard with the current solution, I'm not going to argue about it :)

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -322,6 +329,58 @@ public function updateLoadedRevisionId() {
   +  public function getParentRevisionIds() {
   

   What about adding dedicated accessor methods for the two parents? This would make the semantics of the consumer code more explicit/readable.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -702,6 +702,18 @@ protected function doPreSave(EntityInterface $entity) {
   +        $parent_revision_ids[0] = $this->getLatestRevisionId($entity->id());
   +        $entity->setParentRevisionIds($parent_revision_ids);
   

   I'm wondering whether we could perform this assignment in ContentEntityBase::preSave() and whether that would allow us to avoid the need for the enforced methods.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
   @@ -37,6 +37,12 @@ public function __construct($definition) {
   +        'revision_tree' => 'Drupal\Core\Entity\Sql\SqlRevisionTreeHandler',
   

   Why "tree" and not "graph"?
   (here and everywhere else :)

4. +++ b/core/lib/Drupal/Core/Entity/EntityRevisionTreeHandlerInterface.php
   @@ -0,0 +1,29 @@
   +   *   The lowest common ancestor of the two revisions, or NULL if it couldn't
   

   Can we mention that we are returning a revision ID?

5. +++ b/core/lib/Drupal/Core/Entity/EntityRevisionTreeHandlerInterface.php
   @@ -0,0 +1,29 @@
   +  public function getLowestCommonAncestorId($revision_1, $revision_2, $entity_id = NULL);
   

   Can we add _id to the first two parameters to match the third one?

6. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidRevisionParentConstraintValidator.php
   @@ -0,0 +1,85 @@
   +    assert($entity instanceof RevisionableInterface);
   

   Would it make sense to return early and skip validation if the parent IDs are not enforced?

7. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidRevisionParentConstraintValidator.php
   @@ -0,0 +1,85 @@
   +      if (count($parent_revision_ids) === 2 && $parent_revision_ids[0] == $parent_revision_ids[1]) {
   

   Could we ever have a revision with ID 0? If so we'd have problems here when the merge parent is NULL.

8. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlRevisionTreeHandler.php
   @@ -0,0 +1,175 @@
   +  public function __construct(EntityTypeInterface $entity_type, EntityTypeManagerInterface $entity_type_manager, EntityLastInstalledSchemaRepositoryInterface $entity_last_installed_schema_repository, Connection $database) {
   

   Missing PHP doc.

9. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlRevisionTreeHandler.php
   @@ -0,0 +1,175 @@
   +  protected function getAllRevisionParents($entity_id) {
   

   Could we add an Info suffix to the method name given that we are not returning revision objects?

10. +++ b/core/modules/system/system.install
    @@ -2239,3 +2240,160 @@ function system_update_8601() {
    +  \Drupal::service('plugin.manager.field.field_type')->clearCachedDefinitions();
    

    Is it safe to do this? I vaguely recall seeing issues caused by clearing caches in updates, since this will trigger hooks. I don't have alternative ideas though.

11. +++ b/core/modules/system/system.install
    @@ -2239,3 +2240,160 @@ function system_update_8601() {
    +function system_update_8702(&$sandbox = NULL) {
    

    I was wondering why this is not done in a post update function leveraging entity query + entity save. Is that because we would be forced to process each entity individually?

12. +++ b/core/modules/system/system.install
    @@ -2239,3 +2240,160 @@ function system_update_8601() {
    +    $step_size = Settings::get('entity_update_batch_size', 50) * 10;
    

    This would make it impossible to test the update with batch size 1. What about using a different setting altogether?

13. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityRevisionsTest.php
    @@ -261,4 +269,110 @@ public function testIsLatestAffectedRevisionTranslation() {
    +    $this->assertArrayHasKey('revision_parent', $base_field_definitions);
    

    Can add an assertion that a non revisionable entity type doesn't have the key?

14. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityRevisionsTest.php
    @@ -261,4 +269,110 @@ public function testIsLatestAffectedRevisionTranslation() {
    +    // Check that a new revision that doesn't start from the latest one gets the
    +    // proper revision parent ID, which is the active revision of the branch.
    +    $revision_4 = $this->storage->createRevision($revision_2, FALSE);
    +    $revision_4->save();
    +    $this->assertEquals([$revision_3->getRevisionId()], $revision_4->getParentRevisionIds());
    

    I initially thought this would be the way we'd start a new branch, but I can see how this would not work with reverted revisions. Do we already have a strategy for branching? Maybe we consider a new branch to be created when the new revision contexts differ from the parent revision ones?

15. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityRevisionsTest.php
    @@ -261,4 +269,110 @@ public function testIsLatestAffectedRevisionTranslation() {
    +    /** @var \Drupal\entity_test\Entity\EntityTestRev $revision_1 */
    +    \Drupal::currentUser()->setAccount($this->createUser());
    

    These lines should be swapped.

16. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityRevisionsTest.php
    @@ -261,4 +269,110 @@ public function testIsLatestAffectedRevisionTranslation() {
    +    $violations = $revision_2->revision_parent->validate();
    ...
    +    $violations = $revision_3->revision_parent->validate();
    ...
    +    $violations = $revision_4->revision_parent->validate();
    

    Why parent? Aren't we supposed to validate the entity object itself before saving it?

Thanks for the review, @plach!

Re #69:

1. Sure thing, done.
2. Very good point! We can do that and get rid of the public "enforce" methods, but we need to keep the internal flag because otherwise there's no way to differentiate between the cases when the parent revision has a value set by default (populated on entity load) and when a user sets the same value manually.
3. For the same reason we have a "taxonomy tree" and not a "taxonomy graph" :) I'm not really opposed to changing it if people feel strongly about it..
4. Fixed.
5. Fixed.
6. I'm not sure. I think it's better to validate at all times because these values could be set by an automated process (e.g. a migration) and ensuring the integrity of graph is very important.
7. I don't think we can have 0 as a revision ID, but fixed anyway.
8. Fixed.
9. Fixed.
10. Those problems were caused by clearing the entity type definitions cache, and I also can't think of another way to do it.
11. Yes, and that would be very slow. Another update function that's very similar is system_update_8400() and that's one of the few major updates that didn't cause problems.
12. I'm not sure about this, I think that a new setting would be very specific to this particular update function in which we are doing way less database operations than a regular entity (re)save.
13. Sure, added.
14. The strategy for branching is already included in the patch, where we set the parent revision by default to be the latest revision, and we just have to change from "latest" to "active" in #3023194: [PP-2] Add parallel revisioning support, after we figure out translation branching and whatnot :)
15. Fixed.
16. Good catch, that's a leftover from before moving the new constraint from the field type level to the entity level.

Regarding the generic revision reference field, the fact that we're not introducing it here is a good thing IMO. In addition to @Berdir's and @catch's objections for having something like that in core, at least one additional major problem with it is that it would be incompatible with ERR from the start, because it wouldn't have a reference to the entity ID (the target_id property which ERR inherits from ER).

Forgot to remove a call to setParentRevisionEnforced(), which was removed in #70. Also cleaned up a few more things.

Rerolled for #3037823: The system.theme.data key remains corrupted in state causing performance issues and is not used in >= 8.7 which added system_update_8701() that was also used by this patch.

Fixed those unit tests.

Initial review

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -322,6 +337,54 @@ public function updateLoadedRevisionId() {
   +  public function setParentRevisionIds(array $revision_ids) {
   +    // A revision can have a maximum of two parents.
   +    assert(count($revision_ids) <= 2);
   +
   +    /** @var \Drupal\Core\Entity\ContentEntityTypeInterface $entity_type */
   +    $entity_type = $this->getEntityType();
   +    if ($entity_type->isRevisionable()) {
   +      $revision_ids = array_values($revision_ids);
   +      $revision_parents = [
   +        'target_id' => isset($revision_ids[0]) ? $revision_ids[0] : NULL,
   +        'merge_target_id' => isset($revision_ids[1]) ? $revision_ids[1] : NULL,
   +      ];
   +
   +      $this->set($this->revisionParentKey, [$revision_parents]);
   +    }
   +
   

   i think the API here would make more sense if it was explicit

   public function setParentRevisionIds($target_id = NULL, $merge_target_id = NULL)

   As it stands, its a bit of a mystery API

2. +++ b/core/lib/Drupal/Core/Entity/EntityRevisionTreeHandlerInterface.php
   @@ -0,0 +1,29 @@
   +   * @param int|string $entity_id
   +   *   (optional) If the two revisions belong to the same entity, the entity ID
   +   *   can be used for optimizing the process used to build the revision tree,
   +   *   otherwise the entire revision history might be loaded into memory.
   +   *   Defaults to NULL.
   

   I'm interested to learn when two revisions might have a common ancestor but not be the same entity. Are we trying to support some sort of cloning here? Are we over complicating it by trying to support that this early?

3. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidRevisionParentConstraintValidator.php
   @@ -0,0 +1,86 @@
   +    $parent_revision_ids = array_filter(array_values((array) $entity->get($field_name)->get(0)->getValue()));
   ...
   +      $revision_ids_to_check = array_filter($parent_revision_ids);
   

   any reason to array_filter twice?

4. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidRevisionParentConstraintValidator.php
   @@ -0,0 +1,86 @@
   +      if (count($parent_revision_ids) === 2 && (string) $parent_revision_ids[0] == (string) $parent_revision_ids[1]) {
   

   given the type coercion here, should we use ===?

5. +++ b/core/lib/Drupal/Core/Entity/RevisionableInterface.php
   @@ -63,6 +63,33 @@ public function getRevisionId();
   +  public function getParentRevisionId();
   ...
   +  public function getMergeParentRevisionId();
   ...
   +  public function setParentRevisionIds(array $revision_ids);
   

   We are adding methods to an interface here, so that is an API change. As far as I can tell, it doesn't qualify under the 1:1 rule as there is no base class directly beneath this, instead there is one beneath a child interface (RevisionLogInterface) - is it feasible someone implemented RevisionableInterface but not RevisionLogInterface and hence did not use the RevisionablyContentEntityBase?

6. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlRevisionTreeHandler.php
   @@ -0,0 +1,187 @@
   +    $query = $this->database->select($revision_tree_table, 't');
   +    if ($entity_id) {
   +      $query->condition($id_column, $entity_id, '=');
   +    }
   +    $query->addField('t', $revision_id_column, 'id');
   +    $query->addField('t', $parent_revision_id_column, 'parent_id');
   +    $query->addField('t', $merge_revision_id_column, 'merge_parent_id');
   +    $query->orderBy($revision_id_column, 'ASC');
   

   can you elaborate (perhaps in a comment) as to why this can't be done with an entity query if these are just entity fields?

7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/RevisionTreeItem.php
   @@ -0,0 +1,88 @@
   +      'indexes' => [
   +        'target_id' => ['target_id'],
   +      ],
   

   no index on merge_target_id? we use it in where clauses so I would expect to see one.

8. +++ b/core/modules/system/system.install
   @@ -2247,3 +2248,169 @@ function system_update_8701() {
   +function system_update_8702() {
   +  $entity_definition_update_manager = \Drupal::entityDefinitionUpdateManager();
   +
   +  // Get a list of revisionable entity types.
   +  /** @var \Drupal\Core\Entity\EntityTypeInterface[] $definitions */
   +  $definitions = array_filter($entity_definition_update_manager->getEntityTypes(), function (EntityTypeInterface $entity_type) {
   +    return $entity_type->isRevisionable();
   +  });
   

   should this be done in a batch using the sandbox?

Thanks for the review!

Re #77:

1. Very good point and suggestion, done :)
2. Yes, cloning is exactly what we're supporting there, per @hchonov's (very insistent) arguments. IMO, it's not complicating things that much, we just need to allow the $entity_id parameter to be NULL even though core won't use this feature.
3. No reason at all, fixed.
4. Sure thing, done.
5. We added many new methods to RevisionableInterface, I think the most recent example is #2891215: Add a way to track whether a revision was default when originally created. The expectation here is that people should extend from ContentEntityBase.
6. That's because entity queries always return a set of revision and entity IDs, never field values. You could get field values from aggregate entity queries, but that's not useful in this instance.
7. We never use merge_target_id in WHERE clauses (just in SELECT ones), that's why it doesn't have an index.
8. All update functions from D8 that added a field to all content entity types were done without using the batch sandbox :)

@Berdir did *a lot* of manual testing for the update function from this patch, and the conclusions are:

• the 10x multiplier for entity_update_batch_size is definitely worth it
• removing the count query at the end of each batch run provides a ~30% performance increase; the side effect is that revisions that might be written during the last batch run won't be updated, but on the other hand there might be other entity schema updates that need to run as well, so there's no guarantee that any entity save would work at all while running db updates
• the batch logic was broken but it has now received a very rigorous real-world testing on a database with 800K node revisions and many paragraphs as well

Saving credits.

Personally I'd still prefer a slightly different approach to track parents: a new simple revision reference type (not an ERR!) and two base fields to track parent and merge parent, since parent and merge parent have very defined roles and we don’t foresee to have more parents in this specific context. Having this generic reference field would allow us to implement generic DAGs supporting multiple parents, and any component working with graphs could be adapted to work with both generic DAGs and this specific implementation in the same way.

@plach++
I completely support this.

I am fine with having only two properties on a simplified ERR - entity (computed) and target_revision_id (not target_id because it is super confusing to refer to a revision id simply by ID). This gives the advantage of making it easy to load the parent revision and cache it on the field item.
Additionally the column in the shared table will have a clear naming - revision_parent, merged_revision vs revsion_parent__target_id, revsion_paret__merge_target_id. It also makes it possible to listen to changes on the fields in ContentEntityInterface::onChange, which is impossible to address for only one of the properties if it is a combined field. The revision parent and the merged revision are two units, completely independent of each other and putting them together disregards the separation of concerns design principle.

What problems do you see about a simplified ERR field type without widgets? Even if we don't want ERR then why not having 2 simple integer fields?

The current issue mixes 3 different concepts which could be perfectly discussed in isolation, especially given the fact that we don't reach full agreement on all the aspects - revision parent, merged revision and least common ancestor. This separation would make it easier to review and discuss instead of mixing it all together.

We've always needed the revision parent reference and it is not only needed because of the revision branches, while the merged revision is needed only because of the revision branches.

Per the explanation above, I don't think there's any disagreement here. A merge revision has no special meaning in the graph, it will only be a common ancestor when two branches are split from it, which is not the case in the example you gave earlier.

Then why is the attached test failing?

Then why is the attached test failing?

Because the expectation of the test case is wrong. I've reconstructed your graph, it looks like this:

    // 10 -- 20 -- 21 -- 22
    //   \       /
    //    \     /
    //     \   /
    //      30 -- 31

And the test expects the LCA of 22 and 31 to be 10, when it's 30 instead.

Re-uploading the patch from #79.

No, the expectations are not wrong. You've said :

A merge revision has no special meaning in the graph, it will only be a common ancestor when two branches are split from it

10 should be the least common ancestor in that test. I've already explained pretty detailed why it is wrong to assume anything else than 10 to be the least common ancestor.

Testbot is finally back!

+++ b/core/lib/Drupal/Core/Entity/RevisionableInterface.php
@@ -63,6 +63,34 @@ public function getRevisionId();
+  public function getParentRevisionId();
...
+  public function getMergeParentRevisionId();
...
+  public function setParentRevisionIds($target_id = NULL, $merge_target_id = NULL);

Sorry, I thought it was clear from my previous review, but I was suggesting to have two separate setters to match the two getters.

@plach, works for me.

That was an unrelated random test failure in LayoutBuilderDisableInteractionsTest, queued a re-test.

@hchonov:

Additionally the column in the shared table will have a clear naming - revision_parent, merged_revision vs revsion_parent__target_id, revsion_paret__merge_target_id.

The name of the columns in shared tables have no impact on the functionality of this feature.

It also makes it possible to listen to changes on the fields in ContentEntityInterface::onChange, which is impossible to address for only one of the properties if it is a combined field.

It is possible to check which of the two properties of the field has been changed in ContentEntityInterface::onChange() by getting the current field item values with $this->get(...) and comparing them with what's stored in $this->values.

The revision parent and the merged revision are two units, completely independent of each other and putting them together disregards the separation of concerns design principle.

The revision parent and the merged revision are two edges that belong to a vertex of the graph, so it makes sense for them to be stored together, very much like the taxonomy DAG which uses a multi-valued field to accomplish the same goal. In the case of the revision graph, we know in advance that we can have a maximum of two edges for a vertex, so we can optimize its storage by grouping them in two properties/columns of a single field item. Therefore I don't think this disregards the separation of concerns design principle at all.

What problems do you see about a simplified ERR field type without widgets?

This was answered more than once by @Berdir, @catch and myself.

Even if we don't want ERR then why not having 2 simple integer fields?

See the answer above on why I think they should be stored together.

We've always needed the revision parent reference and it is not only needed because of the revision branches, while the merged revision is needed only because of the revision branches.

The ability to have revision branches is a desired feature for #3023194: [PP-2] Add parallel revisioning support, which means we need to track merged revisions.

After the discussion we had with @kattekrab, @catch, @plach, @dixon_, @amateescu, @Berdir, @hchonov @amateescu to elaborate a bit on why I prefer a different approach. Forming my thoughts still a bit, will write something up in a bit. In looking a bit at this issue, I did notice however, that the issue summary is very sparse and especially given the fact that we had so much discussion I think the current approach should be explained a bit more in detail. Marking needs work for that.

And here is the alternative with 2 fields. I haven't added the computed entity property to the new RevisionReference field in order to keep the changes as minimal as possible. We could add it in a follow-up issue.

OK we discussed this on a call this morning, trying to summarise:

1. The majority of the people on the call do not have a strong opinion between two fields vs. a two column field.

2. The advantage of a two column field is it keeps the DAG information in 'one place'.

3. The advantage of two fields is that the field type is much less customized.

Between 2 and 3, in both cases the revision is semantically different information (i.e not quite the same as taxonomy parents, but it is a multiple parent relationship), so most of the choices between them are subjective.

4. @hchonov brought up things like checking if the field has changed or is empty - to be honest I don't see much use case for that with metadata fields. Similarly swapping out the parent revision field with something else (like pointing to a different entity instead of a different revision) - to me this seems like a completely different use case that should use a different field altogether - entity revisions are a specific concept to 'parent revision' should point to one.

5. I was concerned that switching to a two field approach would invalidate the upgrade path and performance testing that's already been done on this issue, however from the interdiff above, that's not the case, the upgrade path has not changed very much (nor the patch more generally).

This is a very partial account from quite a long phone call.

One additional question that I have is given the large batch size on the update, do the upgrade path tests here definitely cover multiple batches - this is an issue we had with taxonomy parents. I did not try to verify this yet so apologies if it's obvious in the patch.

One additional question that I have is given the large batch size on the update, do the upgrade path tests here definitely cover multiple batches - this is an issue we had with taxonomy parents. I did not try to verify this yet so apologies if it's obvious in the patch.

We committed #3007606: Force all update path tests to only run one entity per batch 9 hours ago to help this exact case :)

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -508,10 +516,7 @@
 
     // Set the parent revision ID automatically when we create a new revision.
     if ($this->isNewRevision() && !$this->enforceRevisionParent) {
-      // By default, the parent revision is the active revision of the branch.
-      // @todo Use the active revision ID once it is available.
-      //   @see https://www.drupal.org/project/drupal/issues/3023194
-      $this->setParentRevisionId($storage->getLatestRevisionId($this->id()));
+      $this->setParentRevisionId($this->getLoadedRevisionId());
     }
 

Why is this change necessary?

Because I think that the revision parent of a new revision is the old revision.

When we have revision ids 1,2,3,4 and 4 is the default and we load revision 2 and save it in a new default revision then the revision parent of the new revision is 2 and not 4.

When we have revision ids 1,2,3,4 and 4 is the default and we load revision 2 and save it in a new default revision then the revision parent of the new revision is 2 and not 4.

This introduces a hardcoded way of branching that the previous patch did not, which I think needs to be properly discussed in #3023194: [PP-2] Add parallel revisioning support.

The interdiff is looking good to me (of course :), however #108 is exactly what I was about to post, that change is definitely follow-up material.

This introduces a hardcoded way of branching that the previous patch did not, which I think needs to be properly discussed in #3023194: [PP-2] Add parallel revisioning support.

This introduces a way of setting the correct parent. You create a new revision out of a currently loaded entity object and this object represents the parent and not the newest entity revision from the storage.

That decision has no relationship to whether we have one field or two fields, so I'd prefer to discuss it in a follow-up.

If revisions 1, 2, 3, and 4 are all default revisions, then even though revision #5 is using the exact same content as #2, the difference on the live site will be between #3 and #4 and #4 and #5. I could also create a new revision that happens to have exactly the same content as revision #2. Is this really semantically different?

If revisions 1, 2, 3, and 4 are all default revisions, then even though revision #5 is using the exact same content as #2, the difference on the live site will be between #3 and #4 and #4 and #5. I could also create a new revision that happens to have exactly the same content as revision #2. Is this really semantically different?

It is not about the diff between revisions, but about how the revision graph develops. In this case the revision has been created out of 2 and not out of 4.

I don't think that it is correct to have the inconsistency, which saves an incorrect information:

$entity_original = $storage->loadRevision(2);
$entity = $storage->loadRevision(2);
$entity->setNewRevision();
$entity->save();
$entity->revision_parent->target_revision_id != $entity_original->getRevisionId()

Additionally if we save 2 as the parent then the following will function correctly, while it will not if we use 4 as parent:

$entity->setNewRevision();
$entity->save();
$entity->original = $entity->revision_parent->entity;
$entity->hasTranslationChanges() => should emit FALSE, because there are no changes. 

This should be fixing the test fails.

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidRevisionParentConstraintValidator.php
@@ -0,0 +1,86 @@
+      $revisions = $storage->loadMultipleRevisions($revision_ids_to_check);

Would it be more efficient if we execute an entity query instead of loading the entities? Here we need to know only if a revision ID exists or not.

So the reason why I preferred to have two separate fields is that I think tracking the revision parent is really important even without any merging of revisions. I.e. if every revision only has a single parent it is worthwhile to track each revision's parent.

The use-case is the current Revert UI we have in core and it is very much related to the discussion started in #106. When you revert to an old revision we take that old revision and save it as a new revision. It is currently not possible to reconstruct this history after the fact (unless you inspect the log message...). The revision parent field makes this possible.

Therefore I like having two separate fields because then I can remove the merged revision ID from my entity types if I do not need that feature. I am not proposing any facility for this removal to be added to core but I can still alter the field out if I want to. This would have been a bit trickier as a custom field type.

Additionally, I think having a revision reference field in core is very very useful in general. In fact (in a follow-up!!!) we can explore using that field (or possibly a more powerful subclass) for content moderation state's entity revision ID reference field.

Now addressing the discussion in #106 and following:
On process: I'm not sure how this can be discussed in a follow-up if changing the implemenation necessarily constitutes a behavior change at that point? I.e. if a different behavior occurs when reverting revisions in the UI how can we discuss and change that post-alpha?
On the issue itself: I was quite surprised to see #54 change from getLoadedRevisionId() to using the latest revision and then glad to see it changed back by @hchonov. Given the above use-case of reverting using the UI I really cannot understand how one can come to the conclusion that the revision I reverted to is not the parent of the newly created revision. I would really like to hear further reasoning on that.

I already stated my opinion on the process of this issue in the call so I will not repeat that here, but one addendum now that I have reviewed the patch: I really think it is unusual to have a patch with such a broad scope. As in fact @hchonov pointed out above we are tackling three (very closely related, albeit distinct) topics in one patch which is very unusual to say the least.

Thanks @dixon_ for the issue summary update. I think it still could into a bit more detail, but maybe others can chime in on that as well. Also, this will certainly need a change record.

@tstoeckler:

Thanks for the additional insights, I won't reply to what #117 brought up yet.

Regarding process:

I really think it is unusual to have a patch with such a broad scope. As in fact @hchonov pointed out above we are tackling three (very closely related, albeit distinct) topics in one patch which is very unusual to say the least.

Revision parent and merge revision parent were obviously tackled together because the fact that they could have been tackled separately has been a matter of discussion until now. @amateescu explicitly asked me whether the LCA stuff should have been a separate issue (don't remember whether that happened here or in Slack) and I told him that addressing that in this issue could have provided some validation and implicit test coverage around the API being introduced. I apologize if this introduced additional confusion and I hope this helps clarifying the scope of the patch, even if you may disagree with it. I would not be opposed to splitting the LCA part out, but in practical terms I don't think it would make a big difference now that the patch has been reviewed several times.

Some more test adjustments.

This should fix the failures.

And this addresses #106 and #111, so now we have all the options on the table.

Of course test coverage needs to be updated as well.

@tstoeckler, thanks for sharing your feedback, it is useful to have more points of view written down in a clear manner so we can have a healthy debate.

As far as I see, there are three major discussion topics:

1. Two competing architectural designs for this feature: one field vs. two fields
2. Branching strategy for the revision graph
3. The process of this issue, specifically the fact that it handles three seemingly distinct features in one patch

I'll try to address them in reverse order.

3. Process

@plach already replied to this in #118, but I'll provide a quick summary as well:

• having both a parent revision and a merge revision is a central part of the design for the revision graph feature that we want to introduce here (see the issue summary of #3023194: [PP-2] Add parallel revisioning support for additional context), so it is natural for them to tackled together
• including a generic algorithm for finding the LCA of two revisions was discussed with a framework and Entity API maintainer, and it was deemed useful for additional validation of the proposed solution of this issue
• therefore I hope we can agree that there are only two distinct features, and the second one can be moved to a followup if it's really contentious, even though the LCA algorithm is generic and it doesn't make any assumption on the branching strategy of the graph
• I'm not sure what was the reason for emphasizing "which is very unusual to say the least" for this point. There is no conspiracy here, just well-intentioned people trying to do their best. In any case, it was totally uncalled for.

2. Branching strategy

The reason for the change in #54 is that this patch is intentionally neutral in regards to branching strategies, and it has a very specific purpose: to keep the current behavior from HEAD which has a linear revision history (by not tracking the revision history at all).

Keeping this patch unopinionated allows us to properly discuss branching strategies and their merits or shortcomings in #3023194: [PP-2] Add parallel revisioning support.

1. One field vs two fields

I think this can only be addressed if we lay out the "pros" and "cons" lists for each of them.

One field using a custom field type

pros:

• conceptually correct design for storing a DAG in a Drupal field (similar in nature to how we store the taxonomy graph): provides an optimized storage for a multi-cardinality field with known constraints (cardiniality <= 2)
• lightweight and maintenance free: minimal code implementation that essentially provides two integer properties, with no other additional features or complexity planned
• performance: it doesn't require two separate base field definitions

cons:

• not reusable: this is by design, see the 'lightweight' point above
• a bit harder (but very much possible, see #100) to see which of the two properties has been changed in ContentEntityInterface::onChange()
• harder to remove the merge parent if it's not needed for a specific project: however, note that the API methods for interacting with the merge parent can not be removed from the interface, and as soon as some code calls them, they expects to have "something" to interact with

Two fields with a minimal implementation of a revision reference field type

pros:

• reusable: I will point out below why this might not be true
• lightweight but not necessarily maintenance free, also described below

cons:

• it is incompatible with the ERR implementation from contrib, because it lacks the target_id property/column among other things
• stores the two edges of a graph's node in two separate fields. This is specifically for @tstoeckler's point from the call that this issue does not strive for a proper architecture as we try to do in Drupal, while I very much think that it does, as per the first "pro" of the one field approach.
• less performant: it requires two base field definitions
• heavily debated feature set, see below

Now, let's talk about the first point from the "con" list of the revision reference field type because I believe that's the most important one. As it stands from the latest patch in #127, there are two proposals to alleviate this point:

- add an optional target_id property (which would be activated by a field storage setting) and a target_type setting, neither of them being required (nor desired) features for the revision graph that we are introducing here. If I remember correctly, @tstoeckler expressed concerns in the past about varying the columns (or properties?) of a field type based on a field setting. I can't recall the specific context nor the exact words from his comment on that issue, but I can try to look it up if needed.

- extend the minimal implementation of the field type from the patch in #127 with another class, therefore creating an additional field type that would provide the ability to reference revisions from a different entity type.

Both of these options have been opposed at length by @Berdir in #65 and @catch in #68. Also, both options will provide some functionality currently provided by ERR in contrib, but not all of it (the "compound entity" concept), and we will end up with either 2 or 3 different revision reference field types (1 or 2 in core and 1 in contrib), neither of them being feature-equivalent.

This is why I think that the minimal implementation of a generic revision reference field type from #127 introduces a maintenance risk at least, because the (future) feature set and complexity of this field type are largely unknown, heavily debated and potentially not even desired for Drupal core.

The third option would be to leave the minimal implementation as-is, which removes the reusability point from its "pro" list and leaves only the lightweight one, which is also provided by the custom field type approach.

Please feel free to correct me if I missed something.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/RevisionReferenceItem.php
@@ -19,21 +19,11 @@
-  public static function defaultStorageSettings() {
-    return [
-        'target_type' => NULL,
-      ] + parent::defaultStorageSettings();
-  }
...
-    $settings = $field_definition->getSettings();
-    $target_type = \Drupal::entityTypeManager()->getDefinition($settings['target_type']);
+    $target_type = \Drupal::entityTypeManager()->getDefinition($field_definition->getTargetEntityTypeId());

What is the reason for this change?

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -516,7 +516,10 @@ public function preSave(EntityStorageInterface $storage) {
-      $this->setParentRevisionId($this->getLoadedRevisionId());
...
+      $this->setParentRevisionId($storage->getLatestRevisionId($this->id()));

This manipulates history information in a wrong way. Why do you want to always define the active revision as the parent revision? In this case when you don't have revision branches, but only the default revision then you will only have linear revisioning and you always can assume that parent revision = getLoadedRevsion() - 1. In this case this definition of parent revision makes no sense and is simply confusing. Why do we want to have it then at all?

If you are working only with a single branch, then the parent is the revision from which you've created the new one and not the last one, otherwise you lose the revision graph information and the tracking of what has exactly happened. When you have revisions 1,2,3,4 and then a user decides to revert to 2 and from there work further, then the reason is that the user decided to throw away the information/work from 3 and 4 and start a new work based on 2. This is very similar to a revision branch if you draw the graph out of it and look at it it becomes clear that the revision parent of 5 is 2 and not 4.

1
|
2
|  \
3  5
|
4

If you are working between different revision branches and merge an entity from branch B into Branch A and use the current active revision of branch A as a base, then the parent will be that very same active revision and the merged one will be the one from B. If I understand it correctly this is the behaviour you are trying to achieve.

The approach that we are proposing works for both cases.

Re #129:

(Only replying where I have anything to add. I have a different opinion than you on a bunch of things brought up, but I don't think it's good use of all of our time to rehash every little detail of every point.)

I'm not sure what was the reason for emphasizing "which is very unusual to say the least" for this point. There is no conspiracy here, just well-intentioned people trying to do their best. In any case, it was totally uncalled for.

I am a bit baffled by this point, to be honest. By saying this is unusual, I meant the literal meaning of that word, I don't know how else to explain that. The scope of issues has long been a very contentious issue in core development and it is my opinion that this issue was handled in a different way most other issues are, i.e. in a way that is not usual. You may of course disagree with my assessment but I don't know why stating that opinion was in any way "uncalled for". I did not in any way allude to any conspiracy or anything of the like (nor do I believe there is one, just to to make this 100% clear). I wouldn't go so far as to say it is "uncalled for" but I find the fact that you make these broad insinuations based (solely?) on my usage of emphasis is a bit unnecessary. I may have been able to express what I meant with better wording, but I would appreciate if we can focus on the points of discussion at hand and not nitpick wording or markup.

The reason for the change in #54 is that this patch is intentionally neutral in regards to branching strategies, and it has a very specific purpose: to keep the current behavior from HEAD which has a linear revision history (by not tracking the revision history at all).

Keeping this patch unopinionated allows us to properly discuss branching strategies and their merits or shortcomings in #3023194: [PP-2] Add parallel revisioning support

I don't understand what "neutral" means in this case, we have to fill the parent revision with something, and we need to decide on a strategy for doing that, I don't see how choosing one strategy over another is neural. Same goes for considering this approach "unopinionated".

I also genuinely don't understand how you can consider this as keeping the current behavior from HEAD as this is an entirely new feature. HEAD has absolutely no revision history at all, which is what we are fixing in this issue.

As stated above, I furthermore don't see how changing the strategy after we have introduced it here is acceptable in terms of BC. Once we have now decided what the parent is of a reverted revision, how can changing that behavior not be considered an API change. For that reason I don't think it's valid to defer this to a follow-up.

stores the two edges of a graph's node in two separate fields. This is specifically for @tstoeckler's point from the call that this issue does not strive for a proper architecture as we try to do in Drupal, while I very much think that it does, as per the first "pro" of the one field approach.

Why is this listed under cons? You are just describing how it works not judging it in any way here.

less performant: it requires two base field definitions

I understand the implications of (base) field definitions but stating in this broad sense that it is "less performant" is simply not correct. If you seriously consider this an impediment of the current patch you need to provide some more concrete reasoning.

heavily debated feature set, see below

Below you are just talking about the first point, so using that as another argument is somewhat circular argumentation.

Now, let's talk about the first point from the "con" list of the revision reference field type because I believe that's the most important one.

To be honest, I don't think not having this compatible with contrib ERR is a big issue. For the use case of Paragraphs / compose entities / ... it turned out to be very beneficial if not utterly necessary to store both the revision ID as well as the entity ID. But that is one use-case of many. In general being to able to reference a specific revision of an entity is a nice feature to have.

The third option would be to leave the minimal implementation as-is, which removes the reusability point from its "pro" list

As said above I completely disagree with this. In particular if we do add a target_type setting in the future (which would also not affect the storage) it would be a really nice addition to the toolkit of data modelling-

Re #131:

IMO, the architecture with two fields does not optimise for robustness and performance, but for modularity instead.

See above for why I disagree with @amateescu's arguments which you base this on. But even if I stipulate @amateescu's arguments coming to this conclusion seems to be a bit far-fetched. Lacking robustness in the context of this issue implies there is an issue with data integrity which is not the case, so I want to point that out explicitly.

As stated above, I furthermore don't see how changing the strategy after we have introduced it here is acceptable in terms of BC. Once we have now decided what the parent is of a reverted revision, how can changing that behavior not be considered an API change.

The specifics of how we manipulate entities aren't an API - reverting an entity is a UI operation, we don't even have an API for it.

For me, the parent revision of a default revision should always be the previous default revision. This is what allows you to trace a linear default revision history once you have forward revisions.

So:

Revision 1 default
Revision 2 draft
Revision 3 default - the parent should be revision 1, and revision 2 would be the merge reference.

When we save revision 3, $entity->original is revision 1 - this is what hooks compare against for example.

That revision 2 was an actual revision at all is an implementation detail - it could have been in temp store or whatever, but the merge reference could then point to that"

1 ----- 3 (default)
  \    /
    2 (draft)

Just for reference, I think we can change the behaviour of the default parent revision during alpha (or probably afterwards too), but I don't think we can change between one fields/two fields.

@hchonov:

1
|
2
|  \
3  5
|
4

With this version of the graph, I can't tell whether 3 and 4 were abandoned drafts, or were published versions that got reverted.

In git terms, 3 and 4 look like 'dangling commits' or an abandoned branch in that graph, when in fact they're commits in the 'master' commit history.

However if we keep linear default revision parents and use the merge ref to point to the revert source:

1
|
2
|  \
3  |
|  |
4  |
|  /
5

Now I can see that 4 and 3 followed 2 sequentially as default revisions, but also that 5 followed 4 and is based on 2.

However even this is not completely correct, because in Drupal a revert is not actually a revert (nor a rebase).

All 'reverting' a revision does, is allow you to create a new revision based on the content of an old one. In technical terms it's not different to if you open up the edit form, make the same change to the content, and save it again.

So in that case, the 'revert' history would be:

1
|
2
|
3
|
4
|
5

Because in fact this is the actual history of the revisions, and whether someone wants exactly the same content as 2, or just happens to edit content again to be the same as 2 (and both cases are possible, such as a status change in the issue queue or promoting/unpromoting from front page) is really incidental, we just provide the revert UI to make it easier.

Thank you @catch for your detailed explanation. At least now I understand where you are coming from. I don't agree with the assessment but if you - as a release manager - say that it is fine to change this behavior after this issue has gone in, I'm fine with discussing this in a follow-up. Therefore I won't discuss this issue further here.

To be completely honest I am a bit wary of that because it will make it more of an uphill battle for me and anyone agreeing with me, as with this being a new feature all options for the possible behavior are on equal footing before it lands, but after it does I will have to make an active case to change it, but I guess that's just tough luck for me in this case.

I do feel it is worth noting, however, that even after we had a >1 hour call which included all Entity API maintainers, this rather fundamental discussion on how a revision history, which this issue enables, should work in the context of reverting only arose now while at the same time this is being considered for inclusion in 8.7.x on the premise that it was "done"/"ready"/... last Friday.

@tstoeckler, #137:

I do feel it is worth noting, however, that even after we had a >1 hour call which included all Entity API maintainers, this rather fundamental discussion on how a revision history, which this issue enables, should work in the context of reverting only arose now while at the same time this is being considered for inclusion in 8.7.x on the premise that it was "done"/"ready"/... last Friday.

To be fair I was surprised as well, I didn't realize we had disagreement also on this aspect until #103, so I thought the only issue to overcome was the 1 field vs 2 fields discussion. I fully agree that having all entity API maintainers on the same page on this matter is highly desirable and something I spent all the weekend mulling over (and that's the reason why I set up the call in the first place). I still think that there is nothing fundamentally flawed in any of the solutions we discussed during the call on Friday (#97 and #127), they just seemed two competing approaches with similar merits and both acceptable. After picking one we would have been ready for commit from my perspective.

Clearly, I have more concerns now that also #123 is on the table, but as long as we can still change the approach during alpha, I'm ok with discussing this in a follow-up. I guess we could even roll back the commit from 8.7.x and only leave it in 8.8.x, if it turns out there are serious flaws that we need to account for.

Personally I'm trying to do everything possible to make this land in 8.7.x, because I'm sensitive to product management goals and this issue was flagged as critical for the Workflow Initiative success, however I'm not willing to do that at the cost of introducing irreparable technical debt. I'm still not convinced this is what we are doing here though.

I have no problem to admit that, weren't we this close to alpha, I'd be ok with postponing commit to the resolution of this last debate, however committing now and possibly revisiting this change (or rolling it back) later still gives us a chance to make the 8.7 deadline. I don't think we would be bending any rule in doing so and I definitely don't think it would be unprecedented. I hope you'll agree with me at least on this point :)

To be fair I was surprised as well, I didn't realize we had disagreement also on this aspect until #103

Just to be 100% clear: I did not mean to insinuate that this was in any way hidden by anyone or that there was any ill intent. I think that we were all quite convinced that what we had in mind was self-evidently correct, unfortunately we didn't all have the same thing in mind.

Re #134:

For me, the parent revision of a default revision should always be the previous default revision. This is what allows you to trace a linear default revision history once you have forward revisions.

In this case what for do we need the field if there are no revision branches and no forward revisions? The parent is then always the previous revision ID. Talking in regards to performance it does not make sense to track this information when there are no revision branches and it only introduces a small but unusefull overhead. Also with linear revisioning I lose the information about what has happened.

Re #136:

@hchonov:

1
|
2
|  \
3  5
|
4

With this version of the graph, I can't tell whether 3 and 4 were abandoned drafts, or were published versions that got reverted.

In git terms, 3 and 4 look like 'dangling commits' or an abandoned branch in that graph, when in fact they're commits in the 'master' commit history.

3, 4 are part of the default revision branch and have their revision history and "was_default" revision field, which makes it possible to "tell whether 3 and 4 were abandoned drafts, or were published versions that got reverted.".

If you compare the tree to the tree with the revision branches, then you can notice that even without an explicit revision branch, we still create a revision branch - an implicit one.

However if we keep linear default revision parents and use the merge ref to point to the revert source:

1
|
2
|  \
3  |
|  |
4  |
|  /
5

Now I can see that 4 and 3 followed 2 sequentially as default revisions, but also that 5 followed 4 and is based on 2.

@catch, I see your point and now I better understand your thinking. Thank you for explaining it.
However I don't agree with it, because for me 2-5 and 2-3-4 are just different "implicit" revision branches and 2 is the parent of both.

I don't feel comfortable comparing this to git, because our API is not based on git and it allows handling things differently - for example loading an older revision and saving it as a new one. In git the reverting of commits results in commits itself, which creates the linear revisioning. In git you explicitly specify which commits to revert, but in Drupal you can skip these intermediate steps by loading a previous revision, which then results into a new revision branch 2-5, while the previous branch 2-3-4 gets abondend.

Reverting multiple commits in git is explained here pretty well - https://stackoverflow.com/a/1470452 .

If we want to do it like git does, then this means that consecutively we have to prevent saving an old revision as the new default revision. Doing this will be pretty bad BC break and as we cannot do it, we can't really be conform with git.

Re #138:

To be fair I was surprised as well, I didn't realize we had disagreement also on this aspect until #103

Because prior the call we weren't aware of that and it wasn't mentioned in the issue summary, which is why it came out later.

Clearly, I have more concerns now that also #123 is on the table, but as long as we can still change the approach during alpha, I'm ok with discussing this in a follow-up. I guess we could even roll back the commit from 8.7.x and only leave it in 8.8.x, if it turns out there are serious flaws that we need to account for.

Personally I'm trying to do everything possible to make this land in 8.7.x, because I'm sensitive to product management goals and this issue was flagged as critical for the Workflow Initiative success, however I'm not willing to do that at the cost of introducing irreparable technical debt. I'm still not convinced this is what we are doing here though.

I have no problem to admit that, weren't we this close to alpha, I'd be ok with postponing commit to the resolution of this last debate, however committing now and possibly revisiting this change (or rolling it back) later still gives us a chance to make the 8.7 deadline. I don't think we would be bending any rule in doing so and I definitely don't think it would be unprecedented. I hope you'll agree with me at least on this point :)

Full disclosure - neither me nor Tobias is paid by the mentioned sponsorship for the workflow initiative during the call. Insisting on commiting this issue in 8.7.x already has caused a pressure on us. Commiting it now will cause even more pressure on us as it will leave us only with a time frame during the alpha release. Considering this issue deals with 3 different topics at the same time, I don't think this is correct.
I don't know about the mentioned sponsorship, but the company I work for sponsors the workflow initiative as well - by having paid for 99 % of the current code of both autosave_form and conflict 2.x. I've visited Drupal Camp Mountain, Davos 2019 this weekend with the agenda of working towards a stable conflict 2.x release based on an agreement for an approach after talking to Philipp Melab. Reference - #2867707-27: WI: Phase H: Conflict management and local workspace merging support. But after all I've ended up with dedicating my whole time this weekend and my energy to this issue instead. Commiting this now having so much disagreement is not correct considering not everybody here can dedicate that time in this little time frame. Also there are no technical reasons for having it in 8.7.x, because everything which will be using it will be targeted against 8.8.x, meaning commiting it and using it in 8.8.x should be fine as well.

Full disclosure: I'm not paid by the aforementioned company as well, and I spent most Friday on this issue, despite none paying me for that at all, just because I think that the overall benefits of having this in 8.7 outweigh the drawbacks.

I fully understand your point but it seems me that if we can't even compromise on this priority, the only solution you're willing to accept is one that fully meets your criteria, despite there being quite a few people disagreeing with those.

We can try to get this matter sorted now. If we can't have this land in 8.7, I won't lose (more) sleep.

@tstoeckler, #140:

Just to be 100% clear: I did not mean to insinuate that this was in any way hidden by anyone or that there was any ill intent. I think that we were all quite convinced that what we had in mind was self-evidently correct, unfortunately we didn't all have the same thing in mind.

Same here, thanks for saying that, it's appreciated.

In this case what for do we need the field if there are no revision branches and no forward revisions?

Forward revisions are supported by the API, whether or not there is content_moderation, workflow or another mechanism to create them. If we added the field when one of those modules is enabled, we'd have to populate it on install which feels painful. edit: Also uninstalling would mean removing the field and losing the history. So it makes sense to me to store the information all the time, even if the history technically mirrors the revision ID sequence.

I don't feel comfortable comparing this to git, because our API is not based on git and it allows handling things differently

To be honest I also think it's not useful comparing this to git.

- for example loading an older revision and saving it as a new one... in Drupal you can skip these intermediate steps by loading a previous revision, which then results into a new revision branch 2-5, while the previous branch 2-3-4 gets abondend.

Except that loading a previous revision, removing the revision ID, and saving it to create a new revision is not a 'revert API' - it's just how the entity system responds when it gets an entity without a revision ID. So the behaviour in the revert case vs. publishing a draft revision case don't need to be identical - what happens under the hood in the entity system might be the same but the intention is very different.

The last example in #136 (1 - 2- 3- 4- 5) shows that there is really no revision branching going on at all in the revert case, that we loaded revision 2 is incidental - an implementation detail of the user interface. Loading revision 4, copying the changed fields over from revision 2, then saving it would work just as well to 'revert'.

Once you get into actual revision branches (a series of draft revisions building on each other, or a fork from a draft to a second workspace etc.) then the parent field starts to show useful information, but I really think revert is a different case.

There's also the issue of translations, to be honest I can't remember how the revert UI works with translations, but for example:

r1 - English
r2 - English + French
r3 - English edit
r4 - English + French + Spanish
r5 - Revert English back to language in 1 (or 2), but preserve the French and Spanish translations.

In this case, the parent of r5 is not r1 or r2, but r4 - the content is a composite of the translations in r4, with the English content from r1/2. It's a completely new revision, that does not match the content of any of the previous revisions, it just happens to combine field values from previous revisions.

Re #141:

I fully understand your point but it seems me that if we can't even compromise on this priority, the only solution you're willing to accept is one that fully meets your criteria, despite there being quite a few people disagreeing with those.

I am not saying "I want it to be like this", but I am supporting everything I say with arguments. I am sorry if in any way it sounds to you like this, but this is absolutely not my intention. This statement was TRUE also for #2880149: Convert taxonomy terms to be revisionable, where I've raised concerns about the parent field being revisionable and I had hard times defending this, however now it turned out, that my concerns were correct and the parent field should not be revisionable - please refer to the latest comments on the issue.
The same statement however is valid in the other direction, because there are people disagreeing with your criteria as well. Please let us not make any accusations and maintain a healthy discussion. The situation is already pretty intense and I will consider this statement only as caused by it and will ignore it for the sake of the progress instead of getting offended.

For some parts here there has been no discussion, which is healthy and prevents bugs in the future and makes the API better. I think that all we want to have a better Drupal and no one wants to slow down something just because of believes or anything else different than the desire to invest in the progress of Drupal. Regarding a healthy discussion please see my next reply to @catch's comment.

Re #143:

- for example loading an older revision and saving it as a new one... in Drupal you can skip these intermediate steps by loading a previous revision, which then results into a new revision branch 2-5, while the previous branch 2-3-4 gets abondend.

Except that loading a previous revision, removing the revision ID, and saving it to create a new revision is not a 'revert API' - it's just how the entity system responds when it gets an entity without a revision ID. So the behaviour in the revert case vs. publishing a draft revision case don't need to be identical - what happens under the hood in the entity system might be the same but the intention is very different.

The last example in #136 (1 - 2- 3- 4- 5) shows that there is really no revision branching going on at all in the revert case, that we loaded revision 2 is incidental - an implementation detail of the user interface. Loading revision 4, copying the changed fields over from revision 2, then saving it would work just as well to 'revert'.

Exactly because you might look at it not only as a 'revert API' is the reason an implicit revision branch is created. You can use the API this way simply in the entity API not in the revision revert UI and in this case the currently loaded entity revision is the parent.

This is why I've stated in my previous comment:

If we want to do it like git does, then this means that consecutively we have to prevent saving an old revision as the new default revision. Doing this will be pretty bad BC break and as we cannot do it, we can't really be conform with git.

If we are unable to save an old revision as a new one, then a revert would work only by copying over values and in this case we will correctly end up with revision parent being the latest revision and merged parent being the entity to which state we want to revert to.

There's also the issue of translations, to be honest I can't remember how the revert UI works with translations, but for example:

r1 - English
r2 - English + French
r3 - English edit
r4 - English + French + Spanish
r5 - Revert English back to language in 1 (or 2), but preserve the French and Spanish translations.

In this case, the parent of r5 is not r1 or r2, but r4 - the content is a composite of the translations in r4, with the English content from r1/2. It's a completely new revision, that does not match the content of any of the previous revisions, it just happens to combine field values from previous revisions.

@catch++. Thank you for bringing up this example. It actually shows that we have unfinished work in the patch. \Drupal\node\Form\NodeRevisionRevertForm::prepareRevertedRevision() loads an old revision and declares it as the new one, but \Drupal\node\Form\NodeRevisionRevertTranslationForm::prepareRevertedRevision() makes use of \Drupal\Core\Entity\TranslatableRevisionableStorageInterface::createRevision(RevisionableInterface $entity, $default = TRUE, $keep_untranslatable_fields = NULL) which is documented as follow:

* When dealing with a translatable entity, this will merge the default
* revision with the active translation of the passed entity.

This case that you describe and the usage of this API this way should indeed result into a linear revisioning and the passed entity is the merged revision, while the current default revision is the parent. In this case I completely agree with you and we should update \Drupal\Core\Entity\ContentEntityStorageBase::createRevision() to set the merged revision on the new entity object. However in this case we merge only a single translation but currently we are able to only flag the whole entity as merged. I was thinking that we might use the revision_translation_affected flag, but I am not sure if this is sufficient, because there might be no changes on a translation and then the flag will remain unset for that translation or if we merge non-translatable fields, then even if merging only one translation all the translations will have the revision_translation_affected flag set. Do we also need a boolean translatable merged_langcode field? Any thoughts on that? I think this issue is CNW because of this. Does this convince you that the patch is not ready for 8.7.x?

Summary:
There are two use cases mentioned here:
1. Loading an entity in an older revision and saving at as a new one, which I think should result into having as parent the old revision.
2. Reverting only a single translation, where the parent should be the current default revision and the entity revision used for the revert should be the merged revision.
--------------------------

A side note:
We've just found a bug in our project and a way to recover from it might be to create some intermediate revisions...

This would be easy if on save the entity receives as parent the currently loaded revision. Otherwise one has to explicitly set the parent, which requires a dedicated knowledge how the parent is being chosen - and I think this knowledge now is limited to only the people participating in the current issue and even with a CR I am not sure that this knowledge is able to propagate. Considering this fact in isolation regardless of everything else convinces me as well that it is much more secure that the parent is always the previously loaded entity object, which in the case of \Drupal\node\Form\NodeRevisionRevertForm::prepareRevertedRevision() will be the loaded revision and in the case of \Drupal\node\Form\NodeRevisionRevertTranslationForm::prepareRevertedRevision() will be the default revision.

Actually instead of introducing one more field, we could make the merged_revision field translatable - something that we are able to do thanks to it being a separate field and not only a property of an another field.

In order to keep it a revision metadata key, we would have to adjust \Drupal\Core\Entity\Sql\DefaultTableMapping::create() to allow translatable revision metadata keys to be put into the data tables. I think that this change is fine, but it also might be considered a BC break if someone has defined a revision metadata field(s) to be translatable, because after such a change the generated schema will change. We could tackle this by explicitly checking only for that field and allowing only it to be translatable.

While I still personally don't have a strong preference on two vs. one fields, I'm tempted to go with #127 - it does not actually introduce ERR, and the field is more agnostic.

I spoke to amateescu and plach separately about the revision parent strategy at the moment, because I had another alternative idea, and they both explained something I hadn't thought of before, which I think is a strong argument for keeping it as it is:

plach: those of us arguing for a linear parent history are thinking about the parent field as a change history for merging purposes. those who are arguing for setting parent to 'currently loaded revision ID' are talking about a process history for auditing_purposes. For me, if a content auditing trail is desired that could be an additional field that always tells you the loaded revision ID regardless of any other branching strategy. I'm not sure that both approaches can be reconciled in a single field, which might explain the deadlock. Or we might find out they can later on in which case it will get resolved by the follow-ups anyway.

The main reason to keep the linear revision history in the patch though, is that until we decide exactly how parallel revisions might work, it ensures the following:

1. Site updates from 8.6.8 to 8.7.0
2. Site updates form 8.6.8 to 8.7.8

Let's assume these are the same site, with identical content changes, and the only difference is when the code update happens, then the revision history they'll end up is identical with the current approach. This is a consistent basis to work out everything else from, it means that everything up until we have a fully defined parenting strategy will be the same on all sites (unless they start to set the parent manually themselves, in which case they made a decision).

The patch needs a re-roll after the menu link conversion so CNW for that.