EntityChangedConstraint needs to compare changed time of all translations

Didn't check yet but I guess that test assertion is flawed somehow?

Note: The only reason it is possible to edit existing translations in HEAD is because ContentTranslationHandler forced the changed time to REQUEST_TIME, which makes this validation useless as it will be always newer.

The difference to paragraph and I guess also inline_entity_form is that they don't use a translation handler because they're nested within another entity form.

I didn't take a closer look at the paragraphs module yet but I don't think that the core patch is correct.

I assume that you "save" translations without modifying them. In this case the changed timestamp is not changed anymore since #2428795: Translatable entity 'changed' timestamps are not working at all has been added in beta 11. BTW that patch didn't modify the logic of the constraint we're discussing here. I think that bug previously just hided the current problem you face in paragraphs:

// A change to any other translation must add a violation to the current
// translation because there might be untranslatable shared fields.

From my point of view the proposed patch in #2 will break the purpose of the constraint. If you consider that part of the constraint to be wrong, it needs to be removed completely instead of applying a patch like #2.

With #2506213: Update content entity changed timestamp on UI save in mind the "correct" solution here is to not save a translation when it's not modified - or to set the changed timestamp manually.

You assume wrong :)

We *are* saving the translation.. we are editing it in the UI.

The steps to reproduce:

1 Create a node with paragraphs (en)
2. Add a translation (de)
3. Edit the original translation, try to save it. (en)

It is now impossible to save the en version of the paragraph because the changed timestamp of that is older than DE. And we're comparing the en changed with the last across all, which is de, which is of course newer.

You can reproduce this problem easily with any entity:

$node = Node::create();
$node->save();
$translation = $node->getTranslation('de');
$translation->save();

$node = Node::load($node->nid();
$node->validate(); // => Fails

The only reason you can still edit the original translation of a node in the UI is because ContentTranslationHandler completely breaks this validation by always setting changed to REQUEST_TIME.

You can reproduce that problem like this:

After creating a node with a translation, open the edit form in one browser window. Now open it in another (same translation or different, doesn't matter), save it. Then go back to the first browser window and save. This will *not* fail validation, but it is supposed to (compare with not having content translation enabled).

Both things are wrong, but in combination, the second bug is hiding the first.

The reason it fails for paragraphs is that we don't have a content translation handler that is breaking this validation for us.

@berdir:
Thanks for the details. I currently looking at the failing tests ...

ok, I think the patch in #2 is correct.

But the tests fail because of #2513094: ContentEntityBase::getTranslatedField and ContentEntityBase::__clone break field reference to parent entity. Due to that bug calling getChangedTimeAcrossTranslations() before save seems to erroneously "synchronize" the timestamps over all translations.

If you additionally apply the incomplete patch #12 from #2513094: ContentEntityBase::getTranslatedField and ContentEntityBase::__clone break field reference to parent entity, the tests here will pass.

Therefor I postpone this issue and increase the priority of the other one.

#2513094: ContentEntityBase::getTranslatedField and ContentEntityBase::__clone break field reference to parent entity was commited, so this one is no more postponed.

Wondering if it would be easier to get both issues in if we merge this fix into #2506213: Update content entity changed timestamp on UI save, since that arguably provides integration test coverage?

And we have a follow-up to add unit/kernel test coverage and that would not have caught this anyway since we'd have just written the test according to the implementation I guess ;)

@berdir: see https://www.drupal.org/node/2506213#comment-10156042

Both patches have to be separately commited, even if it is easier at once.
We proved in #2506213: Decide how the changed timestamp should behave on UI save , that the EntityChangedConstraint is not functioning properly and with the current patch from this issue everything works as expected.

So I feel right to RTBC this one now.

Still needs tests.

The fix for this problem will be integrated in #2506213: Update content entity changed timestamp on UI save, as we have there enough test coverage showing the faulty EntityChangedConstraintValidator.