Concurrently editing two translations of a node may result in data loss for non-translatable fields

Lost input is not critical.

It's fine that you disagree but you would have to justify why this is critical according to the guidelines.

The passage you quoted is about non-bugs and the critical priority is "at the core maintainers' discretion" in that case. You submitted a detailed bug report and I don't like arguing in comments. Do what you feel is right and we can see what others think. I could be wrong.

edited

Editing different translations of the same node is not supported, the only bug here is that it should not be possible and you should get a validation error if someone else saved. See discussion in #2465909: Implement per-language locking at entity form level.

Hi, everyone!

While working on various concurrent content editing issues (in most cases with translations) like:
- #2808335: Changed time not updated when only non-translatable fields are changed on a translated entity
- this one

I've used patch from here https://www.drupal.org/node/2808335#comment-11929474 and got this data

while debugging EntityChangedConstraintValidator::validate + EntityChangedTrait::getChangedTimeAcrossTranslations,
we will see on the screenshot that en translation of the updated entity($saved_entity) has higher date than en translation of being updating entity($entity),

so the question is why we use max() instead of min() to find a right "changed" candidate?

I've did a quick patch on top of 2808335 an also did manual tests with both patches on simplytestme,
and it seems that it fixes this problem:

I am confused as in this case the EntityChangedConstraint should have failed and you shouldn't be allowed to save the other translations, but it looks like the constraint is not failing during your manual test

Issue #2808335: Changed time not updated when only non-translatable fields are changed on a translated entity was resolved and patches were commited to the 8.3/4.x versions.
So lets try run tests for patch on 8.3.x

Change issue description to be more generic.

Ok, the thing is that when you don't make "changed" field translatable then entity that is saving has the same changed value for all translations,
but when you'll set "changed" field to be translatable then saving entity has also same changed values for all translation except one - current, it has lower value!

Despite of the fix above works for validation case, but it might broke entity saving cases, at least breaks tests now)

Then I've came up with:
EntityChangedConstraintValidator uses EntityChangedTrait::getChangedTimeAcrossTranslations method to distinguish changes between original entity and current.
But EntityChangedTrait::getChangedTimeAcrossTranslations uses max function inside to find out latest value for changed field across all translations (according to the function doc block)
Despite of above it sounds good in terms when entity already validated and being saving, but on other side it's unacceptable to use it at validation step.

I propose to use "sum" strategy to detect changes across all translations in two entities inside EntityChangedConstraintValidator and don't use getChangedTimeAcrossTranslations at all.

Use right path.

Discussed with @plach, @Berdir, @tstoeckler, and @amateescu discussed this issue and agreed it's a critical issue. While lost user input itself would only be major, in this case, the data that was saved is deleted/overwritten, especially when revisioning is not enabled. So in this case it's actually lost data that can lead to integrity problems. The validation API is not preventing this, when it should.

Per discussion, we are not sure if we should support changing the translatability of the changed field.

@xjm, thanks for youк attention to this problem.

But I really worry about:

...we are not sure if we should support changing the translatability of the changed field.

As I see all patches has long life in core issues before they will be commited, so I worry about end users and their data.
Do you know when you'll deprecate support for "changing the translatability of the changed field"?

My patch works correctly both for (un)/translated changed field, but the way of fixing constraint seems for me redundant in case when you want to drop off support of the translated changed field.
As I see I leave this patch just here for people who needs it till you'll release a new Drupal version w/o translation support of the changed field.

BTW: I've almost finished a functional test for that case.

The entity changed constraint validator works together with the node form where we set as an input the current changed timestamp, so that even if the entity gets a new timestamp meanwhile on building the entity we'll map on it the changed timestamp from the user input and then no matter if we're dealing with a modified translatable or non-translable field the entity changed constraint validator should fail the validation. Having said this I am not sure how does it happen that the constraint validator isn't failing? Is it possible that it fails but for some reason the error message is filtered away? Could you please debug the use case and post the timestamps being compared in the entity changed constraint validator when you're saving the second translation?

@hchonov. Thanks for participating.

Having said this I am not sure how does it happen that the constraint validator isn't failing?

This is a reproducible bug, see "Steps to reproduce" section.

