Entity validation does not always prevent concurrent editing of pending revisions

Posting a very rough patch (with all sorts of hacks and workarounds) allowing me to satisfy all the use cases I posted at:

https://docs.google.com/spreadsheets/d/1nlQLx5BzXyx1aIB_8ISfPGqSnUPH07YU...

Actually without the revision translation merging logic being implemented at #2860097: Ensure that content translations can be moderated independently this allows to do very nasty stuff, so I think it would make more sense to close this issue and merge the two patches. Posting the patch anyway to check whether bot is happy.

Setting to NR for the bot, but this is heavily NW :)

Fixed stupid thing

Another stupid fix (still heavily NW :)

Main things to address:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -127,6 +127,24 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $current_revision_id = !$entity_type->isRevisionable() ? array() : $this->entityTypeManager
   

   We need an API to retrieve the latest entity revision id, currently we have it in the ModerationInformation service, but we need something at entity subsystem level.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -127,6 +127,24 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form['current_revision'] = [
   

   We need a proper name for this and probably we should add it only for revisionable entity types, nbd.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -177,6 +195,14 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +      $entity->original = $this->entityTypeManager
   

   Currently we are abusing the the $entity->original property, which kind of makes sense, except it doesn't because it doesn't contain the original revision but the original *latest* revision.

   We need to find a way to pass this information along so it's available to the validation code.

   Another property, e.g. $entity->latestRevision, is the simplest solution that comes to my mind but it's not a very clean API.

I spoke with @catch about #8:

• (1) We're fine with introducing an utility class with no interface just wrapping common entity queries. The class would be initially marked as @internal and later, when it's properly shaped up, it would be published as @api.
• (3) We're fine with initially using an @internal magic property, e.g. $entity->originalLatestRevisionId or something like that. We will open a follow-up to introduce a service to track request-scoped entity metadata via a KV interface, details to be discussed. E.g. how to deal with nested requests and information setting/overwriting/merging.

As I mentioned above this should be eventually merged into #2860097: Ensure that content translations can be moderated independently as it allows to edit revisions in a way that, without the proper revision translation merging introduced there, may cause (apparent) data-loss.

Addressed the outstanding issue from #8, hopefully I didn't break anything. The next step should be to add test coverage. Then we should be able to merge this into #2860097: Ensure that content translations can be moderated independently or we could remove the parts that make translation merging required and add them back there along with test coverage.

Setting to NR for the bot.

+++ b/core/lib/Drupal/Core/Entity/Query/Queries.php
@@ -0,0 +1,102 @@
+  public static function get(ContainerInterface $container = NULL) {
+    if (!isset(static::$instance) || isset($container)) {
+      if (!isset($container)) {
+        $container = \Drupal::getContainer();
+      }
+      static::$instance = static::create($container);
+    }
+    return static::$instance;
+  }

You don't really need a singleton. You can achieve the same having that as a service in the container or even have just static methods.

@timmillwood:

This will only work if the entity is saved via a form.

Well, not exactly, although you'd have to manually populate the field as you do in the form. Alternatively we could populate it during hook_load(), but it would be an additional query for each load, since that would need to bypass entity cache.

How about checking the changed time of the revision loaded with the revision ID $entity->getLoadedRevisionId(), and the $entity?

I'm pretty sure I tried that in one of my innumerable experiments, but I will now add test coverage for the typical scenarios I used for testing so we can check whether that works.

@dawehner:

You don't really need a singleton. You can achieve the same having that as a service in the container or even have just static methods.

Static methods I'd like to avoid to allow for proper DI. I went for the singleton because this will be @internal until we discover more about its typical usage, and it becomes clearer who should be responsible for instantiating it. In the meantime it doesn't affect signatures and it's still unit testable. You're right that we can use a service instead of a singleton, though.

In #11 I was suggesting to merge this back into #2860097: Ensure that content translations can be moderated independently, however the originally reported bug has nothing to do with translations, so I decide to limit the changes to only those required to fix the original issue and iterate over the current solution in #2860097: Ensure that content translations can be moderated independently.

The attached patch should be more or less ready to go, I think. I'm not attaching an interdiff because it's almost a complete rewrite wrt the previous one.

This should fix the two failures: this new approach uncovered a bug in the logic of ::setNewRevision().

The other failure is caused by a problematic test that was trying to validate a past revision, which is not supposed to be stored as-is. For now I commented that part. I don't think we are losing test coverage, since the the immediately following assertions are covering the same use case with the latest revision, that is the correct case. I added a todo to re-evaluate that code in #2860097: Ensure that content translations can be moderated independently, since it's dealing with multilingual, but unless I'm missing something, I think it could be simply removed. I'll get in touch with Tim tomorrow to talk about that.

Fixed typo

Slightly adjusted the previous fix

Ok, reviews welcome

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -713,6 +713,15 @@ public function onChange($name) {
   +        if ($key == 'revision') {
   

   🙃 Let's use triple equal

2. +++ b/core/lib/Drupal/Core/Entity/Query/Queries.php
   @@ -0,0 +1,75 @@
   +class Queries {
   ...
   +   *   The latest revision identifier or NULL if no revision could be found.
   +   */
   +  public function getLatestRevisionId(EntityInterface $entity) {
   +    $entity_type = $entity->getEntityType();
   +    if (!$entity instanceof RevisionableInterface || $entity->isNew() || !$entity_type->isRevisionable()) {
   

   Isn't that exactly what the entity repository is about?

3. +++ b/core/modules/content_moderation/tests/src/Kernel/EntityStateChangeValidationTest.php
   @@ -173,6 +173,7 @@ public function testInvalidStateMultilingual() {
   +    /** @var \Drupal\node\NodeInterface $node */
        $node = Node::create([
   
   @@ -181,6 +182,7 @@ public function testInvalidStateMultilingual() {
    
   +    /** @var \Drupal\node\NodeInterface $node_fr */
        $node_fr = $node->addTranslation('fr');
   

   🙃 These changes seems to be a bit out of scope here

Thanks!

Isn't that exactly what the entity repository is about?

I was not aware about its existence, honestly. From what I can tell, it seems more focused on instantiating entities rather than querying them, however I could see it being a candidate for these query methods. The main problem is there is no base class for EntityRepositoryInterface, so we are not allowed to add methods to EntityRepository.

Discussed this in IRC with @timmillwood: we're getting rid of the Queries class for now. We will get back to that topic once #2864995: Allow entity query to query the latest revision is sorted out.

Regarding #26.1 I pointed out :

How can we query the latest revision without being sure the entity type is revisionable?
I mean, I can try the timestamp approach but it would require an entity type that implements the EntityChangedInterface whereas a simple Revisionable entity type is supported currently.

Wrong interdiff, sorry.

I think it's worth waiting a bit for #2864995: Allow entity query to query the latest revision which is RTBC and will add a clean way of querying for the latest revision :)

Also, tests were added by @plach so I'm removing that tag.

#2864995: Allow entity query to query the latest revision just got in so we can use it here!

Done :)

@plach, nice and fast! Now here's an actual review of the patch :)

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -713,6 +713,15 @@ public function onChange($name) {
   +          $revision_id = $this->getRevisionId();
   +          if ($revision_id == $this->getLoadedRevisionId()) {
   

   Is there a reason for assigning the $revision_id variable and then just use it once in the check below?

   Can't this be rewritten to if ($this->getRevisionId() == $this->getLoadedRevisionId())?

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -127,6 +127,17 @@ public function form(array $form, FormStateInterface $form_state) {
   +    // Store the revision id at the time the form is built, so it can be used
   

   revision ID :)

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -127,6 +127,17 @@ public function form(array $form, FormStateInterface $form_state) {
   +      $revision_key = $entity_type->getKey('revision');
   

   Same as above, we're not using the $revision_key variable anywhere else so we can simply use call the method below.

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -127,6 +127,17 @@ public function form(array $form, FormStateInterface $form_state) {
   +        '#default_value' => $this->entity->getRevisionId(),
   
   @@ -155,6 +166,14 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +    // We did not save the entity yet, so we expect the loaded revision id to
   +    // match the revision id. However this may not be the case if the entity was
   +    // saved in another form. Thus we need to make sure the loaded revision id
   +    // actually matches the revision id.
   +    if ($this->entity->getEntityType()->isRevisionable()) {
   +      $entity->updateLoadedRevisionId();
   +    }
   
   +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -14,17 +17,58 @@ class EntityChangedConstraintValidator extends ConstraintValidator {
   +        $latest_revision_id = $this->getLatestRevisionId($entity);
   +        $loaded_revision_id = $entity->getLoadedRevisionId();
   +        $valid = $loaded_revision_id == $latest_revision_id;
   

   I'm not sure where we use the revision ID set as a hidden element in the entity form, since the constraint validator only looks at the loaded revision ID and the latest revision ID..

   A bigger point here is that we're probably missing some dedicated test coverage with actual entity forms being manipulated and submitted at independently of each other.

Addressed #33.

1: Fixed (it's easier to debug if the result is stored in a variable :)
2: Fixed.
3: Not fixed, I think it reads better the current way.
4: By storing the revision ID as an (hidden) form element, we know which value it had when the form was instantiated. This allows to compare it with the latest revision ID in the validator. We are using the loaded revision ID there (which is updated while building the entity with the revision id in the hidden field) because a call to ::setNewRevision() will set the revision ID to NULL. Good point about the test coverage, fixed.

Small additional clean-up

1: Nope, the idea is that both SAVE_NEW_REVISION and SAVE_PENDING_REVISION satisfy that condition
2: That's precisely the SAVE_NEW_REVISION case :)

Maybe the following code would be more explicit?

    if ($type & static::SAVE_NEW_REVISION) {
      $entity->setNewRevision();
      $entity->isDefaultRevision($type !== static::SAVE_PENDING_REVISION);
    }

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -155,6 +166,14 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +    // We did not save the entity yet, so we expect the loaded revision id to
   +    // match the revision id. However this may not be the case if the entity was
   +    // saved in another form. Thus we need to make sure the loaded revision id
   +    // actually matches the revision id.
   
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityLoadedRevisionTest.php
   @@ -167,4 +167,39 @@ public function testSaveInHookEntityInsert() {
   +   * Tests manual revert of the revision id value.
   ...
   +    // Check that revision id field is reset while the loaded revision id is
   ...
   +    // to the revision id field works as expected.
   

   Supernit: s/id/ID/

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -14,17 +17,58 @@ class EntityChangedConstraintValidator extends ConstraintValidator {
   +        // If the entity is revisionable, we need to make sure that no new
   +        // revision was created while the entity was being changed.
   

   ❓ Hm, this is effectively checking against a race condition. But what if two requests are simultaneously executing this code? Then there's still the chance for a race condition, isn't there?

3. +++ b/core/modules/content_moderation/tests/modules/content_moderation_test/content_moderation_test.module
   @@ -0,0 +1,20 @@
   +/**
   + * Implements hook_form_FORM_ID_alter().
   + *
   + * {@inheritdoc}
   + */
   

   Nit: The "inheritdoc" seems out of place here?

4. +++ b/core/modules/content_moderation/tests/src/Kernel/EntityStateChangeValidationTest.php
   @@ -199,14 +199,16 @@ public function testInvalidStateMultilingual() {
   +    // From the latest french revision, there should be no violation.
   

   Supernit: s/french/French/

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityConcurrentRevisionSaveTest.php
   @@ -0,0 +1,122 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   

   Supernit: can be just "inheritdoc"?

6. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityConcurrentRevisionSaveTest.php
   @@ -0,0 +1,122 @@
   +    if ($type & static::SAVE_NEW_REVISION) {
   

   👍 Bitwise AND operator. This is correct/intentional, but since it's so rare, it tripped me up at first :P

7. ❓ Last but not least: the reason I started reviewing this issue: the API-First impact. Since this is implementing a validation constraint, we have every reason to believe this will also work correctly/as expected via the REST API (or JSON API or GraphQL), right?

1: fixed
2: Of course: probably we should be using locking in the storage handler to secure ourselves against race conditions. However not even that would be 100% safe, because PHP provides nothing like Java's synchronized methods. That said, the issue here is about fixing the existing logic, so I think implementing anything else would be out of scope.
3: Actually PHP Storm stops complaining about arguments not being documented with that. Unfortunately it's not so smart yet to actually use the documentation available in .api.php files. One man can dream though ;)
4: fixed
5: fixed
6: yeah, I'm starting to have doubts about that code: it was meant to be more readable that way :D
7: I hope so: we moved from an approach that could work in programmatic workflows with some tweaks to one that should be totally transparent. If you instantiate an entity and save a draft and another client tries to save a draft at the same time, the revision id checks should make validation fail. This should be extensively covered by the kernel test.

Fixed a few more occurrences of "id" and "french".

#42.2: should we have a follow-up for it, or do you think it's just not realistic?

#42.7: 👍

#42.2: should we have a follow-up for it, or do you think it's just not realistic?

I think a followup would be totally fine, I guess that would need to be addressed when implementing some kind of conflict management in core.

Re #34.4:

We are using the loaded revision ID there (which is updated while building the entity with the revision id in the hidden field) because a call to ::setNewRevision() will set the revision ID to NULL.

Ahh, I see, this was the part that I was missing. The explanation makes sense, thanks :)

+++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
@@ -525,4 +527,41 @@ public function testWorkflowInUse() {
+    $revision_id = $this->getLatestRevisionId($node);

+++ b/core/modules/content_moderation/tests/src/Functional/ModerationStateTestBase.php
@@ -151,4 +152,26 @@ protected function grantUserPermissionToCreateContentOfType(AccountInterface $ac
+  protected function getLatestRevisionId(ContentEntityInterface $entity) {

Since this is a content moderation test class, you can use \Drupal\content_moderation\ModerationInformation::getLatestRevisionId().

Also, a small plug for #2918569: Simplify ModerationInformation::getLatestRevisionId() :)

Rrright!

Cool, no other observations from me. RTBC++

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -14,17 +17,58 @@ class EntityChangedConstraintValidator extends ConstraintValidator {
   +        // revision was created while the entity was being changed.
   

   "was being changed" vs "was being edited"?

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -14,17 +17,58 @@ class EntityChangedConstraintValidator extends ConstraintValidator {
   +        $loaded_revision_id = $entity->getLoadedRevisionId();
   ...
   +  protected function getLatestRevisionId(EntityInterface $entity) {
   

   Couple of lines underneath we have to load the unchanged entity, therefore I wonder if we really need the new method or could just retrieve the revision ID from the unchanged entity? Excuse me if this has been already discussed, I didn't manage to find out why the new method has been introduced instead of using the unchanged entity.

3. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -14,17 +17,58 @@ class EntityChangedConstraintValidator extends ConstraintValidator {
   +    $id_key = $entity_type->getKey('id');
   

   Do we need to assign the result to a variable which is used only once?

I would like to use the current thread to point to an issue, the fix of which would make unnecessary the addition of the changed timestamp and the revision ID as a hidden value to the form - #2824293: Inconsistent form build between initial form generation and the first ajax request.

Regarding #49.2:
Retrieving the revision ID through an entity query would be a performance optimization instead of retrieving the unchanged entity in the case where the the entity has been saved meanwhile with a new revision.
If the entity has not been saved with a new revision meanwhile or has not been saved at all meanwhile then we will always perform both an entity query for retrieving the latest revision ID and load the unchanged entity.
I have nothing against an entity query, I even do it as an optimazation for this constraint validator in #2837707: EntityChangedConstraintValidator should be retrieving the latest changed time through an entity query instead by loading the unchanged entity. I am however against the case where we will both do an entity query and entity load, therefore I am putting back the status to needs work.

What would be better is to have is a solution where we either retrieve the changed timestamp and the revision ID from the unchanged entity object or we retrieve them both through an entity query, but not a mixed solution which is more expensive in the case the entity has not been saved meanwhile

What would be better is to have is a solution where we either retrieve the changed timestamp and the revision ID from the unchanged entity object or we retrieve them both through an entity query

The unchanged entity won't provide us the latest revision id (yet), however it will after #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types, and that will also save us the additional entity query. With this change, the timestamp check is only required when concurrently editing the same revision.

I am happy to get rid of these checks altogether, if #2824293: Inconsistent form build between initial form generation and the first ajax request can find a solution that works also in programmatic workflows, but meanwhile let's get this in to unblock progress in #2860097: Ensure that content translations can be moderated independently.

The testbot is failing due to an infrastructure issue, moving to NR since the latest changes were minor enough to be harmless.

Here's what's happening: #2919773: Fail to download drupal/coder because github.com/klausi/coder has gone missing

@plach, I am sorry, I think I've misunderstood the issue by thinking we are not preventing the use case where an entity has been saved in a new revision without any field value changes, which case is being solved by this issue as well.

The issue summary also states:

This is not really correct, because if two users are editing a draft revision, and one user saves before the other, we'd want to throw the 'The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved.' violation message.

The change however affects not only drafts being saved, but normal entity revisions as well.

With the current change we will not be able to edit entities if they currently have forward revisions? If this is true, then this is BC break because forward revisions were possible before the concept of drafts came into core and additionally I do not really understand the need for drafts anymore. If we disallow saving an entity if it contains drafts, then what is the difference to locking the entity through e.g. using the content_lock module?

With the current change we will not be able to edit entities if they currently have forward revisions?

We are always editing the latest revision, even when pending revisions are involved, so this should be the intended behavior AFAICT. Once the latest revision is loaded into the edit form (or instantiated via a programmatic workflow), validation will always succeed.

We are always editing the latest revision, even when pending revisions are involved

I don't think that this is true. We might have forward revisions for an entity, but when we visit entity_type/{entity}/edit the entity will be loaded in its default revision, which is not the latest revision if forward revisions are present.

Conceptually we should alway be editing the latest revision. In core currently, if Content Moderation is not installed, the default revision is always the latest revision. If CM is enabled the latest revision is always loaded in the entity form. We also have an issue to make sure we always load the latest revision: #2865616: Add an option for EntityConverter to load the latest entity revision and fix all entity forms to use this option..

That being said, unless some custom code is attempting to edit and save a past revision, which is not supported, there should be no BC issue.

That being said, unless some custom code is attempting to edit and save a past revision, which is not supported, there should be no BC issue.

But there might be custom cases, which are using forward revisions. By doing this change in the validatior we will break them by introducing logic which should be only available for entities using content moderation. I think we should add an another constraint with this new logic through a hook only for entities being handled by content moderation instead of introducing a BC break.

@hchonov, editing a forward revision which is *not* the latest revision is a data-loss scenario (i.e. a critical bug), because the forward revision that is being edited will become the new latest revision, and the data from the previous latest revision will never see the light of day :)

So I see this change as one that is enforcing the correct *default* behavior for editing revisionable entities (i.e. the behavior that prevents data loss by default), and custom code which may want to allow editing a pending revision which is not also the latest revision can simply opt out of this default constraint provided by core.

@hchonov, editing a forward revision which is *not* the latest revision is a data-loss scenario (i.e. a critical bug), because the forward revision that is being edited will become the new latest revision, and the data from the previous latest revision will never see the light of day :)

I am aware of this problem, but it could be solved. We could flag the type of the revision e.g. default or draft and not relly on a draft being the latest. We could do then entity_load(5, 'draft');.

I am sorry, but I am not entirely convinced by this change, because in our software we were thinking already of employing forward revisions.
One of the places where I wanted to experiment with is the autosave_form module where instead of writing to a custom storage I could create a forward revision something just like drafts, but I would need multiple autosave revisions by multiple users, which means the latest revision is only for a certain user and not the one that other users will be editing, because multiple users would have multiple forward autosave revisions. But if you do this change in core you do it only to solve the problem of content moderation and force any another approaches using forward revisions to adjust or to completely stop working. This change will make e.g. autosave revisions impossible.

If at some point we introduce the concept of revision type then I could imagine a lot of usages of forward revisions. Having said this I would like if this constraint only applies to default revisions, otherwise it is BC break from my POV.

@hchonov the use case you describe in #64 with the autosave_form module is quite an advanced flow for an entity form and it will need exactly the same validation code that's being added by this patch (i.e. prevent saving if the user is not editing the latest revision), with the only difference that the latest revision check needs to be per user. And saying that it is impossible is kind of exaggerating a bit, modules that want to provide advanced editing workflows like that simply need to alter the EntityChanged constraint with one that is suited for their needs.

Since we don't have different revision types in core, I stand by what I said in #62 that this patch provides the correct behavior for current HEAD, which is to prevent data-loss by default.

@hchonov the use case you describe in #64 with the autosave_form module is quite an advanced flow for an entity form and it will need exactly the same validation code that's being added by this patch (i.e. prevent saving if the user is not editing the latest revision), with the only difference that the latest revision check needs to be per user. And saying that it is impossible is kind of exaggerating a bit, modules that want to provide advanced editing workflows like that simply need to alter the EntityChanged constraint with one that is suited for their needs.

@amateescu, no, it will not be possible for an user to save a default revision if the entity already has an autosave forward revision and we don't want to prevent others from editing just because there are autosave revisions, for this case we have the conflict module. Imagine an user goes to Node/edit and edits default revision 3, because of the changes on the page there are 2 autosave forward revisions created 4 and 5. Now another user goes to that same edit form and wants to save it, but that is impossible because of the new constraint logic.

You keep saying that it's impossible for the autosave_form module to work after this change, do I really need to mention for the third time that it simply needs to replace the EntityChanged constraint added by core?

I don't understand why should custom and contrib adjust and replace core constraints, but not content moderation, which is the one currently needing that change? Why could we not consider a solution to first make the change isolated and only available to content moderation?

@hchonov:

Would it alleviate your concerns if we got back to comparing the latest revision id at the time the entity was instantiated with the current latest revision id? This would still allow us to detect concurrent edits without making assumptions on which revision was loaded in the edit form.

I've also answered this a few times already. Because core needs to provide a solution which prevents data loss by default. Yes, this problem is surfaced by Content Moderation in core, but so were other critical bugs which had to be fixed in their respective modules and not in Content Moderation. These are the most prominent examples that come to mind right now:

#2858431: Book storage and UI is not revision aware, changes to drafts leak into live site
#2856363: Path alias changes for draft revisions immediately leak into live site
#2858434: Menu changes from node form leak into live site when creating draft revision

@plach, would that approach have any drawbacks compared to the latest patch?

Well, it would have to wait for #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types to be implemented cleanly, that is in a way that doesn't force programmatic workflows to perform additional steps (see #11).

Hm, I think this is at least major.

@amateescu, if it is needed to give an example, I could also refer to a major bug, that had impact on data integrity, but the fix of which was reverted just because we changed a behaviour and a contrib module broke. Then it took another half an year until we agreed on a solution without introducing a regression. This is where I've learned, that even if we wish and insist on a behaviour being correct or wrong we're not allowed to just change it without thinking on the possible consequences of doing this and forcing somebody to just adjust to the new behaviour.

Because core needs to provide a solution which prevents data loss by default.

I agree that this could be considered as data loss, but then only from the POV of content moderation. We could not declare the content moderation use case the default one just like this. We should not think of content moderation being the one and only one making use of forward revisions.
The problem we have is that we have different POVs how forward revisions could be used. From the perspective of content moderation it should not be possible to save an entity if it is loaded in the default revision but has forward revisions. From the perspective of a possible autosave solution it should be possible to save an entity loaded in the default revision even if it has forward (autosave) revisions. As there might be different use cases each requiring a different behaviour, why should we at all define a default behaviour that then has to be overwritten by each module using forward revisions?

Would it alleviate your concerns if we got back to comparing the latest revision id at the time the entity was instantiated with the current latest revision id? This would still allow us to detect concurrent edits without making assumptions on which revision was loaded in the edit form.

@plach, I don't really understand this approach. Could you please describe it in more details?

@hchonov:

See #11.

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
@@ -14,15 +17,36 @@ class EntityChangedConstraintValidator extends ConstraintValidator {
+        // If the entity is revisionable, we need to make sure that no new
+        // revision was created while the entity was being changed.
+        $latest_revision_id = Queries::get()->getLatestRevisionId($entity);
+        $valid = $entity->original_latest_revision_id == $latest_revision_id;

@plach, #11 will also prevent an user to save a default entity revision if an autosave forward revision was created in the meanwhile by another user.

Unfortunately I see only two available options here:

• either declare the content moderation use case as the default one and let the entity changed constraint validator prevent saving if a forward revision was created in the meanwhile and force custom and contrib to adapt by replacing the core constraint
• or define a new constraint in content moderation which prevents saving if a forward revision was created in the meanwhile, but is isolated only to entities covered by content moderation

No matter what the decision would be I think that we need a framework or release manager review for that.

I spoke with @hchonov in IRC, we agreed to leave the final word to a committer.

@plach, #11 will also prevent an user to save a default entity revision if an autosave forward revision was created in the meanwhile by another user.

That's correct. @hchonov explained me that the reason why autosave is not concerned about concurrent edits is that it works with the conflict module out of the box.
IMO our ultimate goal should be to bring a (basic) conflict management solution in core and get rid of this validator, however I still think the solution proposed in #11 (comparing the latest revision at the time the entity was instantiated, with the one at validation time) would be a reasonable behavior without conflict management available.

Aside from that, we agreed we see three possible ways forward here:

• Declare the CM use case as the default one and let the entity changed constraint validator prevent saving if a pending revision was created in the meantime. This may force custom and contrib code to adapt by replacing the core constraint.
• Define a new constraint in CM preventing concurrent edits if a forward revision was created in the meantime. However this would apply only to entities handled via CM.
• Add revision validation as a separate validator, as part of the core entity system, but activate it through CM. This would allow other systems that need a similar behavior for pending revisions not to depend on CM and not require contrib/custom modules to change their default behavior, unless CM is enabled. This would probably trickier for CM to handle as it would be harder to selectively apply the constraint only to moderated entities. Maybe CM could provide a validator subclass dealing with that.

The main outcome of our discussion was that neither of us is aware of an official definition of the "pending revision" concept and what their default behavior is supposed to be. This lack of clarity may end up affecting the contrib/custom space regardless of the solution we pick above, but we should try to minimize the impact of these choices. A discussion on this topic, unless it has already taken place, would help in avoiding the risk of making some changes in the current behavior, just to revert them or change them again, if we find other problems in the core entity system or in CM.

Catching up on this. One thing that hasn't been discussed is that workspaces theoretically at least allows you to have multiple latest-pending-revisions. Take the example of workspace 1 changing an image and workspace 2 adding a French translation on the same entity. This ends up with the same conflict resolution issues that autosave has to deal with.

However a first implementation of workspaces is likely to need the same constraint added here, restricting things one entity can only be in one draft/pending workspace at a time - until we have that conflict management in core at least.

I'm not sure where that leaves things on [i]this[/i] issue yet.

@timmillwood: I think this one would be first given that the presence of this bug prevent the merge from happening cleanly in #2860097: Ensure that content translations can be moderated independently, right?

Another way to think about workspaces is there is only ever one "latest pending revision", but it is possible to create a new revision from any other revision.

One more reason to go back to #11 then :)

We still need release manager review and framework manager review because of all the discussions we had about the BC break changes the patch is introducing.

@catch is both a framework and a release manager, so #79 is a start at those things. :)

I read through the issue comments, and found #62, #68, and #78 particularly interesting, so thank you for those. In trying to think of a way to address all those concerns, what do you all think of following the single responsibility principle, by which I mean:

• Leave EntityChangedConstraintValidator as it is in HEAD: its responsibility is solely to ensure that the entity itself (meaning its default revision) hasn't changed while the user has been editing.
• Add a new constraint, EntityNoNewPendingRevisionsConstraint, to implement something along the lines of #11.
• Automatically set this constraint either for all revisionable entity types, or only if EntityChanged is also set. The latter is a bit of a weird coupling, but a semi-reasonable one, since allowing a save when there are new pending revisions means that the EntityChanged "problem" is punted to a later time when one of the revisions needs to become the default one.
• If we want to enforce that you can only begin editing from the latest revision, ala #51, then I think we should have yet a 3rd constraint for that: EntityNoLaterPendingRevisionsConstraint, since that seems like yet another responsibility. However, per #83, I'm not sure we want such a constraint, or at least I don't think we need it to solve the scope of this issue, do we?
• As to #68, I think it is reasonable to make removing the EntityNoNewPendingRevisionsConstraint the responsibility of conflict module, or something like it. Since it is only through the capability of such a module that the constraint can be relaxed while still satisfying the spirit of EntityChanged when later advancing the default revision.

@timmillwood:

#2860097: Ensure that content translations can be moderated independently has now a patch that includes a fix for this, more or less #11. So I think we should got back to #11 and adjust that solution with whatever committers suggest to do to move forward.

@effulgentsia:

Thanks for your feedback! #86 seems like a reasonable way forward to me.

If we want to enforce that you can only begin editing from the latest revision, ala #51, then I think we should have yet a 3rd constraint for that: EntityNoLaterPendingRevisionsConstraint, since that seems like yet another responsibility. However, per #83, I'm not sure we want such a constraint, or at least I don't think we need it to solve the scope of this issue, do we?

Yep, after experimenting heavily with #2860097: Ensure that content translations can be moderated independently, I think we would need to turn #51 to something similar to #11 anyway over there. Actually this was more or less my plan from the beginning (see #16), as I thought #51 would be less controversial and cleaner to implement as a first step :D

@effulgentsia:

I just realized that splitting the validator into two separate ones may not be possible because timestamp validation needs to be skipped in some cases, depending on the revisions being checked.

timestamp validation needs to be skipped in some cases, depending on the revisions being checked

What's an example of that?

What's an example of that?

In case of multilingual entities we may loading a translation in the edit form that's not the latest revision, because other translations may have been edited after that, so newer revisions exist. In that case it can happen that the loaded timestamp is lowed than the default revision timestamp, but this is not signalling a concurrent edit.

Can we update the issue summary to describe what happens when the drafts are concurrently edited and what the result is, both with CM and HEAD? Specific STR and actual vs. expected behavior would help. Plus let's retitle the issue to describe the bug rather than a proposed change in an implementation. That information will help us weigh in on what direction we should go here.

This patch relies on #2926483: Add API methods for determining whether an entity object is the latest (translation-affecting) revision and restores an approach similar to #11. I'll update the issue summary ASAP. Attached you can find also a patch to review.

Split off the following fix that was originally included in #51: #2928778: Exception when trying to save a new revision after manually setting the original revision ID.

Small improvement

Retitled and updated IS

Still working on the IS.

Completed the IS with the description of the current status.

Here's an updated patch based on the latest conversations I had with @amateescu, @catch, @hchonov and @timmillwood.

This is allowing the revision validation logic to be enabled per bundle, via a bundle info key. Content moderation enables that only for moderated bundles.

Rerolled combined patch.

Added test coverage for multilingual pending revision validation. This should be ready for review now.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -331,6 +331,26 @@ public function isDefaultRevision($new_value = NULL) {
   +  public function isLatestRevision() {
   +    /** @var \Drupal\Core\Entity\ContentEntityStorageInterface $storage */
   +    $storage = $this->entityTypeManager()->getStorage($this->getEntityTypeId());
   +
   +    return $this->getLoadedRevisionId() == $storage->getLatestRevisionId($this->id());
   
   +++ b/core/lib/Drupal/Core/Entity/RevisionableInterface.php
   @@ -51,6 +51,14 @@ public function getRevisionId();
   +  public function isLatestRevision();
   

   While I understand that this method is added for convenience I am not sure that I like the idea of duplication. The method for retrieving the latest revision ID lives in the storage so such a method could live in the storage as well e.g. - \Drupal\Core\Entity\RevisionableStorageInterface::isLatestRevision(RevisionableInterface $entity). One could argue that we have \Drupal\Core\Entity\RevisionableInterface::isDefaultRevision(), which lives on the entity class but it is not entirely correct because both methods should be actually performing on call entity queries instead on entity load, which is being done correctly by ::isLatestRevision(), but not yet by ::isDefaultRevision().

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -331,6 +331,26 @@ public function isDefaultRevision($new_value = NULL) {
   +  public function isLatestTranslationAffectingRevision() {
   +    /** @var \Drupal\Core\Entity\ContentEntityStorageInterface $storage */
   +    $storage = $this->entityTypeManager()->getStorage($this->getEntityTypeId());
   ...
   +    return $this->getLoadedRevisionId() == $storage->getLatestTranslationAffectingRevisionId($this->id(), $this->language()->getId());
   +  }
   
   +++ b/core/lib/Drupal/Core/Entity/TranslatableRevisionableInterface.php
   @@ -7,6 +7,15 @@
   +  public function isLatestTranslationAffectingRevision();
   

   Same as the previous remark - I think this method belongs in the storage.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -155,6 +181,15 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +      //   in https://www.drupal.org/project/drupal/issues/2784201.
   
   +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -10,21 +13,108 @@
   +      if (isset($entity->original_latest_revision_id)) {
   +        // If the entity is revisionable, we need to make sure that no new
   +        // revision was created while the entity was being changed.
   +        // @todo Check the "latest_revision_id" field once we have introduced it
   +        //   in https://www.drupal.org/project/drupal/issues/2784201.
   +        $valid = $entity->original_latest_revision_id == $storage->getLatestRevisionId($entity->id());
   ...
   +  protected function isRevisionValidationEnabled(EntityInterface $entity) {
   +    $bundle_name = $entity->bundle();
   +    $bundle_info = \Drupal::service('entity_type.bundle.info')
   +      ->getBundleInfo($entity->getEntityTypeId());
   +    return !empty($bundle_info[$bundle_name]['entity_validation_constraints.EntityChanged.changed_revision']);
   

   This kind of reminds me of the rule of three, according to which a code should be generalised as soon as there are three places where it is used - otherwise it is a premature refactoring which is not necessary correct. As content moderation is currently the only one example needing such a logic I still think that it should be introducing its own constraint instead of inserting this logic in the default core constraint and making it much more complex - there is a critical that is not yet committed but currently is making the logic already harder to understand. In our call we've decided to postpone to revision type field because we don't have enough examples requiring it and I think the same applies for this constraint as well.

1. & 2. are actually not that relevant. 3. is the important point.

High-level review from API-First Initiative POV.

+++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
@@ -530,4 +533,43 @@ public function testWorkflowInUse() {
+    $revision_id = $node_storage->getLatestRevisionId($node->id());
...
+    // Simulate a concurrent draft post: we expect to see a validation error in
+    // this case.
+    \Drupal::state()->set('content_moderation_test.concurrent_edit', 1);
+    $edit = [
+      'body[0][value]' => 'Third version of the content.',
+      'moderation_state[0][state]' => 'draft',
+    ];
+    $this->drupalPostForm($edit_path, $edit, t('Save'));

Since we can't test 2 open tabs this content_moderation_test_form_node_moderated_content_edit_form_alter to change the revision id stored in the form.

Would it closer to actual conditions to:

1. $this->drupalGet($edit_path);
2. Do $node->save()
3. $revision_id = $node_storage->getLatestRevisionId($node->id());
4. Use $this->submitForm() to submit the form that was created in the drupalGet() instead of $this->drupalPostForm()
5. Do the asserts

Then we won't need the content_moderation_test module at all right?

In our call we've decided to postpone to revision type field because we don't have enough examples requiring it and I think the same applies for this constraint as well.

I'm sorry but I have to disagree. In this case we have a strong use case in core, so the situation is completely different. Additionally this solution is following the approach we discussed during the call: we decided to go with a logic available to any contrib/custom module, without needing to depend on Content Moderation, but activated by Content Moderation. At least this was my understanding and this is what the patch is doing. I tried to make activation as little intrusive as possible, without introducing a full-fledged API exactly to allow to iterate on this approach. We can get rid of that and make CM extend the revision validator to apply it only to CM-enabled entities, but doing more than that would mean going too far IMO. The fact that we are willing to support different ways to use pending revisions does not mean that the way pending revision are used in CM is a minor use case, in fact I'd say it's quite the opposite. I think this a sensible default behavior and forcing code willing to use the same logic to depend on CM would be an unneeded requirement IMO.

Even the ".review" patch in #104 no longer applies, due to a conflict with #2837022: Concurrently editing two translations of a node may result in data loss for non-translatable fields. An updated patch here would help with reviewing.

Rerolled

The IS says Three possible ways forward were discussed so far: and points to a bunch of comments, but doesn't say which solution got consensus. Further down in the IS: Find an accepted solution: two committers provided their feedback so far in #79 and #86, but there was no consensus yet on a way forward. — but we're now ~30 comments further. Do we have consensus now?

And which of the three possible solutions does the current patch implement? AFAICT it's solution 3.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -139,6 +141,30 @@ public function form(array $form, FormStateInterface $form_state) {
   +   * Adds an hidden input field storing the (current) latest revision ID.
   

   Nit: s/an/a/

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -139,6 +141,30 @@ public function form(array $form, FormStateInterface $form_state) {
   +   *   internal. They will be replaced by a regular field in
   +   *   https://www.drupal.org/project/drupal/issues/2784201.
   
   @@ -155,6 +181,15 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +      // @todo Update the "latest_revision_id" field once we have introduced it
   +      //   in https://www.drupal.org/project/drupal/issues/2784201.
   
   +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -10,34 +13,121 @@
   +        // @todo Check the "latest_revision_id" field once we have introduced it
   +        //   in https://www.drupal.org/project/drupal/issues/2784201.
   

   Note that the latest direction in #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types actually is NOT adding a field anymore. Which would make these comments inaccurate. Perhaps better to simply state "@todo revise when lands"?

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -139,6 +141,30 @@ public function form(array $form, FormStateInterface $form_state) {
   +  protected function addLatestRevisionIdWidget(array& $form) {
   

   Nit: s/array&/array &/

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -139,6 +141,30 @@ public function form(array $form, FormStateInterface $form_state) {
   +    // Store the latest revision id at the time the form is built, so it can be
   

   Nit: s/id/ID/

5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -139,6 +141,30 @@ public function form(array $form, FormStateInterface $form_state) {
   +    if ($entity_type->isRevisionable()) {
   
   +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -10,34 +13,121 @@
   +    if ($entity instanceof RevisionableInterface && $this->isRevisionValidationEnabled($entity)) {
   ...
   +      if (isset($entity->original_latest_revision_id)) {
   

   These if-conditions do not match. This is super confusing.

   AFAIK it's not valid to do instanceof RevisionableInterface, one must do $entity_type->isRevisionable()?

   Also, shouldn't all three be triggered only when the bundle has this new constraint?

6. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -139,6 +141,30 @@ public function form(array $form, FormStateInterface $form_state) {
   +      $form['original_latest_revision_id'] = [
   +        '#type' => 'hidden',
   +        '#default_value' => $storage->getLatestRevisionId($this->entity->id()),
   +      ];
   

   Why can't we store this in form state storage?

7. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -155,6 +181,15 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +    // Ensure the original latest revision identifier is available for
   

   Nit: s/identifier/ID/

8. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -10,34 +13,121 @@
   +   * Flag determining whether changed time validation should be performed.
   

   This sentence is hard to parse.

   An @see \Drupal\Core\Entity\EntityChangedInterface could help.

9. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -10,34 +13,121 @@
   +      $valid = $this->validateChangedRevisions($entity);
   +      if ($this->checkChangedTime) {
   +        $valid = $this->validateChangedTime($entity);
   +      }
   

   This is confusing. If $this->checkChangedTime is set, then we'll overwrite $valid. Should they be AND'ed or OR'd? If not, let's do an if/else.

10. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
    @@ -10,34 +13,121 @@
    +   * Performs validation based on revision changes.
    ...
    +  protected function validateChangedRevisions(EntityInterface $entity) {
    

    The function name says that it's going to validate changed revisions.

    The docblock says it's going to do validation (of what?) based on revision changes.

    It's similar, but not the same.

11. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
    @@ -10,34 +13,121 @@
    +    $valid = TRUE;
    

    Nit: s/$valid/$is_valid/

12. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
    @@ -10,34 +13,121 @@
    +        $valid = $entity->original_latest_revision_id == $storage->getLatestRevisionId($entity->id());
    ...
    +        $this->checkChangedTime = $valid && $entity->original_latest_revision_id == $entity->getLoadedRevisionId();
    

    === is preferred.

13. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
    @@ -10,34 +13,121 @@
    +    return !empty($bundle_info[$bundle_name]['entity_validation_constraints.EntityChanged.changed_revision']);
    

    This is only enabled by Content Moderation for now. It's fine that this logic lives in core, outside content_moderation, because as the IS says: that allows other modules to reuse this behavior.

    But I don't understand why we need to update the existing constraint validator. Especially if we're adding a new constraint, which is what is happening here.

    Such a new constraint could also get a much clearer name: SequentialPendingRevisionConstraint(Validator).

After some additional manual testing in #2860097: Ensure that content translations can be moderated independently, I verified that this no longer triggers a validation error when dealing with pending multilingual revisions, due to the revision merge strategy introduced in #2924724: Add an API to create a new revision correctly handling multilingual pending revisions. This allowed me to implement the solution @effulgentsia suggested in #86: implementing two separate constraint validators. He also suggested in Slack a very neat way to register validators only for specific bundles, which allowed me to get rid of all that bundle info crap. This should now be way cleaner and address @hchonov's concerns, while at the same time allow this logic to be reused without depending on Content Moderation.

@Wim Leers, #113:

6: Because if the form is not cached it will be retrieved again on submit. We need to make sure that we get the latest revision ID when the form was originally displayed. This is the same strategy we are using for the changed time stamp.
12: Not with revision ids ;)
13: See above for an explanation on why we were not introducing a separate constraint/validator. Great name suggestion, btw, I slightly changed it because the validation itself does not differentiate between default and pending revisions: it would prevent concurrent editing even if the changed timestamp constraint were disabled, as long as new revisions are created on every save.

Everything else is fixed or did not apply anymore.

The interdiff is completely useless ;)

Reviewing my own code:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -139,6 +142,33 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $has_revision_changed_constraint = (bool) array_filter(
   ...
   +    if ($has_revision_changed_constraint) {
   

   Outdated variable name.

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/SequentialEntityRevisionCreationConstraintValidator.php
   @@ -0,0 +1,66 @@
   +    // @todo Check the "latest_revision_id" field once we have introduced it
   +    //   in https://www.drupal.org/project/drupal/issues/2784201.
   
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityConcurrentRevisionSaveTest.php
   @@ -0,0 +1,229 @@
   +      // @todo Remove this line once the "latest_revision_id" field is introduced
   +      //   in https://www.drupal.org/project/drupal/issues/2784201.
   ...
   +    // @todo Remove this line once the "latest_revision_id" field is introduced
   +    //   in https://www.drupal.org/project/drupal/issues/2784201.
   

   These are still mentioning the new field.

Updated the IS.

Just nits, really — this patch is much clearer! I don't feel qualified to RTBC this patch though.

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/SequentialEntityRevisionCreationConstraint.php
   @@ -0,0 +1,20 @@
   + * Validation constraint for the entity revision changes.
   ...
   + *   label = @Translation("Sequential entity revision creation", context = "Validation"),
   ...
   +class SequentialEntityRevisionCreationConstraint extends Constraint {
   

   Description, label, name don't match.

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/SequentialEntityRevisionCreationConstraintValidator.php
   @@ -0,0 +1,65 @@
   +class SequentialEntityRevisionCreationConstraintValidator extends ConstraintValidator implements ContainerInjectionInterface {
   ...
   +   * EntityRevisionChangedConstraintValidator constructor.
   

   Mismatch.

3. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -257,6 +257,25 @@ function content_moderation_entity_bundle_info_alter(&$bundles) {
   +function content_moderation_data_type_info_alter(&$data_types) {
   +  /** @var \Drupal\workflows\WorkflowInterface $workflow */
   +  foreach (Workflow::loadMultipleByType('content_moderation') as $workflow) {
   +    /** @var \Drupal\content_moderation\Plugin\WorkflowType\ContentModeration $plugin */
   +    $plugin = $workflow->getTypePlugin();
   +    foreach ($plugin->getEntityTypes() as $entity_type_id) {
   +      foreach ($plugin->getBundlesForEntityType($entity_type_id) as $bundle_id) {
   +        $key = 'entity:' . $entity_type_id . ':' . $bundle_id;
   +        if (isset($data_types[$key])) {
   +          $data_types[$key]['constraints']['SequentialEntityRevisionCreation'] = NULL;
   +        }
   +      }
   +    }
   +  }
   +}
   

   This could use a very brief description of what it actually does. Because it touches many abstraction layers.

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityConcurrentRevisionSaveTest.php
   @@ -0,0 +1,227 @@
   + * @coversDefaultClass \Drupal\Core\Entity\Plugin\Validation\Constraint\EntityRevisionChangedConstraintValidator
   

   That's not the validator class it tests.

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityConcurrentRevisionSaveTest.php
   @@ -0,0 +1,227 @@
   +      'pending revision - pending revision' => [[static::SAVE_PENDING_REVISION, static::SAVE_PENDING_REVISION]],
   ...
   +   * @param int[] $type
   +   *   An array of save types as defined by the related ::SAVE_* constants. One
   ...
   +  public function testConcurrentSave(array $type) {
   

   Why an array, if 100% of them contain 2 values? Then why not do a $from, $to or $before, $after or something like that?

This should fix test failures and address #119.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -127,6 +128,8 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $this->addLatestRevisionIdWidget($form);
   
   @@ -139,6 +142,33 @@ public function form(array $form, FormStateInterface $form_state) {
   +   * @internal This widget and the related property should be considered
   +   *   internal. This will be revisited once https://www.drupal.org/node/2784201
   +   *   is committed.
   ...
   +  protected function addLatestRevisionIdWidget(array &$form) {
   

   Calling this hidden field a widget is a bit misleading, how about addLatestRevisionIdFormField()?

2. +++ b/core/modules/content_moderation/tests/modules/content_moderation_test/content_moderation_test.module
   @@ -0,0 +1,24 @@
   + * @file Test module for Content Moderation.
   

   I think the description needs to go on the next line, after @file :)

3. +++ b/core/modules/content_moderation/tests/modules/content_moderation_test/content_moderation_test.module
   @@ -0,0 +1,24 @@
   + * {@inheritdoc}
   

   No need for {@inheritdoc} in hook implementations.

4. +++ b/core/modules/content_moderation/tests/modules/content_moderation_test/content_moderation_test.module
   @@ -0,0 +1,24 @@
   +    /** @var \Drupal\Core\Entity\ContentEntityForm $form_object */
   

   You could use the interface here.

5. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
   @@ -530,4 +532,43 @@ public function testWorkflowInUse() {
   +    $edit_path = sprintf('node/%d/edit', $node->id());
   

   No need for sprintf(), 'node/' . $node->id() . '/edit' works just fine and it's more clear :)

6. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
   @@ -530,4 +532,43 @@ public function testWorkflowInUse() {
   +    $this->drupalPostForm($edit_path, $edit, t('Save'));
   ...
   +    $this->drupalPostForm($edit_path, $edit, t('Save'));
   

   No need to use t() in tests.

7. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
   @@ -530,4 +532,43 @@ public function testWorkflowInUse() {
   +      ->responseContains((new SequentialEntityRevisionCreationConstraint())->message);
   

   I'm not sure this is valid PHP 5 syntax, but why don't we copy the message text in the assertion? That's what we usually do in other places..

8. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ConcurrentEntityRevisionSaveTest.php
   @@ -0,0 +1,230 @@
   +  const SAVE_NO_REVISION = 0x0;
   +  const SAVE_NEW_REVISION = 0x1;
   +  const SAVE_PENDING_REVISION = 0x3;
   

   Anything wrong with using 0, 1 and 2 as the constant values? :)

9. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityKernelTestBase.php
   @@ -26,6 +26,13 @@
   +  protected $bundleInfo;
   
   @@ -45,6 +52,7 @@ protected function setUp() {
   +    $this->bundleInfo = $this->container->get('entity_type.bundle.info');
   
   @@ -194,4 +202,12 @@ protected function generateRandomEntityId($string = FALSE) {
   +  protected function enableEntityTestTranslation() {
   

   Is this really needed in the base test class?

Thanks!

1-7: Fixed
8: Those wouldn't allow me to do fancy bitwise things in ::doEntitySave() ;)
9: Well, entity_test.translation is a generic state key supported in entity_test, so I thought it would be more useful to make it an "API".

+++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
@@ -530,4 +531,43 @@ public function testWorkflowInUse() {
+    $this->drupalPostForm($edit_url, $edit, t('Save'));

Missed a spot :)

Basically I think that it is fine now, but I would rather move the CM logic to the CM module.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -127,6 +128,8 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $this->addLatestRevisionIdFormField($form);
   
   @@ -139,6 +142,33 @@ public function form(array $form, FormStateInterface $form_state) {
   +  protected function addLatestRevisionIdFormField(array &$form) {
   
   @@ -155,6 +185,13 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +      $entity->original_latest_revision_id = $original_latest_revision_id;
   

   If we don't declare this as the default behavior, then I am not sure if this code should be placed here instead in a hook_form_alter implementation in the content moderation module. Then we could use an entity builder to add the value to the entity when the form is submitted.

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/SequentialEntityRevisionCreationConstraint.php
   @@ -0,0 +1,20 @@
   +namespace Drupal\Core\Entity\Plugin\Validation\Constraint;
   
   +++ b/core/modules/content_moderation/content_moderation.module
   @@ -257,6 +257,27 @@ function content_moderation_entity_bundle_info_alter(&$bundles) {
   +          $data_types[$key]['constraints']['SequentialEntityRevisionCreation'] = NULL;
   

   As the new constraint is only added by content moderation probably it should be placed in the content moderation module instead in core?

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ConcurrentEntityRevisionSaveTest.php
   @@ -0,0 +1,230 @@
   +namespace Drupal\KernelTests\Core\Entity;
   ...
   +class ConcurrentEntityRevisionSaveTest extends EntityKernelTestBase {
   

   If this test remains in core and is not moved to the content moderation module, which I think would be better until we have defined the way to go, then I think that we should move the test to \Drupal\KernelTests\Core\Entity\EntityValidationTest, where we already have a test for concurrent editing - ::testEntityChangedConstraintOnConcurrentMultilingualEditing().

Side note: as actually only CM is the one that currently needs those changes then probably the right component would be "content moderation" instead the "entity system"?

I tested patch in #123

When I apply that patch I'm not able to concurrently add two drafts in two different languages. I'm also not able to concurrently edit two translations in two different languages even though I only edit translatable fields. Is that how it is suppose to work?

I find it a little bit strange that I'm constrained to work concurrently in two different language version of a node.

@hchonov:

1/2: I believe in #2940575: Document the scope and purpose of pending revisions we clarified that sequential revision creation is not CM-specific, so it should be ok for it to live in the core Entity system.
3: Well, EntityValidationTest does not deal with revisions, so merging the tests would force to add a lot of logic in the setup phase. On top of that SequentialEntityRevisionCreationValidationTest focuses on the sequential revision logic, the multilingual logic is not the main use case under test.

@matsbla:

I will check whether it is possible to allow concurrent editing of different translations.

This is a data integrity issue, so it should be considered critical.

@plach:

Any updates on the data integrity issue? We have the same problem as matsbla.

Just closed a duplicate of this: #3091890: Publishing multiple translations of the same node at the same time leads to data loss

Patch fails to apply.

Re-rolled for 9.2.x

Removed the "core" key from content_moderation_test.info.yml file.

Are we seriously thinking about blocking the edition as a long term solution for this problem? There's the content_lock module for that. It cannot be accepted as a core solution.
My company is composed of multiple translators working on the same nodes, sometime at the same time, and I cannot imagine to defend Drupal solution with that kind of behavior.
IMHO It should be ok to edit different language on the same node with no problem.

1. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ConcurrentEntityRevisionSaveTest.php
   @@ -0,0 +1,230 @@
   +  const SAVE_NO_REVISION = 0x0;
   +  const SAVE_NEW_REVISION = 0x1;
   +  const SAVE_PENDING_REVISION = 0x3;
   ...
   +    if ($type & static::SAVE_NEW_REVISION) {
   

   It is great the maker of this show off his/her programming skills by using hexadecimal numbers, only it does not make the test easy to understand. Can we remove the hexadecimal numbers and adhere to the KISS principle.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ConcurrentEntityRevisionSaveTest.php
   @@ -0,0 +1,230 @@
   +    $this->assertEquals($expected_violation_count, $violations->count(), 'Wrong violations count with constraints');
   

   Could we change this to testing if one of the violations is the one we expect.

3. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
   @@ -557,4 +558,45 @@ public function testWorkflowInUse() {
   +  public function testConcurrentDraftEdit() {
   

   Would it be possible to add a second test like this with a multilingual entity?