For the asked timestamps look at my screenshot:

But at your screenshot I see the validation failing message? If we see this message then we have no bug. We have a bug only if the second translation gets saved without validation errors being shown to the user preventing the saving.

This message appeared because I've manually fixed some code to check my ideas, so if you want I can add another screenshot without my fixes and with outputted timestamps.

I've looked at the constraint once more and I get it now where the error happens :
if ($saved_entity && $saved_entity->getChangedTimeAcrossTranslations() > $entity->getChangedTimeAcrossTranslations()) {..}

The problem is that we build the entity from the user input and apply the changed timestamp only for the current language, but as the entity is not yet cached as part of the form cache it is reloaded with new timestamps on the other translations ..

Possible solutions:
1. Provide input fields for all changed timestamps of all translations
2. Instead of directly using getChangedTimeAcrossTranslations first compare the changed timestamp of the entity in its current translation with the timestamp of the original in the current translation of the entity - here we have to get the translation entity object. If this comparison doesn't fail then continue to the usual check as of now.
3. Solve #2824293: Inconsistent form build between initial form generation and the first ajax request.

I would go with 2 as the fastest solution here.

I suggest the following :

if ($saved_entity && (($saved_entity->getChangedTime() > $entity->getChangedTime()) || ($saved_entity->getChangedTimeAcrossTranslations() > $entity->getChangedTimeAcrossTranslations()))) {..}

This will solve the current issue but we still need a test. A kernel test showing that the entity changed constraint is failing will be sufficient.

@hchonov, just checked:
> I suggest the following
And this solution doesn't work for me.

Also I want to say that I've tried to create kernel test (using as base EntityKernelTestBase) but I have no luck with it, I mean test can't test this case, entity was built with other changed timestamps than via admin interface, did I missed something?

$reloaded = EntityTestMulChanged::load($entity->id());
$rel_translation = clone EntityTestMulChanged::load($entity->id())->getTranslation('uk');
$rel_translation->set('translatable', 'Привiт');

$reloaded->set('translatable', 'Hello1');
$reloaded->save();

$errors = $this->getErrorsForEntity($rel_translation);
....
function getErrorsForEntity(...) {
...
  $form_object = $form_state->getFormObject();
  $form_object
      ->setEntity($entity)
      ->setFormDisplay($display, $form_state)
      ->validateForm($form, $form_state);

  return $form_state->getErrors();
}

The framework and release managers discussed this issue a couple weeks ago and agreed it was critical because of the data integrity/data loss issue.

Marking NW for the test coverage. In terms of how to test it, if the bug only happens with a browser, one possibility is to add a sleep(1) to ensure the timestamp rolls over. We do this in a few places in tests already for changed functionality, e.g. NodeFormSaveChangedTimeTest.

I have been trying to reproduce this bug but not able to. Followed the steps to reproduce it and I am on the same branch mentioned in the issue.
Does the issue still exists? Did anyone face this recently?

Ok, I am able to reproduce the bug now.

After applying the patch same problem while saving Taxonomy by editing two different translation same time.

Here is my try with this test. I've haven't had a success. Maybe someone knows what I missed or could make some changes into it.

I've updated the EntityChangedConstraintValidator the way I've proposed in #29 and provided a test which shows that it works.

@matsbla, could you please describe step by step how I can reproduce the problem?

After looking deeper at the issue my conclusion is that the main problem described by the issue is fixed. Yay! :).

The problem described by @matsbla in #41 might sound similar but is of completely different character. The problem is that content_translation allows for making only specific field properties translatable or not translatable and syncs the non-translatable properties in hook_entity_presave(), which means content_translation will be updating a field after the check for changes by ChangedItem::preSave() has run, because the entity field preSave methods are executed before the presave hooks have run. I've created a separate issue for this problem - #2912318: Changing field value in an entity presave hook will not update the entity changed timestamp where I've provided an initial patch, which is not perfect, but should fix the problem. @matsbla would you please test the patch from the other issue? Thank you.

I am re-uploading the patch that is solving the problem of the current issue. I've updated the comment in the patch as well. As there is already a test coverage I am removing the needs tests tag.

A related issue is #2824293: Inconsistent form build between initial form generation and the first ajax request, which probably will solve the problem as well when ready, but it is a much harder fix to get in than the current simple solution of the latest patch in the current Issue.

The fix looks good, to me, however I found the comments a bit hard to parse. I'll try to suggest something clearer and add commas and points here and there to to pretend I can use punctuation :)

@plach, I've refactored the comments and I hope that they are more easy to understand now :)

I got back to this the other day and briefly discussed it with @hchonov as, after thinking a bit more about it, I had some doubts.

Basically I think that the current fix is correct but not enough. For a form workflow it's correct, but I think that we should check that no translated value of the changed timestamp is less then any translation of the original one. That simply cannot happen unless the entity we are saving was instantiated before the "original" entity we are using for comparison was stored.

Hristo is onboard with this and is planning to get back to this ASAP.

Actually as we've talked I've told you that I am going to respond the very same day, however I didn't managed that. I apologize for this, Francesco.

I had previously the same idea as @plach, but however I was afraid that the patch is going to go beyond the bug fix and therefore I've implemented it only for the scenario where it is failing, because it is critical in the FAPI. But as I have now @plach on my side about a more general solution then this is what I am providing in the current patch :).

Looks good, thanks!

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
@@ -18,10 +18,20 @@ public function validate($entity, Constraint $constraint) {
+        // It is not allowed that there is an entity translation with an older
+        // changed timestamp than the one of the corresponding saved entity
+        // translation. Comparing only the highest changed timestamps across all
+        // translations is not a sufficient check as by doing this we will not
+        // be able to detect the case where only one entity translation is
+        // obsolete.

This comment is not explaining the root cause, what about something like the following?

We need to make sure that no entity translation appears as modified "before" its stored timestamp. This situation can only arise when the entity being saved was instantiated before a subsequent save. This typically happens when two edit forms for the same entity are instantiated concurrently and subsequently submitted without a reload in between. To avoid reverting changes, we mark the entity as invalid.

Is EntityChangedConstraintValidator meant to work for revisions as well? The same issue exists for me if two users are editing an entity, and they both hit save, you get two new revisions so all the data was stored but there is no indication to the user that saved the form last and they can unknowingly promote a revision that doesn't include the other users changes.

@benjy - #2892132: Entity validation does not always prevent concurrent editing of pending revisions is trying to address EntityChangedConstraintValidator issues.

Thanks, that's exactly the issue I was looking for.

I've rewritten the comment and hope that it looks better now.

Looks a bit verbose, but not something for a non-native English speaker to judge. Let's see whether committers are happy.

Agreed that the comment could be a little clearer and less verbose. +1 for documenting it so thoroughly though. I'll see if I can edit it down to make it a little clearer while still keeping all the information.

Alright, how's this? I tried to remove unnecessary words and clarify a few phrases, plus moved the bit about comparing translations individually down into the foreach.

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
@@ -18,10 +18,30 @@ public function validate($entity, Constraint $constraint) {
-        if ($saved_entity && $saved_entity->getChangedTimeAcrossTranslations() > $entity->getChangedTimeAcrossTranslations()) {

If getChangedTimeAcrossTranslations() is this dangerously insufficient, should we be deprecating the method? Or at least updating its docs to warn people when it should not be used?

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
@@ -18,10 +18,30 @@ public function validate($entity, Constraint $constraint) {
-        // A change to any other translation must add a violation to the current
-        // translation because there might be untranslatable shared fields.

We're also losing the bit about untranslatable shared fields; should we add that back?

Sorry for all the serial comments. The comment said that the changed timestamp "might" be added to the form; it wasn't clear to me when or whether that would happen, or if we had to worry about it not being the case. The storage validation shouldn't know about the form, but we could maybe provide an @see to where this happens for entity forms?

Thanks Jess!

1. The storage validation shouldn't know about the form, but we could maybe provide an @see to where this happens for entity forms?

   Yep, that was my main doubt around that comment block: on one hand I understand the will to save the time spent to troubleshoot the symptom to people reading this code (including our future selves ;), OTOH mentioning the form workflow with this level of detail feels a bit out of place and can be misleading for people coming from different use cases, e.g REST workflows. That's why I had suggested something more neutral in #51.

2. The comment said that the changed timestamp "might" be added to the form; it wasn't clear to me when or whether that would happen, or if we had to worry about it not being the case.

     +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
     @@ -18,26 +18,25 @@ public function validate($entity, Constraint $constraint) {
     +        // first, the changed timestamp can be added to the form so that it
     

   Since we are documenting what actually happens, I'd use "is added" here, if we keep this part.

3. If ::getChangedTimeAcrossTranslations() is this dangerously insufficient, should we be deprecating the method? Or at least updating its docs to warn people when it should not be used?

   IIRC this method was explicitly added to deal with this use case, however its logic is correct: it is useful to determine when the last time the entity was modified is, we just didn't realize that logic is not suitable for this check. In fact it is being correctly used in the comment module. That said, I'm not sure we should change its documentation as the implementation is following it closely. Maybe we could add a paragraph saying something along these lines:

   This will return the highest timestamp across all translations, if you need to check that no translation was changed after a certain time, you need to check each translation's timestamp individually.

4. We're also losing the bit about untranslatable shared fields; should we add that back?

   IMO that comment was misleading: even if there are no changed untranslatable fields, concurrently editing two different translations will lead to overwriting one of the two. That is exactly the main use case we are dealing with: preventing data loss due to concurrent edits. I wouldn't stress on untranslatable fields, given that we are already mentioning translations. I'd rather add a couple of lines explicitly mentioning what use case this constraint is meant to address in the class docblock, given it's not explicitly stated anywhere.

Jess++, Francesco++ thank you both :)

Regarding the addition of the changed timestamp to the form:

#64

The comment said that the changed timestamp "might" be added to the form; it wasn't clear to me when or whether that would happen, or if we had to worry about it not being the case.

#65.2

Since we are documenting what actually happens, I'd use "is added" here, if we keep this part.

The correct answer atm is - the changed timestamp might be added :). The reason I choose "might" is that this happens in core only in the NodeForm. I don't know why this logic is not placed in ContentEntityForm, where actually it belongs so that it is available per default for all content entity forms and not only for node forms. This means core offers this protection by default only for node entities. If we write "is added" then we should add the changed timestamp to the form in ContentEntityForm, which unfortunately is out of scope of the current issue.

The storage validation shouldn't know about the form, but we could maybe provide an @see to where this happens for entity forms

Currently - only in \Drupal\node\NodeForm::form().

Regarding ::getChangedTimeAcrossTranslations():
I think that the current documentation on the method is sufficient:

Gets the timestamp of the last entity change across all translations.

and doesn't need any narrow explanation, but I am fine if you both think it should be explained better :).

The correct answer atm is - the changed timestamp might be added :). The reason I choose "might" is that this happens in core only in the NodeForm.

Ah, sorry, I didn't realize that was the case, then "might" is correct. That's a major bug in itself, anyway.

I've just created #2920251: Offer concurrent editing protection for all entities by including the changed timestamp to each entity form. Feel free to raise the status to "major" if you consider it major :).

As suggested by Jess in #64 I've added @see referring to the NodeForm.

As suggested by Francesco in #65 I've extended the ::getChangedTimeAcrossTranslations() documentation.

I hope that this issue gets in before #2920251: Offer concurrent editing protection for all entities by including the changed timestamp to each entity form, where we could exchange

the changed timestamp can be added to the form so that

with

the changed timestamp is added to the form so that

Back to @xjm, let's see what she thinks.

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
@@ -18,10 +18,31 @@ public function validate($entity, Constraint $constraint) {
+        if ($saved_entity) {
+          $common_translation_languages = array_intersect_key($entity->getTranslationLanguages(), $saved_entity->getTranslationLanguages());
+          foreach (array_keys($common_translation_languages) as $langcode) {
+            // Merely comparing the latest changed timestamps across all
+            // translations is not sufficient since other translations may have
+            // been edited and saved in the meanwhile. Therefore, compare the
+            // changed timestamps of each entity translation individually.
+            if ($saved_entity->getTranslation($langcode)->getChangedTime() > $entity->getTranslation($langcode)->getChangedTime()) {
+              $this->context->addViolation($constraint->message);
+              break;
+            }
+          }
         }

One case that could happen and is not covered is the removal of a translation language, which this validation will not catch and add back, no? I am not sure how we could handle that other than purging translations instead of removing them so not un-RTBC-ing for this :)

If this case is handled already, it would be nice to document it.

One case that could happen and is not covered is the removal of a translation language, which this validation will not catch and add back, no? I am not sure how we could handle that other than purging translations instead of removing them so not un-RTBC-ing for this :)

I think we can detect this. Good catch, Gábor!

What if we have as a condition that the unchanged entity has all the translations of the current entity or if it doesn't have a specific translation that then this translation should be flagged as new on the current entity, otherwise fail as we will add a removed translation back? I mean something like this

foreach (array_keys($entity->getTranslationLanguages()) as $langcode) {
  if (!$unchanged->hasTranslation($langcode) && !$entity->getTranslation($langcode)->isNewTranslation()) {
  // Validation fail as the entity object is obsolete.
  }
}

Should we adress this problem in this issue or in a new one?

@Gábor Hojtsy, I think that the current issue is large enough and the problem described here already fixed in the patch. In order to keep it only to that and as it is already RTBC, I've created another issue and posted the initial patch on top of this one there - #2920889: EntityChangedConstraintValidator doesn't take adding, removing and reverting translations into account.

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
@@ -18,10 +18,31 @@ public function validate($entity, Constraint $constraint) {
+        // Ensure that all the entity translations match their current version
+        // in the storage in order to avoid reverting other changes. The entity
+        // object might contain an older entity translation when a translation
+        // is being concurrently edited. If the form that is submitted last
+        // is not yet cached, the entity will be retrieved from the entity
+        // storage on submit, and that entity object will be the updated one
+        // from the previous form submission. To prevent the entity from the
+        // second submission from being saved and overwriting changes from the
+        // first, the changed timestamp can be added to the form so that it
+        // updates the entity object on submit. We use the timestamp to
+        // determine when a previous entity translation version has been edited
+        // and is currently being saved.
+        // @see \Drupal\node\NodeForm::form().
+        if ($saved_entity) {
+          $common_translation_languages = array_intersect_key($entity->getTranslationLanguages(), $saved_entity->getTranslationLanguages());
+          foreach (array_keys($common_translation_languages) as $langcode) {
+            // Merely comparing the latest changed timestamps across all
+            // translations is not sufficient since other translations may have
+            // been edited and saved in the meanwhile. Therefore, compare the
+            // changed timestamps of each entity translation individually.
+            if ($saved_entity->getTranslation($langcode)->getChangedTime() > $entity->getTranslation($langcode)->getChangedTime()) {
+              $this->context->addViolation($constraint->message);
+              break;
+            }
+          }

Another key question here. Do we need this to happen for entities where all fields are translatable? Sounds like an artificial limitation for them without any benefit to me? Why should they not be able to save translation changes concurrently?

@Gábor Hojtsy, this is currently not possible in core, because in that case we have to merge the entities. This is something I've already implemented in the conflict module and by enabling and activating it for an entity type it will automatically perform all the needed merges e.g. revision ID and changed timestamps of concurrently edited translations, even if they have non translatable fields, but no changes made on the non translatable fields :).

The conflict module will ensure that the constraint doesn't fail by properly merging the entities.

Even if the conflict module is enabled we still need the limitations in the constraint, as after it has merged the concurrent edits and before the validation a new concurrent edit might occur.

@hchonov and that is ... because we always save over all translations of the entity even when only editing one of them?

@Gábor Hojtsy, yes exactly because of this.

Ok, I was pondering if we should document that or not, but we don't need to document the whole entity system in this one comment really. All right, since we now have #2920889: EntityChangedConstraintValidator doesn't take adding, removing and reverting translations into account as a separate issue, this sounds fine with me.

A few more points of feedback:

1. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityValidationTest.php
   @@ -200,4 +205,47 @@ public function testCompositeConstraintValidation() {
   +    $this->assertNotEquals($entity->getChangedTime(), $translation->getChangedTime());
   

   For this, it actually matters which is greater, right? So we should be asserting that the later is greater than the former to validate that the test provides the correct coverage?

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -18,10 +18,30 @@ public function validate($entity, Constraint $constraint) {
   +            if ($saved_entity->getTranslation($langcode)->getChangedTime() > $entity->getTranslation($langcode)->getChangedTime()) {
   
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityValidationTest.php
   @@ -200,4 +205,47 @@ public function testCompositeConstraintValidation() {
   +    $this->assertNotEquals($entity->getChangedTime(), $translation->getChangedTime());
   

   For this, it actually matters which is greater, right? So we should be asserting that the later is greater than the former to validate that the test provides the correct coverage?

3. +++ b/core/lib/Drupal/Core/Entity/EntityChangedInterface.php
   @@ -37,6 +37,10 @@ public function setChangedTime($timestamp);
   +   * This method will return the highest timestamp across all translations. If
   +   * you need to check that no translation was changed after a certain time, you
   +   * have to check each translation's timestamp individually.
   

   Makes sense to me, thanks!

4. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -18,10 +18,31 @@ public function validate($entity, Constraint $constraint) {
   +        // object might contain an older entity translation when a translation
   ...
   +          $common_translation_languages = array_intersect_key($entity->getTranslationLanguages(), $saved_entity->getTranslationLanguages());
   +          foreach (array_keys($common_translation_languages) as $langcode) {
   +            // Merely comparing the latest changed timestamps across all
   +            // translations is not sufficient since other translations may have
   +            // been edited and saved in the meanwhile. Therefore, compare the
   +            // changed timestamps of each entity translation individually.
   +            if ($saved_entity->getTranslation($langcode)->getChangedTime() > $entity->getTranslation($langcode)->getChangedTime()) {
   +              $this->context->addViolation($constraint->message);
   +              break;
   +            }
   

   I had another thought: If there's a getChangedTimeAcrossTranslations() method that is reused elsewhere, how about we rename it to getLatestChangedTimeAcrossTranslations() (deprecating the old one) and then move this logic into a getOldestChangedTimeAcrossTranslations()? That'd make the difference really clear. We'd still have to inline it for the backport, but that might be more descriptive and also be useful in other places.

5. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -18,10 +18,31 @@ public function validate($entity, Constraint $constraint) {
   +        // is being concurrently edited. If the form that is submitted last
   +        // is not yet cached, the entity will be retrieved from the entity
   
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityValidationTest.php
   @@ -200,4 +205,47 @@ public function testCompositeConstraintValidation() {
   +    // an uncached form by setting the previous timestamp of an entity
   

   So what does the form being cached or not have to do with it? It seems irrelevant, right? Either the form is cached with the same or an even older timestamp, or it's uncached with the same timestamp?

6. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityValidationTest.php
   @@ -200,4 +205,47 @@ public function testCompositeConstraintValidation() {
   +    // Simulate concurrent form editing by saving the entity with an altered
   +    // non-translatable field in order for the changed timestamp to be updated
   +    // across all entity translations. Afterwards we simulate form submitting of
   ...
   +    // translation on the saved entity object. This is what happens in the
   +    // entity form API where we put the changed timestamp of the entity in a
   +    // form hidden value and then set it on the entity which on form submit is
   +    // loaded from the storage. Setting the initial changed timestamp on the
   +    // entity loaded from the storage is used as a prevention from saving a form
   +    // built with a previous version of the entity and thus reverting changes by
   +    // other users.
   

   This is also a bit long; I start to get confused about halfway through it. To address this, instead of having one long comment, I'd put part of it directly above the specific lines being described.

Thanks!

@xjm thank you for the extended review!

1. Yes. I've changed that.
2. Hmm, 2 is actually 1 or I didn't understand something?
3. :).
4. Done. I've exchanged also all the calls to ::getChangedTimeAcrossTranslations() with ::getLatestChangedTimeAcrossTranslations().
5.

So what does the form being cached or not have to do with it? It seems irrelevant, right? Either the form is cached with the same or an even older timestamp, or it's uncached with the same timestamp?

The problems are caused since #2502785: Remove support for $form_state->setCached() for GET requests, because until that issue got in the forms were cached on their retrieval i.e. form get request and afterwards they are being cached only after the first ajax request e.g. after clicking on "Add another item" on a multiple field. When the form is cached, then the entity is being cached as part of the form and in such case we don't even have to put the changed timestamp to the form, as we'll not load the entity from the storage if the form is cached, but retrieve it from the form object. But if the form is not yet cached and we load the entity from the storage, then we might load a newer version and this is why all the problems arise, as in this case we have to map the changed timestamp from the user input on the entity loaded from the storage, so that the entity changed constraint validator fails. I've already created an issue and am working on it to tackle the problems caused since #2502785: Remove support for $form_state->setCached() for GET requests got in - #2824293: Inconsistent form build between initial form generation and the first ajax request, but it will need more time until it is ready and stable. I hope, that this answers your question, @xjm?

6.

This is also a bit long; I start to get confused about halfway through it. To address this, instead of having one long comment, I'd put part of it directly above the specific lines being described.

I was able to split it in three parts, but not to reduce the content of the comment. If this is still no-go, then help making it clearer and shorter will be much appreciated :).

Looks good to me, but we are still missing the ::getOldestChangedTimeAcrossTranslations() method mentioned in #80.4.

Oh I am sorry, I was going to ask in my previous comment about more details on that method, but I've forgotten. I didn't really understand what for do we need the oldest timestamp across all translations?

Mmh, read #80 again and I think @xjm was referring to encapsulating the logic we have now in the validator, but that's actually comparing two different entity objects so I'm not sure ::getOldestChangedTimeAcrossTranslations() would make sense in that context. Sending back to her to get a clarification ;)

Just a random failure => back to RTBC.

Same

Ah yes, you're right, that doesn't make sense when we're comparing two entity objects. Sorry for the misdirect.

How about a getChangedTimesForAllTranslations() that returns a map of langcode to timestamp? Or oh, how about a method for isAnyTranslationNewerThan($other_entity)? That way we can still encapsulate the logic in a reusable way that will help developers write safer code in similar situations, and attach the documentation to the method rather than an inline wall of text. Then the getLatest...() method docs can say "use isAnyTranslationNewerThan() instead" instead of "you have to check each translation's timestamp individually."

What do you think?

@xjm, on the one hand we have the changes here and on the other we have a follow-up issue that checks for a removed translation and relies on the changes here i.e. iterating over the translation objects - #2920889: EntityChangedConstraintValidator doesn't take adding, removing and reverting translations into account. If we move the logic into a separate method on the entity then we'll have to iterate two times over the translation objects when the follow-up is ready and I think it will be better if we avoid this. Would you please take a look at the latest patch in the referenced issue? I rather see this as a framework internal check and would not make it publicly available - at least not yet as we have also one more issue that might eventually make changes to the constraint validator - #2892132: Entity validation does not always prevent concurrent editing of pending revisions.

Agreed, I'd open a follow-up to explore this idea after all the validation issues are in and dust has settled :)

Here's the follow-up: #2925677: [PP-2] Introduce a ::getChangedTimesForAllTranslations() method.

Alright, I guess it doesn't make sense to block a critical on API improvements especially when there's a dependency chain that will further evolve all this. Was just trying to think of ways to make this all more explicit so we don't introduce similar criticals in the future.

I'll try to look at this again later today.

Thanks!

Alright, so I read over #82 again and I think we should just remove the bit about the form cache from the comment as well as most of the stuff about the form. Our validation logic should not make any assumptions about caching (or even that it's invoked from a form at all). When we implement improvements to the form, we can add docs like the removed ones there.

I also figured out from #84 what we actually need to communicate on the docs for the other method.

Attached implements both those docs changes. NR again to check the accuracy of these changes... (sorry).

+++ b/core/lib/Drupal/Core/Entity/EntityChangedInterface.php
@@ -37,10 +37,32 @@ public function setChangedTime($timestamp);
+  /**
+   * Gets the timestamp of the last entity change across all translations.
+   *
+   * This method will return the highest timestamp across all translations. If
+   * you need to check that no translation was changed after a certain time, you
+   * have to check each translation's timestamp individually.
+   *
+   * @return int
+   *   The timestamp of the last entity save operation across all
+   *   translations.
+   *
+   * @deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0. Use
+   *   \Drupal\Core\Entity\EntityChangedInterface::getLatestChangedTimeAcrossTranslations()
+   *   instead.

Mmmm, we have some docs duplication here. Usually we should make the deprecated docs reference the replacement (as well as the change record which this issue does not yet have). For the same reason as #94 I think we should not do the rename/deprecation yet in this issue and instead add it in (another) followup. That will also allow this fix to be backported to 8.4.x safely.

For #96.

Posted #2926061: Rename EntityChangedInterface::getChangedTimeAcrossTranslations() to getLatestChangedTimeAcrossTranslations() and deprecate the old version as a followup.

I discussed this with @hchonov and @xjm (separately) and we agreed that probably we should remove from comments any reference to the Form API and describe only the generic root cause. This will avoid confusion, since this validator applies also for instance to REST saves. Moving back to RTBC since I just removed a few lines.

The patch is safe to apply 8.5.x, the fix has been stable for many versions now, we've just been bikeshedding over comments :)

It should be safe to apply to 8.4.x too, but obviously some preliminary QA is recommended.

This will avoid confusion, since this validator applies also for instance to REST saves.

And https://www.drupal.org/project/jsonapi and https://www.drupal.org/project/graphql :)

Thanks for helping to make Drupal really API-First!

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -18,10 +18,23 @@ public function validate($entity, Constraint $constraint) {
   +            // Merely comparing the latest changed timestamps across all
   +            // translations is not sufficient since other translations may have
   +            // been edited and saved in the meanwhile. Therefore, compare the
   +            // changed timestamps of each entity translation individually.
   +            if ($saved_entity->getTranslation($langcode)->getChangedTime() > $entity->getTranslation($langcode)->getChangedTime()) {
   

   <3 improved robustness

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityValidationTest.php
   @@ -200,4 +205,49 @@ public function testCompositeConstraintValidation() {
   +   * Tests the EntityChangedConstraintValidator.
   

   Nit: … with multiple translations.

Addressed #102. Restoring the RTBC status as these are clearly testbot issue.

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -18,10 +18,23 @@ public function validate($entity, Constraint $constraint) {
   -          $this->context->addViolation($constraint->message);
   +        // Ensure that all the entity translations are the same as or newer
   +        // than their current version in the storage in order to avoid
   +        // reverting other changes. In fact the entity object that is being
   +        // saved might contain an older entity translation when different
   +        // translations are being concurrently edited.
   +        if ($saved_entity) {
   +          $common_translation_languages = array_intersect_key($entity->getTranslationLanguages(), $saved_entity->getTranslationLanguages());
   +          foreach (array_keys($common_translation_languages) as $langcode) {
   +            // Merely comparing the latest changed timestamps across all
   +            // translations is not sufficient since other translations may have
   +            // been edited and saved in the meanwhile. Therefore, compare the
   +            // changed timestamps of each entity translation individually.
   +            if ($saved_entity->getTranslation($langcode)->getChangedTime() > $entity->getTranslation($langcode)->getChangedTime()) {
   +              $this->context->addViolation($constraint->message);
   +              break;
   +            }
   +          }
   

   At the moment translations are simply removed or added to entities so I don't think we can tell if there's a conflict at all - but should we add a @todo to check for translation deletion/creation if/when we explicitly track those things?

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityValidationTest.php
   @@ -200,4 +205,49 @@ public function testCompositeConstraintValidation() {
   +
   +    // Simulate concurrent form editing by saving the entity with an altered
   +    // non-translatable field in order for the changed timestamp to be updated
   +    // across all entity translations.
   

   If we're removing references to the form/form cache should we also remove them here? On the other hand a concrete example doesn't hurt to explain what's going on in the test, so leaving RTBC.

Re #106.1:
The deletion of translations is the one that could cause problems and this has been brought up by Gábor in #71. Therefore we've created a follow-up issue to cover this problem - #2920889: EntityChangedConstraintValidator doesn't take adding, removing and reverting translations into account.
New translations could not cause any problems as they will not be present on the saved entity object.

Re #106.2:
I would like to leave this comment describing a concurrent editing through the entity form API :). A concrete example makes sense, yes.

I agree with #106.2

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!