Translatable entity 'changed' timestamps are not working at all

8.0.x is still affected. But I'm not sure if it's a bug or a feature. If this issue gets fixed it will probably reduce the complexity of #2453153: Node revisions cannot be reverted per translation.

I had a closer look at the code. The reason why "changed" has always the same value across all languages is that it is the stored value of a Drupal\Core\Field\Plugin\Field\FieldType\ChangedItem. Such a ChangedItem is always set hardcoded to REQUEST_TIME before save:

class ChangedItem extends CreatedItem { 
  public function preSave() {
    parent::preSave();
    $this->value = REQUEST_TIME;
  }
}

When a node is saved, all translations of it get saved, no matter if their content changed or not.

From my understanding such a ChangedItem just indicates that something has been saved, regardless if some values changed for real or not - like a TIMESTAMP column in MySQL. Therefor the current behavior is correct!

But if the current behavior is correct, it makes no sense to declare that field as translatable. I attached a patch to fix that.

If the "changed" field in the database should reflect real changes on a translation, then the field type is incorrect. In this case a TimestampItem has to be used instead of ChangedItem. (It's a little bit confusing that the meaning of "timestamp" in Drupal is inverted compared to MySQL.)

I searched the code for the similar mistakes and extended the patch accordingly. Additionally I extended the descriptions of these fields to (hopefully) clarify their meaning.

I don't think this is the correct direction, although the patch makes sense, for sure it's not how it's supposed to behave for the translation modification date, which is supposed to be always translatable to allow to track the modification date of each translation.

I guess there's a bug in ChangedItem or it's the wrong type as suggested in #3.

I will ask @Berdir to provide some feedback about this.

I'm convinced that the ChangedField is misunderstood - at least by some developers - because It is used for two different use cases.

If you save an entity, SqlContentEntityStorage::saveToSharedTables() iterates over all translations and saves them. It's impossible to save a specific translation on its own. Additionally you can't detect which values have been changed for real. It's possible to load an entity that has five translations, modify some values in two translations and save the entity. In this case it's correct to mark all translations as "dirty" and to invalidate all affected caches for example. This is how instances of ChangedField are currently working in core! One value per revision and de facto non translatable.

The second meaning is, that you want to indicate that a specific translation of an entity has been modified (or let's say edited, because you can't safely detect if it has been modified for real when it gets saved). This feature is not yet used in core but this is what developers actually expect from a translatable instance of ChangedField, like hchonov demonstrated with his bug report and like plach argued in comment #7.

If we don't want to change the nature of entity translation fundamentally we have to deal with both use cases described above. From my point of view we need two separate fields:
one to indicate that an entity has been "changed" in general (non-translatable, revisionable), and a second one to track edits on entity translations (translatable, revisionable).

The second one is definitely needed for future development and needs to be part of the API for contrib modules. I'm already working on that additional field in the context of #2453153: Node revisions cannot be reverted per translation.

Discussed this with @Berdir. We reached more or less the same conclusions outlined in #8: changed needs to be untranslatable to support detecting edit conflicts. However we need a separate field to track the translations modification date.

Additionally you can't detect which values have been changed for real.

I'm not sure why: we have the original entity available to do that.

1. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -134,10 +134,9 @@ public function getFieldDefinitions() {
   -        ->setDescription(t('The Unix timestamp when the translation was most recently saved.'))
   +        ->setDescription(t('The Unix timestamp when the translations were most recently saved, no matter if their content was modified or not.'))
            ->setPropertyConstraints('value', array('EntityChanged' => array()))
   -        ->setRevisionable(TRUE)
   -        ->setTranslatable(TRUE);
   +        ->setRevisionable(TRUE);
   

   This does not work: we need to make this field always translatable and it can't be optional, nor defined only when the Content Translation module is enabled, because the Entity API allows to create translations and revisions programmatically without needing CT. I think we need to have it automatically defined by the Entity Manager, as we do for the default_langcode field. I'd call it translation_changed so it's clear it's not provided by the Content Translation module.

2. +++ b/core/modules/content_translation/src/ContentTranslationMetadataWrapperInterface.php
   @@ -110,10 +110,11 @@ public function getCreatedTime();
   +   * Sets the translation changed timestamp which indicates when the translation
   +   * was last saved.
   

   PHP docs must start with a single-line summary, then a blank line, and only then additional information, if needed (sigh :)

The created field instead should be translatable, so we should change its type, I guess.

As discussed with plach via IRC, here's a different approach to deal with the dualism of the ChangedField.

It's just a proof-of-concept!

The basic idea is to let getChangedTime() act differently depending on the targeted field instance.
If the field is not translateable, getChangedTime() acts like before!

If the filed is translatable, getChangedTime() returns the changed date of the current translation of an entity, like most developers would expect it. But additionally it's possible to get the latest changed date across all translations via getChangedTime(TRUE), which is explicitly required as a functionality at a few places over the core. Using the additional parameter we avoid to introduce new fields or new interfaces because both meanings are accessed via the EntityChangedInterface.

For nodes the changed field in the database now works like expected across translations and revisions.

fixed some stuff from patch #12. But there're issues left caused by NodeForm::validate():
"The content on this page has either been modified by another user, or you have already submitted modifications using this form. As a result, your changes cannot be saved."

But before we dive into that we should discuss the approach in this proof-on-concept in general.

An improved patch. Let's leverage a faster test bot ;-)

Lets see what happens ...

I'm currently fixing the last issue with quickedit. But it would be nice if someone can review the approach I suggested here.

Sorry, I didn't complete my review yet. I will go on tomorrow.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -124,6 +124,16 @@
   +   * An associative array keyed by translation language code. Every value is a
   +   * boolean.
   
   @@ -592,6 +602,32 @@ public function onChange($name) {
   +    return isset($this->dirty[$langcode]);
   

   Be clear that if a key is present, we consider it dirty no matter what value is set.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -124,6 +124,16 @@
   +  protected $dirty = array();
   

   => dirtyTranslations

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -592,6 +602,32 @@ public function onChange($name) {
   +      $this->dirty[$this->activeLangcode] = TRUE;
   

   Please explain in comment why we mark untranslatable fields as dirty translations.

4. +++ b/core/lib/Drupal/Core/Entity/Exception/EntityTranslationChangedException.php
   @@ -0,0 +1,14 @@
   +class EntityTranslationChangedException extends \Exception {
   

   Aha... Seems untested since you used two different names. :-)

5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -28,7 +29,31 @@ class ChangedItem extends CreatedItem {
   +    if ($entity->isNew() || !$this->getFieldDefinition()->isTranslatable()) {
   ...
   +    else {
   ...
   +      if ($this->getFieldDefinition()->isTranslatable()) {
   

   This is always true.

6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -28,7 +29,31 @@ class ChangedItem extends CreatedItem {
   +        if (($entity instanceof ContentEntityInterface)) {
   +          if ($entity->isTranslationDirty($entity->language()->getId())) {
   +            $this->value = REQUEST_TIME;
   ...
   +        else {
   +          // @todo something else for non-ContentEntities?
   +          $this->value = REQUEST_TIME;
   

   Confuses me. Why should this ever be executed for non content entities? Config entities have no fields.

7. +++ b/core/modules/comment/src/CommentStorage.php
   +++ b/core/modules/comment/src/CommentStorageInterface.php
   +  public function getChangedTimeAcrossTranslations(CommentInterface $comment);
   +++ b/core/modules/comment/src/Entity/Comment.php
   +  public function getChangedTime($across_translations = FALSE) {
   +++ b/core/modules/file/src/Entity/File.php
   +  public function getChangedTime($across_translations = TRUE) {
   +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   +  public function getChangedTime($across_translations = TRUE) {
   +++ b/core/modules/node/src/NodeStorage.php
   +++ b/core/modules/node/src/NodeStorageInterface.php
   +  public function getChangedTimeAcrossTranslations(NodeInterface $node);
   

   I'm confused if this all is a general concept, why does it need so much fragmented implementation for all supported content entities?

   I think getChangedTime [AcrossTranslations] should be supported by ContentEntity / ContentEntityInterface.
   Every new entity is now required to reimplement this logic again to support it?
   If this is the way to go, we should unify all applications of the changed concept.

8. +++ b/core/modules/node/src/Entity/Node.php
   @@ -217,7 +217,23 @@ public function setCreatedTime($timestamp) {
   +  public function getChangedTime($across_translations = FALSE) {
   ...
   +        return $this->entityManager()->getStorage($this->getEntityTypeId())
   ...
   +          $changed_tmp = $this->getTranslation($language->getId())->getChangedTime();
   

   I'm confused here, so changed now exists on both entity and translation?
   Is this just because some entities are not translatable at all or are there mixed situations?

Just rebased the patch and resolved conflicts due to the commit of #2429037: Allow adding entity level constraints. Lets see what happens ...

Accidentally uploaded a zero byte patch ;-)

@miro_dietiker: Thanks for your feedback.

1. done
2. done
3. done
4. renamed. test case will follow

5. I don't think so. Wether the field representing the changed timestamp of an entity is translatable or not is up to the entity.

6. I'm just not sure about the possibilities to re-use this code elsewhere or to programatically provide a complete new kind of fieldable entity. Normally this fallback shopuld not be reached in the current state of core. That why I left a todo at his point.

7. The fragmentation itself is not introduced by this patch. It is caused by the fact that all the entity classes implement EntityChangedInterface by themselves. There's no base class implementing that interface in the current state of the core. From my point of view this patch should be kept focused. But you're right that this could be improved. But that should happen in a different patch afterwards.

8. Have a look at the explainations of comment #8 and #12.

Back to the same state we reached with #19 before the latest commits to core. A last issue with quickedit ...

My initial reaction after looking to the latest patches was: "OMG, this is not what we discussed!". In itially I was about to post a comment suggesting to change approach (sorry for that :). I had to read the code more and more before it started growing on me. This is a brief summary of my current thoughts:

Pros

• I'm appealed by the way this approach avoids the need to make the changed field not translatable, and thus requiring an additional translatable field to track translation changes.
• It also minimizes the impact on the API, as the change can be (more or less) BC, given that changed is already marked as translatable in a few cases, and that marking it translatable in the other ones does not imply storage changes.

Cons

• The changed field is not mandatory, nor its translatability, but to make #2453153: Node revisions cannot be reverted per translation work, we do need to be able to store translation modification timestamps. This approach does not help with that, as it is not reliable. However entity type providers could opt in by implementing EntityChangedInterface at any time.
• This approach makes the query to determine when an entity was last modified across all translations less performant. However this can be solved by adding a new untranslatable custom field definition, if needed by one's specific scenario.
• Making ChangedItem work with translatable fields requires very complex logic, moreover we are hard-coding lots of assumptions about our particular use case. OTOH this might not be a big issue in practice, since ChangedItem supports a very specific use-case.

Actually It seems I found less pros than cons, however in my initial attempt to bash the current approach, I was able to find counterarguments for most of them, so basically I made myself change my mind :)

All that said, I still have some concerns that I'd like to see addressed before this is committed:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -124,6 +124,17 @@
   +  protected $dirtyTranslations = array();
   

   Since we might expand the logic tied to this property to track modified field values, I'd stick with just $dirty. I'm also wondering whether it would make sense to make this private, to avoid introducing BC breaks later.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -592,6 +603,44 @@ public function onChange($name) {
   +      // A Translation is marked dirty if the translated value of a translatable
   +      // field changed or is new.
   +      // @todo This detection of modifications is just a proof-of-concept
   +      // and might be far away from being robust.
   

   Since this logic is quite complex and to make it perform reliably we may need some change to the internal field structure, I think for now we could just assume that setting a field means its value has changed. Basically we'd mark the current translation object as dirty whenever a field is set. I'd also ignore field translatability: setting an untranslatable field on a particular translation object should not imply all translations have been modified IMO.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1020,4 +1071,14 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
   +  public function save() {
   

   This is not reliable: save can be triggered also by calling the storage directly, so we should expose a resetChanges() method and call it from there.

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
   @@ -38,4 +38,13 @@
   +  public function isTranslationDirty($langcode = NULL);
   

   The Entity Translation API does not use $langcode parameters, language is implied by the translation object's active language. It would be confusing to add this parameter only here. Moreover dirty is not great terminology for a public API method: what about ::hasChanges()?

5. +++ b/core/lib/Drupal/Core/Entity/EntityChangedInterface.php
   @@ -24,9 +24,12 @@
   +  public function getChangedTime($across_translations = FALSE);
   

   I'm not a fan of this parameter: I think it makes the API a bit confusing, as it introduces a switch to make a translatable field behave as untranslatable. I'd rather have a separate getChangedTimeAcrossTranslations() method, which would allow us to have a common implementation in ContentEntityBase. This way getChangedTime() applies only to the translation object it's invoked on. Additionally this way we wouldn't unnecessarily pollute the content translation metadata wrapper interface.

6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -6,6 +6,7 @@
   +use Drupal\Core\Entity\ContentEntityInterface;
   

   Missing empty line above.

7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -30,7 +31,35 @@ class ChangedItem extends CreatedItem {
   +      // If the timestamp has not been set explicitly auto detect a modification
   +      // of the current translation and set the timestamp if needed.
   +      // An example of setting the timestamp explicitly is
   +      // \Drupal\content_translation\ContentTranslationMetadataWrapperInterface
   

   I'm wondering whether we can simplify this code: I think it's fine to ignore the values passed to the setter when the "changed" property is implemented via a ChangedItem.

8. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -210,7 +211,14 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    if (!$across_translations) {
   +      throw new EntityChangedTranslationException(
   +        sprintf('The changed time of the %s entity type is not translatable.',
   +          $this->getEntityTypeId()
   +        )
   +      );
   +    }
   

   If we remove the parameter we can get rid of this code, the method will return the field value regardless of its translatability. Btw, I'd switch all changed fields to translatable, so contrib modules can get a consistent example from core.

9. +++ b/core/modules/comment/src/CommentStorageInterface.php
   @@ -115,4 +115,15 @@ public function loadThread(EntityInterface $entity, $field_name, $mode, $comment
   +  public function getChangedTimeAcrossTranslations(CommentInterface $comment);
   

   It's not clear to me why we need to query the storage: can't we just iterate over the translation objects and retrieve the latest timestamp?

10. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
    @@ -132,7 +133,14 @@ public function getWeight() {
    +  public function getChangedTime($across_translations = TRUE) {
    
    +++ b/core/modules/node/src/Entity/Node.php
    @@ -217,7 +217,23 @@ public function setCreatedTime($timestamp) {
    +  public function getChangedTime($across_translations = FALSE) {
    

    Another reason to drop this parameter is that having different defaults is bad DX.

11. +++ b/core/modules/node/node.module
    @@ -751,7 +751,14 @@ function node_page_title(NodeInterface $node) {
    -  $changed = \Drupal::entityManager()->getStorage('node')->loadUnchanged($nid)->getChangedTime();
    +  $changed = FALSE;
    +  $node =  \Drupal::entityManager()->getStorage('node')->loadUnchanged($nid);
    +  if ($langcode) {
    +    $changed = $node->getTranslation($langcode)->getChangedTime();
    +  }
    +  else {
    +    $changed = $node->getChangedTime(TRUE);
    +  }
    

    We could simplify this a bit:

    $node = \Drupal::entityManager()->getStorage('node')->loadUnchanged($nid);
    $changed = $langcode ? $node->getTranslation($langcode)->getChangedTime() : $node->getChangedTimeAcrossTranslations();
    return $changed ?: FALSE;
    

12. +++ b/core/modules/node/src/Entity/Node.php
    @@ -217,7 +217,23 @@ public function setCreatedTime($timestamp) {
    +          ->getChangedTimeAcrossTranslations($this);
    

    I'm not sure we should query the storage: IMO when invoked on a revision object getChangedTimeAccrossTranslations() should just iterate over the translations of the current revision, so the code would be exactly the same in both cases (and it could be moved on ContentEntityBase, btw). Am I missing something?

13. +++ b/core/modules/node/src/Entity/Node.php
    @@ -217,7 +217,23 @@ public function setCreatedTime($timestamp) {
    +        $changed = $this->get('changed')->value;
    +        foreach ($this->getTranslationLanguages(FALSE) as $language) {
    +          $changed_tmp = $this->getTranslation($language->getId())->getChangedTime();
    

    This might not always work, as this may not be the default translation object. We can initialize $changed to 0 and iterate over all translations instead. Also (forgive me :), I'm not a fond of tmp variables, can we call it $translation_changed or something along those lines?

14. +++ b/core/modules/node/src/Tests/NodeSaveTest.php
    @@ -130,7 +130,10 @@ function testTimestamps() {
    -    $this->assertNotEqual($node->getChangedTime(), 280299600, 'Updating a node does not use user-set "changed" timestamp.');
    +    // Allowing setting changed timestamps is required see
    +    // Drupal\content_translation\ContentTranslationMetadataWrapper::setChangedTime($timestamp)
    +    // for example
    +    $this->assertEqual($node->getChangedTime(), 280299600, 'Updating a node uses user-set "changed" timestamp.');
    

    As I said above, I'd try to restore the original behavior and fix the content translation wrapper instead.

One more thing, we need to address also the created field: since it's only initialized with the request time, I don't see why it shouldn't be possible to make it work as intended.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -592,6 +603,44 @@ public function onChange($name) {
+      // A Translation is marked dirty if the translated value of a translatable
+      // field changed or is new.
+      // @todo This detection of modifications is just a proof-of-concept
+      // and might be far away from being robust.
+      $value = $this->{$name}->getValue();
+      foreach ($this->get($name) as $key => $field) {
+        $mainPropertyName = $field->mainPropertyName();
+        if (!$mainPropertyName) {
+          // @todo could that happen, that there's no main property?

Another possibility to improve this could be looping on every field property and skip only computed ones. If even one stored property has changes we are fine. We still need to check $this->value for existence as the setter might be used to initialize the field value.

@plach: Thanks for your review. Maybe I'm not able to work on it the next days but I'll definitely continue and improve the patch. (And I reserved a lot of time for the dev days in Montpellier.)

But to minimize the effort I want to be sure that we all prefer the same approach:

The preferred solution will be to continue my idea described in #12 in combination with (most of) plach's suggestions from #35 and #36. That means to improve the patch from #34 instead of introducing a second field like discussed in #8 and #9.

Anyone who doesn't agree with that?

Created meta

hmm I think I'd agreed with #9 rather than the current version of the patch.

The last changed time of the entity is a different thing from the last time a specific translation was saved, so it feels OK to have two columns to represent this - we had a similar-ish problem in previous releases of Drupal where there was {node}.uid and {revision}.uid but not {revision}.revision_uid - so you could either store revisions of the node author, or store the author of the revision, but not both at the same time.

In practice plach pointed out in irc that changed will also be last(translation_updated) but it seems worth making that explicit in the schema.

This also complicates for example admin/content - if you list all entities in the current language, but want to order by entity changed.

Well, personally I'd tend to configure admin/content so it either lists translations only in the current content language (with fallback), in which case sorting on translation modification would make sense, or lists all translations, in which case sorting on translation modification would still make sense. This would leave out the case where you list translations only in the default entity language or current language and you want to sort on changed across all translations. Not sure this use case is that common (or useful), although it's how things work in D7 with ET (because they cannot work in any other way).

Anyway, I'm fine to get back to #8 if @catch thinks it's viable. It would be good to wait also for @Berdir's feedback before proceeding, I asked him to have a look to the latest comments.

I just want to summarize both approaches.

The first approach as described in #8 and #9:

• The required "changed" field of entities will be turned in an untranslatable field.
• We additionally introduce a new translatable "translation_changed" field.
• The information when an entity has been saved in general or when a specific translation has been modified for the last time is stored in the database.

The second approach as described in #12 and discussed later on:

• The "changed" field is kept as translatable as it is now in core.
• The information when an entity has been saved in general or when a specific translation has been modified for the last time is implemented in code.

Independent from the preference of one of both approaches, we require these common additions:

• New getters (and setters) to access the last changed time of an entity in general or a concrete translation.
• A tracking of modifications of translated fields. BTW we should watch #2456257: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work which is related.

My patch from #34 already includes (some of) these common additions and implements the second approach. Now the question is if I should just polish this patch to meet plach's requirements or if I should additionally modify it to follow the first approach?

If we go for the first approach we need to be aware of these two points:

• Unless a feature like #2297817: Do not attempt field storage write when field content did not change exists in core, we need a new field type different from ChangedItem because this one always acts like a MySQL timestamp.
• We need a kind of EntityTranslationChangedInterface because EntityChangedInterface seems to be currently used for to access both meanings of the changed time.

These two issues were the reason why I introduced the second approach as an alternative. I also asked some developers in my company after I noticed the issues during implementing the first approach. The feedback I got was that they expect the same transparent behavior of that existing changed field like they know from other fields: if it's translatable it should reflect the latest change of a translation, the latest change of an entity in general is just needed internally in core (to invalidate caches, avoid edit conflicts, ...) and should be irrelevant when you deal with entities as a contrib developer.

I'll try the second approach again.

Regarding plach's comments from #35:

1., 3., 4., 5., 14. and Miro's concerns from #23
all solved by merging the essential parts from #2456257: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work patch #6 and made them "translation aware"

2. good enough for the moment. Maybe we can leverage FieldItemListInterface::equals from https://www.drupal.org/node/2297817#comment-9817055 later.

6. done

7. TBD because it's used by ContentTranslationMetadataWrapper

8., 9., 10., 12. done by providing a generic implementation in ContentEntityBase

11. done

13. done

function boolval() does not exist in PHP 5.4 ;-)

Hi!! @mkalkbrenner could you post an interdiff? Could be great to review the last patch. Also, we need tests.

The test is inside the patch and the interdiff seems to be as long as the patch so we can review it.

Here's a interdiff for #44 that leverages the new FieldItemList::equals() method from #2297817: Do not attempt field storage write when field content did not change for detection of changes.

I'm a bit wary about changing the semantics of 'updated' / 'created' to suddenly be about "a given translation" rather than "the entity as a whole" - which I guess means a +1 on approach 1) ?

Also, I have to say I've never been a fan of CreatedItem / ChangedItem being standalone field types (= Timestamp field type for the data model + hardocded special behavior. Field types should be about data, and semantics/behavior should be assigned from the outside - i.e the Entity class). Not strictly for this patch to change, but if fixing this issue here had a side effect of getting rid of those, I wouldn't mourn them :-)

After a quick discussion with fago I added a small performance improvement to the interdiff.

https://www.drupal.org/documentation/git/interdiff

The idea of an interdiff is the difference between patches. If there's no patch 50 I don't understand the interdiff

@rodrigoaguilera: you're right. The missing patch adds an improvement depending on #2297817: Do not attempt field storage write when field content did not change. Therefore I just uploaded the interdiff for discussion to avoid that the test result turns red until #2297817: Do not attempt field storage write when field content did not change gets committed. And I didn't want to postpone this issue just for an optimization.

Patch #44 is still the one to be applied against current core.

So here is an improved version of the patch from #44 which requires the patch #89 from #2297817: Do not attempt field storage write when field content did not change to be applied first.
(I'm doing that to enable others to test this stuff at the drupal dev days)

Tests are included in the latest patches.

My patches solve the translatable "changed" timestamp issue. But I'm not sure about the translatable "created" date.
For nodes it's a feature to allow modifications to it via the node edit form. But we can discuss about the correct default value. Like for the other stuff I see both use cases where you want to display or sort by the created timestamp of the original node or by the created timestamp of a specific translation.
Anyhow I consider the translatable created timestamp default value issue less critical as the changed timestamp, because the change timestamp issue blocks others.

Therefor I recommend to focus on the changed timestamp here and to maybe open a separate issue for the created timestamp.

Noting some things we talked about in person and in IRC

@mkalkbrenner
I suggest reuploading the green patch. to be clear that it could go in.
open another postponed issue on this an the critical for the interdiff of using that method that will simplify the logic.
especially a summary of the two proposed resolutions and the pros and cons of them.
and then identify what the next steps are
it might be that the next step is a full review of your patch
or that we need to have concensous about a solution.

As @YesCT suggested I uploaded the patch from #44 again. This one could go in and improved later if #2297817: Do not attempt field storage write when field content did not change is committed.

Finally #2297817: Do not attempt field storage write when field content did not change got committed :-) Let's leverage it's improvements.

Additionally I had another discussion with @fago and decided to remove the getter methods form ContentEntityBase again and introduce a Trait instead to get rid of all this redundant code.

I expected that leveraging FiledItemList:equals() should ease things. But it seems to break stuff, at least the way I used it :-(

It turned out that FieldItemListInterface::equals() introduces a lot of overhead if you want to re-use it here and therefor have to convert values into a FieldItemList in a non erroneous way like I tried in #62. Due to the fact that this list gets then converted back to a list of values within FieldItemListInterface::equals() it makes sense to extend the FieldItemListInterface and to offer an additional convenience method FieldItemListInterface::consistOfValues().

I adjusted patch #62 accordingly.

I mad a small mistake and took the wrong variable in on line.

And after a short discussion with @jhodgdon I renamed FieldItemListInterface::consistOfValues() to FieldItemListInterface::valuesEqual().

Thanks for the issue summary update, that helps to get a hold of this very complex situation.

Some notes:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -52,6 +52,13 @@
      /**
   +   * The list of field names of any fields that have changed.
   +   *
   +   * @var string[]
   +   */
   +  protected $changedFields = array();
   

   1. The field names are additionally keyed by language code so that should be documented (i.e. also document which langcode they are keyed by.

   2. Because of the above I think the correct type would be string[][], but I don't think we allow that currently in core, so let's just do "array" for now.

   3. Minor: Use "[]" instead of "array()". (also below)

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -428,6 +435,23 @@ public function getFields($include_computed = TRUE) {
   +    return array_filter($this->getFields(), function ($field) {
   +      return isset($this->changedFields[$field->getName()]);
   +    });
   

   It seems a little bit backwards to go through all fields and filter them, when we already have an explicit list in $this->changedFields. I would find something like

   $fields = [];
   foreach ($this->changedFields as $field_name => $langcodes) {
     $fields[$field_name] = $this->getFieldDefinition($name);
   }
   return $fields;
   

   more intuitive. The only functional difference would be that that includes computed fields, so we could add a check to avoid that if wanted.

   Both options are not really optimal, so you can also leave it as is, just wanted to note that the code is sort of unintuitive to me.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -592,6 +616,38 @@ public function onChange($name) {
   +    if ($this->hasChangesAcrossTranslations()) {
   +      foreach ($this->changedFields as $langcodes) {
   +        if (isset($langcodes[$this->activeLangcode])) {
   +          return TRUE;
   +        }
   +      }
   

   It seems the $this->hasChangesAcrossTranslations() check is unnecessary, no? If there aren't any changes, $this->changedFields will simply be an empty array, which is fine to foreach over.

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -728,6 +784,8 @@ public function addTranslation($langcode, array $values = array()) {
   +    // Todo Review
   +    $this->changedFields['__dummy'][$langcode] = TRUE;
   

   Hehe :-)

5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -752,6 +810,12 @@ public function removeTranslation($langcode) {
   +        if (empty($this->changedFields[$name])) {
   

   Minor: Since we already know that $this->changedFields[$name] is set, this can be just !$this->changedFields[$name]

6. +++ b/core/lib/Drupal/Core/Entity/EntityChangedTrait.php
   @@ -0,0 +1,44 @@
   +    return $this->get('changed')->value;
   

   I don't think it's good to hardcode the name of the changed field. Content translation already does that in ContentTranslationMetadataWrapper, but we should not add even more of that. So for now, I think we should just leave this out of the trait, as we also have #2209971: Automatically provide a changed field definition for that.

7. +++ b/core/lib/Drupal/Core/Entity/EntityChangedTrait.php
   @@ -0,0 +1,44 @@
   +    $changed = $this->getUntranslated()->getChangedTime();
   +    foreach ($this->getTranslationLanguages(FALSE) as $language) {
   

   Pretty sure that explicitly getting the untranslated one can be avoided by not setting the $include_default parameter in getTranslationLanguages() to FALSE.

8. +++ b/core/lib/Drupal/Core/Entity/EntityChangedTrait.php
   @@ -0,0 +1,44 @@
   +      if ($translation_changed > $changed) {
   +        $changed = $translation_changed;
   +      }
   

   Minor: I personally find $changed = max($changed, $translation_changed); more readable but I guess that's a matter of opinion so feel free to leave as is.

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

   This seems very unintuitive at first. Let me try to understand it:
   If *any* translation of the current entity has been updated in the meantime the current translation can no longer be saved. Is that really what we want?

   In any case, the expected behavior should be documented here.

10. +++ b/core/lib/Drupal/Core/Field/FieldItemListInterface.php
    @@ -271,4 +271,15 @@ public static function processDefaultValue($default_value, FieldableEntityInterf
    +  /**
    +   * Determines if the field items match a given value list.
    +   *
    +   * @param array $values
    +   *   The value list to compare to.
    +   *
    +   * @return bool
    +   *   TRUE if the field item list matches $values, FALSE if not.
    +   */
    +  public function valuesEqual($values);
    

    This is rather unfortunate, as AFAIK the whole $values concept is an implementation detail of ContentEntityBase that only exists to avoid instantiating FieldItemList objects in some cases. Thus, it is not very semantic to expose this in the interface.

    I understand, though, why you added this and I have no better suggestion ATM, so just noting for now.

11. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
    @@ -30,7 +32,35 @@ class ChangedItem extends CreatedItem {
    +        $original = $original->getTranslation($entity->language()->getId());
    

    If this is necessary, then the wrong translation of $original is passed. Shouldn't we fix the calling code instead?

12. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
    @@ -30,7 +32,35 @@ class ChangedItem extends CreatedItem {
    +      // An example of setting the timestamp explicitly is
    +      // \Drupal\content_translation\ContentTranslationMetadataWrapperInterface
    

    Let's reference the respective method directly, i.e. ContentTranslationMetadataWrapperInterface::setChangedTime(). (I think the namespace can be omitted in this case, as the class name is pretty clear.)

    More importantly, I have yet to understand, why content translation needs to set anything at all, as this very code should take care of it?! That seems to be pre-existing, but still...

13. +++ b/core/modules/comment/src/CommentStatistics.php
    @@ -127,7 +127,7 @@ public function create(FieldableEntityInterface $entity, $fields) {
    -        $last_comment_timestamp = $entity->getChangedTime();
    +        $last_comment_timestamp = $entity->getChangedTimeAcrossTranslations();
    
    @@ -243,9 +243,9 @@ public function update(CommentInterface $comment) {
    -          'last_comment_timestamp' => ($entity instanceof EntityChangedInterface) ? $entity->getChangedTime() : REQUEST_TIME,
    +          'last_comment_timestamp' => ($entity instanceof EntityChangedInterface) ? $entity->getChangedTimeAcrossTranslations() : REQUEST_TIME,
    

    Hmm... that's very unfortunate but there's no other way currently, I guess. Let's add a @todo to make comment statistics (entity) language aware.

14. +++ b/core/modules/node/node.module
    @@ -751,8 +751,9 @@ function node_page_title(NodeInterface $node) {
     function node_last_changed($nid, $langcode = NULL) {
    -  $changed = \Drupal::entityManager()->getStorage('node')->loadUnchanged($nid)->getChangedTime();
    -  return $changed ? $changed : FALSE;
    +  $node = \Drupal::entityManager()->getStorage('node')->loadUnchanged($nid);
    +  $changed = $langcode ? $node->getTranslation($langcode)->getChangedTime() : $node->getChangedTimeAcrossTranslations();
    +  return $changed ?: FALSE;
     }
    

    Wow, that's messed up that $langcode was ignored previously...

    There's still two possible meanings of omitting the langcode:
    1. Give me the changed time across translations (the current implementation)
    2. Give me the changed time of the default (== original) language

    I think 1. (the current approach) does make more sense, but we should update the documentation to make that clear.

15. +++ b/core/modules/node/src/NodeForm.php
    @@ -289,7 +289,7 @@ protected function actions(array $form, FormStateInterface $form_state) {
    -    if ($node->id() && (node_last_changed($node->id(), $this->getFormLangcode($form_state)) > $node->getChangedTime())) {
    +    if ($node->id() && (node_last_changed($node->id()) > $node->getChangedTimeAcrossTranslations())) {
           $form_state->setErrorByName('changed', $this->t('The content on this page has either been modified by another user, or you have already submitted modifications using this form. As a result, your changes cannot be saved.'));
         }
    

    Wow, I didn't know this code still existed, I thought that was all handled by the changed field. As stated above, I'm not sure this is the correct implementation: I personally think the changed validation should per translation (i.e. intra-translation). If the French translation is changed. I should still be able to edit the German translation in the meantime.

    A more advanced feature would be notifying if the source language of the current translation has been updated in the meantime but that would certainly be a new feature.

16. +++ b/core/modules/node/src/Tests/NodeTranslationChangedTest.php
    @@ -0,0 +1,96 @@
    +  public function run(array $methods = array()) {
    +    if (empty($methods)) {
    +      $class = get_class($this);
    +      // Iterate through all the methods in this class,
    +      // unless a specific list of methods to run was passed.
    +      $methods = array_filter(get_class_methods($class), function ($method) {
    +        return (strpos($method, 'test') === 0) && (!in_array($method, array(
    +          // Avoid to run these tests implemented in NodeTranslationUITest
    +          // twice.
    +          'testTranslationUI',
    +          'testTranslationLinkTheme',
    +          'testDisabledBundle',
    +          'testTranslationRendering',
    +        )));
    +      });
    +    }
    +    parent::run($methods);
    +  }
    

    Let's not hack around our own codebase. Instead let's add a "NodeTranslationUiTestBase" (also note the lowercase "i") which has the shared methods and then make the two classes extend that.

17. +++ b/core/modules/node/src/Tests/NodeTranslationChangedTest.php
    @@ -0,0 +1,96 @@
    +    $entity = entity_load($this->entityTypeId, $this->entityId, TRUE);
    

    Let's use the entity manager for that.

18. +++ b/core/modules/node/src/Tests/NodeTranslationChangedTest.php
    @@ -0,0 +1,96 @@
    +        // Ensure different timestamps.
    +        sleep(2);
    

    Wow, very unfortunate :-( Can't we just set the timestamp manually now?

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -428,6 +435,23 @@ public function getFields($include_computed = TRUE) {
   +    return array_filter($this->getFields(), function ($field) {
   +      return isset($this->changedFields[$field->getName()]);
   +    });
   

   This is a bit inefficient, since we are not interested in all fields. Just iterate over changedFields and call ->get() foreach of them.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -728,6 +784,8 @@ public function addTranslation($langcode, array $values = array()) {
   +    // Todo Review
   +    $this->changedFields['__dummy'][$langcode] = TRUE;
   

   Leftover, this should probably b removed?

3. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityInterface.php
   @@ -204,6 +219,17 @@ public function getFields($include_computed = TRUE);
      /**
   +   * Determines if at least one field item (of the current translation) has been
   +   * changed.
   +   */
   +  public function hasChanges();
   

   @return docs missing. I assume this is bool?

4. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityInterface.php
   @@ -204,6 +219,17 @@ public function getFields($include_computed = TRUE);
   +  /**
   +   * Determines if at least one field item has been changed.
   +   */
   +  public function hasChangesAcrossTranslations();
   

   Same here.

5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -30,7 +32,35 @@ class ChangedItem extends CreatedItem {
   +        else {
   +          // @todo is this field or its code re-usable for something else than
   +          // ContentEntities? However the default behavior is to set the current
   +          // REQUEST_TIME as value on save.
   +          $this->value = REQUEST_TIME;
   +        }
   

   I think this @todo can be removed and replaced with a comment like "Always update the changed time for non-fieldable entities."

Otherwise I think this looks good!

@tstoeckler and I discussed his and @klausi's findings face to face at dev days and updated the patch accordingly.

we missed to remove one function from the trait ...

I moved the tests from NodeTranslationChangedTest to ContentTranslationUITest to apply them to all entities using content_translation.

talked with @mkalkbrenner about that and completely agree that we should have working changed timestamp for each translation.

I'm trying to look at that from the point of a developer reading the tests to understand that difference between getChangedTimeAcrossTranslations() and getChangedTime(), and I think it's a bit confusing:

1. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityInterface.php
   @@ -204,6 +211,25 @@ public function getFields($include_computed = TRUE);
   +  public function hasChanges();
   
   +++ b/core/modules/comment/src/Tests/CommentTokenReplaceTest.php
   @@ -61,7 +61,7 @@ function testCommentTokenReplacement() {
   -    $tests['[comment:changed:since]'] = \Drupal::service('date.formatter')->formatInterval(REQUEST_TIME - $comment->getChangedTime(), 2, $language_interface->getId());
   
   +++ b/core/modules/node/src/Tests/NodeTranslationChangedTest.php
   @@ -0,0 +1,74 @@
   +        // Set the title and the custom translatable field and the revision log
   +        // to predictable values containing a counter.
   +        $edit = array(
   +          'title[0][value]' => 'title ' . $langcode . ' ' . $i,
   +          $this->fieldName . '[0][value]' => $this->fieldName . ' ' . $langcode . ' ' . $i,
   +        );
   +        $edit_path = $entity->urlInfo('edit-form', array('language' => $language));
   +        $this->drupalPostForm($edit_path, $edit, $this->getFormSubmitAction($entity, $langcode));
   

   do we need to use the FormAPI to do that? Should work with just using the Entity API.

2. +++ b/core/modules/node/src/Tests/NodeTranslationChangedTest.php
   @@ -0,0 +1,74 @@
   +        sleep(2);
   

   doesn't sleep(1) also work? less long running of tests :)

3. +++ b/core/modules/node/src/Tests/NodeTranslationChangedTest.php
   @@ -0,0 +1,74 @@
   +        $counters[$langcode] = $i;
   

   not used anymore?

4. +++ b/core/modules/node/src/Tests/NodeTranslationChangedTest.php
   @@ -0,0 +1,74 @@
   +        if ($i > 1) {
   +          // Because the base class of this test created the translations
   +          // without a sleep command, the changed timestamps might not differ
   +          // until the second run of that loop.
   +          $timestamps = array();
   +          foreach ($entity->getTranslationLanguages() as $language) {
   +            $next_timestamp = $entity->getTranslation($language->getId())
   +              ->getChangedTime();
   +            if (!in_array($next_timestamp, $timestamps)) {
   +              $timestamps[] = $next_timestamp;
   +            }
   +          }
   +          $this->assertTrue(
   +            count($timestamps) == count($entity->getTranslationLanguages()),
   +            'All timestamps from all languages are different.'
   +          );
   +        }
   

   does that really have to run inside of the for loop? It's super hard to understand :) as we run inhere again a foreach loop. This could just happen at the end no?

5. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -375,9 +375,19 @@ protected function defaultValueWidget(FormStateInterface $form_state) {
   +    return $this->valuesEqual($list_to_compare->getValue());
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function valuesEqual($value2) {
   +    if (!is_array($value2)) {
   +      $value2 = array(0 => $value2);
   +    }
        $columns = $this->getFieldDefinition()->getFieldStorageDefinition()->getColumns();
        $count1 = count($this);
   -    $count2 = count($list_to_compare);
   +    $count2 = count($value2);
        if ($count1 === 0 && $count2 === 0) {
          // Both are empty we can safely assume that it did not change.
          return TRUE;
   @@ -387,7 +397,6 @@ public function equals(FieldItemListInterface $list_to_compare) {
   
   @@ -387,7 +397,6 @@ public function equals(FieldItemListInterface $list_to_compare) {
          return FALSE;
        }
        $value1 = $this->getValue();
   -    $value2 = $list_to_compare->getValue();
        if ($value1 === $value2) {
   

   not sure if this is directly related to this issue? Shouldn't this be it's separate issue with a test? But don't want to postpone anything here.

Worked together with @mkalkbrenner on reviewing this patch. I have to agree that the solution 2), so not storing two different fields, is the better way to move forward.

I just added some comments @dawehner requested during review (he was fine with the code itself).

Additionally I (hopefully) fixed the tests.

Had a lunchtime discussion with @mkalkbrenner @yched @berdir @swentel @hchonov @amateescu about this. This issue is important because we can't solve most of the issues under #2453153: Node revisions cannot be reverted per translation without it.

Generally there was agreement that:

- The issue noted in #2453153: Node revisions cannot be reverted per translation is legitimate, and needs to be fixed.
- It doesn't make any sense for node.created to be translatable.

yched was still pretty concerned about using a single field for this. A couple of things:

1) We want to make sure that for people who are trying to get a list of entities, they can do a quick query to do e.g. "get all nodes that were updated since the last time cron ran." There was concern that tying this to translations would invoke a entity load on each one.

2) Using two fields/API methods allows people to be very explicit about what they want.

For #1, we can use EntityFieldQuery (which you should be doing anyway), which will already factor in translations (unless you narrow to a translation, it's cross-translation).

For #2, the general idea of loading an entity in a particular translation, and thereafter all fields of that entity get accessed in that language, so treating changed like this is consistent with other fields that are translatable. (e.g. changed is, node ID is not)

In the end, yched is out of counter-arguments, as long as we do additional manual testing to ensure EFQ works as intended.

Did I fix the last two remaining instances of ContentTranslationUITest?

Just a quick update.

I fixed all the UI tests and added some more using the Entity API only.
But during writing the Entity API test I run into various new issues I have to fix before continuing here and uploading a "final" patch:
#2475237: Method FieldItemList::equals() uses type safe comparison limiting usefulness
#2474075: Fix Node::preSave() and document that preSave() and postSave() are not working with ContentEntity translations

I upload a new patch that includes the one from #2475237: Method FieldItemList::equals() uses type safe comparison limiting usefulness #6 just to leverage a fast testbot.
Using that patch we could simplify this one. Let's see what happens ...

Adjusted patch 2428795_translatable_changed_time_including_2475237-9_86.patch to comment #9 of #2475237: Method FieldItemList::equals() uses type safe comparison limiting usefulness.
It includes 2475237-9.patch from there to be testable here. Otherwise the tests will fail.

2428795_translatable_changed_time_86-do-not-test.patch is the right patch to commit if 2475237-9.patch has been committed.

Thanks to #2475237: Method FieldItemList::equals() uses type safe comparison limiting usefulness I'm able to simplify this patch here. I removed all the changed tracking in setters and the corresponding API changes. So the only remaining API change is the new getChangedTimeAcrossTranslations(). That's what all the participates in comment #80 agreed on. I adjusted the issue description accordingly.

I also extended the tests included in the patch. UI based tests for the correct changed timestamp for content entities using content translation have been added to

• BlockContentTranslationUITest
• CommentTranslationUITest
• ContentTestTranslationUITest
• MenuLinkContentUITest
• NodeTranslationUITest
• ShortcutTranslationUITest
• TermTranslationUITest
• UserTranslationUITest

I also added a test that directly uses the ebtity API as @Schnitzel requested in #76 named Drupal\system\Tests\Entity\ContentEntityChangedTest.

I added even more tests compared to #86:

• changed times on translatable and revisionable content entities on the API layer
• EntityQueries that query the changed field in various ways, like requested by @yched and documented by @webchick in #80

The (confusing) schema of patches is still the same as explained in #86 ;-)

I re-rolled the patch against current head and probably solved #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating) as well.

@pjonckiere: thanks for your formal review. I adjusted the comments accordingly.

Here are some comments:

4. This todo was requested by tstoeckler. It's up to him to open the follow up. Due to the fact that this is not related to this issue I removed that comment.

6. This comment is left from an earlier version of this patch. I missed to remove it.

12. I'm not yet familiar with @covers. Examples?

15. Which line exactly?

20. This test was explicitly requested by @yched. I thought the assertion explains its purpose.

21. I don't think so ...

25. That's copy and paste. It makes no sense to just modify this comment.

comment statistics have weird interface
And currently maintained with

->condition('default_langcode', 1)

+++ b/core/modules/comment/src/CommentStatistics.php
@@ -126,7 +126,6 @@
-      // @todo make comment statistics language aware.
       if ($entity instanceof EntityChangedInterface) {
         $last_comment_timestamp = $entity->getChangedTimeAcrossTranslations();

Here should be comment and todo with link to add tests for that

re-added the comment as requested by @andypost and @tstoeckler.

the failed test reported in #97 seemed to be a testbot issue.

Overall this looks great to me, some nitpicks:

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -30,7 +30,48 @@ class ChangedItem extends CreatedItem {
   +        // \Drupal\content_translation\ContentTranslationMetadataWrapperInterface::setChangedTime()
   

   Missing trailing dot.

2. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -77,6 +78,8 @@ class BlockContent extends ContentEntityBase implements BlockContentInterface {
   +  use EntityChangedTrait;
   +
   
   +++ b/core/modules/comment/src/Entity/Comment.php
   @@ -63,6 +64,8 @@ class Comment extends ContentEntityBase implements CommentInterface {
   +  use EntityChangedTrait;
   +
   
   +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -59,6 +60,8 @@ class MenuLinkContent extends ContentEntityBase implements MenuLinkContentInterf
   +  use EntityChangedTrait;
   +
   
   +++ b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
   @@ -0,0 +1,350 @@
   + * Definition of Drupal\system\Tests\Entity\ContentEntityChangedTest.
   
   +++ b/core/modules/user/src/Entity/User.php
   @@ -69,6 +70,8 @@ class User extends ContentEntityBase implements UserInterface {
   +  use EntityChangedTrait;
   +
   

   We usually add use statements on the top, below the class name.

3. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
   @@ -384,4 +385,61 @@ protected function doTestTranslationEdit() {
   +    $changed_field_name = $entity->hasField('content_translation_changed') ? 'content_translation_changed' : 'changed';
   

   Please, let's add a method subclasses can override to provide the field name. This way contrib modules can provide alternate implementations and can exploit core tests to add test coverage.

4. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
   @@ -384,4 +385,61 @@ protected function doTestTranslationEdit() {
   +    $translatable_changed_field = $entity->getFieldDefinition($changed_field_name)->isTranslatable();
   

   I guess we should manually switch translatability and test both cases for each run.

5. +++ b/core/modules/file/src/Tests/SaveTest.php
   @@ -22,8 +22,6 @@ function testFileSave() {
   -      'created' => 1,
   -      'changed' => 1,
          'status' => FILE_STATUS_PERMANENT,
   
   @@ -64,8 +62,6 @@ function testFileSave() {
   -      'created' => 1,
   -      'changed' => 1,
          'status' => FILE_STATUS_PERMANENT,
   

   Why we need to remove these?

6. +++ b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
   @@ -0,0 +1,350 @@
   + * Definition of Drupal\system\Tests\Entity\ContentEntityChangedTest.
   

   "Definition of" should be "Contains" (see https://www.drupal.org/coding-standards/docs#file)

7. +++ b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
   @@ -0,0 +1,350 @@
   +    // We can't assert equality here because the created time is set to the
   +    // request time, while instances of ChangedTestItem use the current
   +    // timestamp every time.
   +    $this->assertTrue(
   +      ($entity->getChangedTime() >= $entity->get('created')->value) &&
   +      (($entity->getChangedTime() - $entity->get('created')->value) <= time() - REQUEST_TIME),
   +      'Changed and created time of original language can be assumed to be identical.'
   +    );
   +
   

   IMO this is another clue that ChangedItem should not encapsulate all that logic, as @yched was pointing out above. However this patch did not introduce that and it's probably too late to change it.

8. +++ b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
   @@ -0,0 +1,350 @@
   +    $entity->addTranslation('de');
   +
   +    $german = $entity->getTranslation('de');
   

   ::addTranslation() will return $german, no need to get it afterwards.

9. +++ b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
   @@ -0,0 +1,350 @@
   +    $entity_query_service = $this->container->get('entity.query');
   +
   +    $query = $entity_query_service->get('entity_test_mul_changed');
   ...
   +    $query = $entity_query_service->get('entity_test_mul_changed');
   ...
   +    $query = $entity_query_service->get('entity_test_mul_changed');
   ...
   +    $query = $entity_query_service->get('entity_test_mul_changed');
   ...
   +    $query = $entity_query_service->get('entity_test_mul_changed');
   ...
   +    $query = $entity_query_service->get('entity_test_mul_changed');
   ...
   +    $query = $entity_query_service->get('entity_test_mul_changed');
   ...
   +
   +    $query = $entity_query_service->get('entity_test_mul_changed');
   ...
   +    $query = $entity_query_service->get('entity_test_mul_changed');
   ...
   +    $query = $entity_query_service->get('entity_test_mul_changed');
   

   The recommended way tor retrieve the query backend is from the storage handler: EntityStorageInterface::getQuery().

10. +++ b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
    @@ -0,0 +1,350 @@
    +    $ids = $query->condition('changed', $changed_en, '=', 'en')->execute();
    

    Can we add also a check when using default_langcode = 1 instead of an explicit language?

11. +++ b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
    @@ -0,0 +1,350 @@
    +    $this->assertTrue(
    +      empty($ids),
    +      'There\'s no original entity stored having the changed time of the German translation.'
    +    );
    ...
    +    $this->assertTrue(
    +      empty($ids),
    +      'There\'s no German translation stored having the changed time of the original language.'
    +    );
    

    We usually write this as assertFalse($ids, ...).

12. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/ChangedTestItem.php
    @@ -0,0 +1,44 @@
    + * Contains \Drupal\Core\Entity\Plugin\Field\FieldType\ChangedItem.
    

    Wrong class name.

@plach: I addressed all your findings from #101. Have a look at the interdiff. Regarding number 5: the test should verify if created and changed are set to a correct value on save. Due to the fact that it is now allowed to explicitly set a value for changed programmatically, setting a a value of 1 in the test is wrong. Setting to *nothing* is correct. That hopefully solves #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating) as well. I additionally removed the intial value '1' for created as well, because it has no effect on the test and looks confusing is it stands alone.

Meanwhile #2475237: Method FieldItemList::equals() uses type safe comparison limiting usefulness has been committed to core :-)
So we can really proceed here and I don't upload these confusing four files any longer.

missed the interdiff ...

fixed the tests and added some comments

Looks good, it would be nice to address the couple of nitpicks listed below, if we happen to reroll this. Setting to needs work as this will need a change record. I provided the beta evaluation.

It would be great if @catch had the chance to review this before commit, since he was involved in the discussion.

1. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
   @@ -390,56 +404,75 @@
   +    $base_field_override = BaseFieldOverride::createFromBaseFieldDefinition($entity->getFieldDefinition($changed_field_name), $entity->bundle());
   ...
   +        $base_field_override->setTranslatable($translatable_changed_field);
   +        $base_field_override->save();
   

   A cleaner way to do this would be:

   $definition = $entity->getFieldDefinition($changed_field_name);
   $config = $definition->getConfig($entity->bundle());
   
   foreach (array(FALSE, TRUE) as $translatable_changed_field) {
     if ($definition->isTranslatable()) {
       // For entities defining a translatable changed field we want to test
       // the correct behavior of that field even if the translatability is
       // revoked. In that case the changed timestamp should be synchronized
       // across all translations.
       $config->setTranslatable($translatable_changed_field);
       $config->save();
     }
     // ...
   }
   

2. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
   @@ -390,56 +404,75 @@
   +    foreach (array(FALSE, TRUE) as $translatable_changed_field) {
   

   We use square brackets for arrays now.

3. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
   @@ -390,56 +404,75 @@
   +      elseif ($translatable_changed_field) {
   +        // For entities defining a non-translatable changed field we cannot
   +        // declare the field as translatable on the fly using BaseFieldOverride
   +        // because the schema doesn't support this.
   +        break;
   +      }
   

   Are we adding this just for completeness' sake or do we have cases in core where this happens?

I addressed the remaining nitpicks @plach pointed out in #106.
The answer for point 3 is, that we need to avoid translatability tests for entities that don't have declared the changed field to be translatable up-front. If we want to do that, we have to modify the schema as well to be able to store translated values. For me that seems to be too much effort for that test, because there's no corresponding use-case.
For example for nodes the test turns off the translatability of the changed field, tests if the timestamps are identical for all translations, turns translatability on again and tests if the timestamps are different. For entities that don't have a translatable changed field this flip is skipped and they are only tested for identical timestamps. Without that break; the tests will fail for $translatable_changed_field = TRUE.

I created a changed record draft: https://www.drupal.org/node/2483093

The answer for point 3 is, that we need to avoid translatability tests for entities that don't have declared the changed field to be translatable up-front.

I get that, I was just wondering whether we have such cases in core, I was assuming all changed field storage definitions would be marked as translatable in this issue.

This looks ready to me now, just waiting for @mkalkbrenner's reply before RTBC-ing it.

From the CR:

The translatability of entity changed timestamps provided by the ChangedItem field type was broken and is fixed now.

Can we explain what was broken and how it is now fixed?

I was assuming all changed field storage definitions would be marked as translatable in this issue.

No, I didn't change that. I think we should open a follow-up issue if we decide to change that for some entities. Here's the list.

Entities containing a translatable changed field:

• Comment
• Node
• ... and all entities that get their changed field "injected" by the ContentTranslationHandler

Entities containing a non-translatable changed field:

• BlockContent
• File
• MenuLinkContent
• Term
• User

I extended the change record.

As requested by @plach via IRC I upload a patch that turns the changed filed into a translated one for BlockContent, MenuLinkContent, Term and User. Let's see what happens to the tests ...

This looks good to go to me. I know @catch had some doubts about this approach so it would be better if he had a chance to review this before commit.

Just a reminder, see comment #80. Both approaches have been discussed in a meeting at Drupal Dev Days moderated by @webchick. @yched, @berdir and @dewehner agreed on the one field approach and I continued the development on this patch accordingly.

The created field is being addressed in #2472621: Translatable entity 'created' and 'uid' fields not initialized properly during content translation 'Add'.

Assigning to catch, per #115.

I talked to plach about the approach on irc, and after quite a bit of persuading, I think all of my objections are covered now. I understand yched had similar reservations and a similar conversation at dev days.

Don't have time for a full review this morning, here's a couple of small things I noticed though.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -30,7 +30,48 @@ class ChangedItem extends CreatedItem {
   +        // If the timestamp has not been set explicitly auto detect a
   +        // modification of the current translation and set the timestamp if
   +        // needed. An example of setting the timestamp explicitly is
   +        // \Drupal\content_translation\ContentTranslationMetadataWrapperInterface::setChangedTime().
   +        if ($this->value == $original->{$field_name}->value) {
   +          foreach ($entity->getFieldDefinitions() as $other_field_definition) {
   +            $other_field_name = $other_field_definition->getName();
   

   This looks like it'd be worth factoring out to a helper method.

   - there's a lot of nesting
   - knowing if the current translation was modified or not could be generally useful elsewhere? Could start with a protected helper then make it public later if that turns out to be useful.

2. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
   @@ -384,4 +399,79 @@ protected function doTestTranslationEdit() {
   +            // Because the base of this test created the translations without a
   

   Why not sleep(2) at the start?

knowing if the current translation was modified or not could be generally useful elsewhere? Could start with a protected helper then make it public later if that turns out to be useful.

Yes, it is useful :-)
Therefor I started such an enhancement in #2453153: Node revisions cannot be reverted per translation. I was of the opinion that it should be implemented there along with the concrete use-case instead of complicating the discussions here in this issue. I suggest to also reduce the nesting and to introduce helper functions in this follow-up. Do you agree?
(see #80 for a reference to #2453153: Node revisions cannot be reverted per translation)

Why not sleep(2) at the start?

A single sleep(2) at the start is not sufficient because we need to ensure that two translations aren't modified and saved within in the same second.
To get rid of that loop we have to add a sleep(1) to doTestBasicTranslation() before any translation that is added. I didn't do that, because it's up to the subclass of ContentTranslationUITest to decide which parts of the tests to run. And the developer may decide to run doTestBasicTranslation() but to skip doTestTranslationChanged().
But I can change that if you want.

I think I'd probably move the sleep(1) to the base class then, if there's no other way to avoid the loop. If we have an explicit follow-up that will add the helper that seems OK yes, but maybe add a @todo pointing to that issue.

I added the todo and reviewed the test. I got rid of that loop without adding any additional sleeps :-)

Looks much better.

Back to RTBC

I've looked at this twice since yesterday and each time I wasn't quite ready to commit it yet despite thinking I was over my initial misgivings beforehand.

If another committer sees this and thinks it's fine, please go ahead, otherwise I'll think on it a bit more and/or try to get a second opinion.

I'm not sure that the method names are that great. getChangedTime on the Entity level sounds like it should get me the time that the entity last changed (regardless of translations) whereas this is actually getChangedTimeAcrossTranslations which sounds like something from another dimension. How about getChangedTime and getTranslationChangedTime? Leaving at rtbc for @catch to disregard this if he likes.

diff --git a/core/modules/content_translation/src/Tests/ContentTranslationUITest.php b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
index 8ef68e2..8d12742 100644
--- a/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
+++ b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
@@ -8,7 +8,6 @@
 namespace Drupal\content_translation\Tests;
 
 use Drupal\Core\Entity\EntityInterface;
-use Drupal\Core\Field\Entity\BaseFieldOverride;
 use Drupal\Core\Language\Language;
 use Drupal\Core\Language\LanguageInterface;
 use Drupal\Core\Url;
diff --git a/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
index 0a38c2e..34e9f0a 100644
--- a/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
+++ b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
@@ -7,9 +7,7 @@
 
 namespace Drupal\system\Tests\Entity;
 
-use Drupal\Core\Entity\EntityStorageException;
 use Drupal\language\Entity\ConfigurableLanguage;
-use Drupal\user\UserInterface;
 
 /**
  * Tests basic EntityChangedInterface functionality.
diff --git a/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulRevChanged.php b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulRevChanged.php
index 67e772b..c2f572f 100644
--- a/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulRevChanged.php
+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulRevChanged.php
@@ -9,7 +9,6 @@
 
 use Drupal\Core\Entity\EntityTypeInterface;
 use Drupal\Core\Field\BaseFieldDefinition;
-use Drupal\entity_test\Entity\EntityTestRev;
 
 /**
  * Defines the test entity class.

There are some unused uses that this patch adds.

At the dev days we also discussed the method names. getChangedTime() is the one that already exists. The argument to keep this existing method to get the changed timestamp of the current translation is that across the core 90% of the existing calls need and expect that behavior and it is consistent with other getters that act on the translation. As you can see in the patch there're only a few places where changed timestamp is requeset across the translations. @webchick agreed on these arguments.

Another small advantage of the naming is that both methods start with "getChangedTime". In this case your editors code completion will present you both variants while typing and they are listed next to each other.

BTW I think we'll get some more functions with an "AcrossTranslations" suffix when we keep fixing the remaining D8MI issues. So it might become a pattern instead of something from another dimension ;-) Again, the default of other getters always return the value of the current translation if the field is translated.

I think I‘d like to get a look at this too, but I can't do so before monday night. Can we hold this until then ?

@yched yep we can.

I removed the use statements.

Sorry, for being out of this for such a long time. @mkalkbrenner and everyone else: incredible work on this in the meantime. The current state of the patch looks great, I'm very impressed. The test coverage is also not only extensive but the test itself is very readable. Awesome, awesome, awesome.

I read through the entire patch and I don't have any big objections. The following nitpicks are really unimportant I just didn't want them to get lost, since I noticed them anyway.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -30,7 +30,52 @@ class ChangedItem extends CreatedItem {
   +      // for example when migration is running.
   

   "when a migration is running" or maybe just "during migrations".

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -30,7 +30,52 @@ class ChangedItem extends CreatedItem {
   +        // If the timestamp has not been set explicitly auto detect a
   

   "auto detect" -> "auto-detect"

3. +++ b/core/modules/node/node.module
   @@ -743,8 +743,8 @@ function node_page_title(NodeInterface $node) {
   + *   (optional) The language to get the last changed time for. If omitted the
   

   Comma after "If omitted"

4. +++ b/core/modules/shortcut/src/Tests/ShortcutTranslationUITest.php
   @@ -102,4 +103,16 @@ protected function doTestTranslationEdit() {
   +      format_string('%entity is not implementing EntityChangedInterface.' , array('%entity' => $this->entityTypeId))
   

   Should be String::format().

5. +++ b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
   @@ -0,0 +1,362 @@
   +    $this->assertTrue(
   +      $entity->getChangedTime() >= REQUEST_TIME,
   +      'Changed time of original language is valid.'
   +    );
   +
   +    // We can't assert equality here because the created time is set to the
   +    // request time, while instances of ChangedTestItem use the current
   +    // timestamp every time.
   +    $this->assertTrue(
   +      ($entity->getChangedTime() >= $entity->get('created')->value) &&
   +      (($entity->getChangedTime() - $entity->get('created')->value) <= time() - REQUEST_TIME),
   +      'Changed and created time of original language can be assumed to be identical.'
   +    );
   +
   +    $this->assertEqual(
   +      $entity->getChangedTime(), $entity->getChangedTimeAcrossTranslations(),
   +      'Changed time of original language is the same as changed time across all translations.'
   +    );
   

   Seems like the comment should be moved above the upper assertTrue()?

   Also, instead of using &&, there should be two assertions. That makes things easier to debug.

6. +++ b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
   @@ -0,0 +1,362 @@
   +    // Current state: changed time of original language (en) is newer than the
   +    // changed time of the German translation. Now test that entity queries work
   +    // as expected.
   

   Reading a "Current state:", I would expect a similar "State now:" or something below, but there's no such thing. Maybe just something like "At this point ..."

   Also: "the changed time of the original language (en)".

7. +++ b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
   @@ -0,0 +1,362 @@
   +      'Entity query can access changed time of original language.'
   ...
   +      'Entity query can access changed time of original language.'
   

   In general, I'm a fan of leaving out assertion messages altogether because it makes it easier to debug the tests. In general I don't care enough to point it out, but in this case showing the same assertion message for two different assertions right after another is not very helpful, IMO.

   What I do think would be helpful is moving that sentence into the inline code comment for future onlookers of this test code.

8. +++ b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
   @@ -0,0 +1,362 @@
   +      'There\'s no original entity stored having the changed time of the German translation.'
   ...
   +      'There\'s no entity stored using the default language having the changed time of the German translation.'
   ...
   +      'There\'s no German translation stored having the changed time of the original language.'
   

   Instead of escaping the possessive apostrophe it's generally preferred to use a double-quoted string ("").

9. +++ b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
   @@ -0,0 +1,362 @@
   +      'Entity query can access changed time of German translation.'
   ...
   +      'Entity query can access changed time of German translation.'
   ...
   +      'Entity query can access changed time regardless of translation.'
   ...
   +      'Entity query can access changed time regardless of translation.'
   ...
   +      'Entity query can access changed time regardless of translation.'
   ...
   +      'Entity query can access changed time regardless of translation.'
   

   See the above comment about the assertion messages.

10. +++ b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
    @@ -0,0 +1,362 @@
    +    $entity->addTranslation('de');
    ...
    +    $german = $entity->getTranslation('de');
    

    Super minor, but I don't think the addTranslation() should be necessary.

11. +++ b/core/modules/system/src/Tests/Entity/ContentEntityChangedTest.php
    @@ -0,0 +1,362 @@
    +      'Changed time of German translation is newer then the original language.'
    ...
    +      'Changed time of the German translation is the newest time across all translations.'
    

    Once it's just "German translation" and once it's "the German translation", can we make that consistent?

    Also the second one has the word "time" duplicated, which is unnecessary IMO.

12. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/ChangedTestItem.php
    @@ -0,0 +1,44 @@
    +      // During a test the request time is immutable. To allow tests of the
    +      // algorithm of
    

    "To allow tests of the algorithm" doesn't sound like proper English to me.

1. +++ b/core/lib/Drupal/Core/Entity/EntityChangedInterface.php
   @@ -22,11 +22,19 @@
   +   * Returns the timestamp of the last entity change for current translation.
   

   "for the current translation" ?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -30,7 +30,52 @@ class ChangedItem extends CreatedItem {
   +    if ($entity->isNew()){
   +      // Allow the changed timestamp to be set manually on entity creation,
   +      // for example when migration is running.
   +      if (!$this->value) {
   +        $this->value = REQUEST_TIME;
   +      }
   +    }
   +    else {
   +      // Don't allow an illegal value.
   +      if (!$this->value) {
   +        $this->value = REQUEST_TIME;
   +      }
   +      else {
   

   - Couldn't this be shortened to :

   if (!$this->value) {
     $this->value = REQUEST_TIME;
   }
   elseif (!$entity->isNew()) {
     ...
   }
   

   - I don't get the "Don't allow an illegal value" comment in the patch, BTW - not clear how this is related to a notion of "illegal values", nor what those "illegal values" would be ? Maybe we should just skip that comment, we simply set the value if it's not set ?

3. Then, more generally about the code in that "else {...}" branch :

   - Could really use a sentence explaining the high-level logic here - like "Only update the timestamp if the translation has actually been modified, i.e. if some other field values have changed" ?

   - The way we do this (get all field values, compare them) scares me a bit.
   a) SqlStorage already does this comparison, we re-do it here, too bad, but not sure how to avoid that.
   b) SqlStorage only does it for fields that are actually stored, this here could get messy if we have computed fields : getting their value can be expensive, and they are irrelevant for our concerns here anyway (a computed field can change for reasons external to the $entity, we are only interested in inner entity values).
   So I think we should at least skip fields where $definition->isComputed() is TRUE.

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -30,7 +30,52 @@ class ChangedItem extends CreatedItem {
   +        // We have to clone the original because getTranslation() will modify
   +        // the state of original.
   

   nitpick : repeating "the original" sounds awkward - maybe : "Clone $enity->original to avoid modifying it when calling getTranslation()" ?

5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -30,7 +30,52 @@ class ChangedItem extends CreatedItem {
   +        if ($this->value == $original->{$field_name}->value) {
   

   We should avoid mixing $entity->{$name} and $entity->get($name) syntax in the same code block

6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -30,7 +30,52 @@ class ChangedItem extends CreatedItem {
   +          foreach ($entity->getFieldDefinitions() as $other_field_definition) {
   +            $other_field_name = $other_field_definition->getName();
   

   could be shortened to foreach ($entity->getFieldDefinitions() as $other_field_name => $other_field_definition)

7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -30,7 +30,52 @@ class ChangedItem extends CreatedItem {
   +              $items = $entity->get($other_field_name)->filterEmptyItems();
   +              $original_items = $original->get($other_field_name)
   +                ->filterEmptyItems();
   

   nitpick : inline both ->fieldterEmptyItems() or none ?
   (I'd vote for both)

8. +++ b/core/modules/node/src/NodeForm.php
   @@ -289,7 +289,7 @@ protected function actions(array $form, FormStateInterface $form_state) {
   -    if ($node->id() && (node_last_changed($node->id(), $this->getFormLangcode($form_state)) > $node->getChangedTime())) {
   +    if ($node->id() && (node_last_changed($node->id()) > $node->getChangedTimeAcrossTranslations())) {
   

   This removes the last use of the $langcode param for node_last_changed(), do we want to keep it ?

Then, I wasn't really aware of content_translation/src/ContentTranslationMetadataWrapper,
and the fact that content_translation/src/ContentTranslationMetadataWrapper adds its own per-translation tracking fields for status, changed, etc, if the entity type doesn't provide them.

I hate to ask again at this point, but - what was the problem with keeping 'changed' as entity-level, cross-translation and letting content_translation track per-translation metadata for all entities ?

I hate to ask again at this point, but - what was the problem with keeping 'changed' as entity-level, cross-translation and letting content_translation track per-translation metadata for all entities ?

We cannot rely on CT being enabled, the Entity Field API needs to deal with this stuff without depending on it. Moreover the current approach is more consistent with the regular Entity Translation API usage: fields translatability can be switched and each translation gives access to the field in a certain language if they are translatable.

Also:

[This approach] minimizes the impact on the API, as the change can be (more or less) BC, given that changed is already marked as translatable in a few cases, and that marking it translatable in the other ones does not imply storage changes.

Alright. Still can't say I'm fully on board, but well, this has been discussed at length, and I don't have a better proposal, so I'll shut up :-)

rebase conflicts ... just leverage a test bot

OK, I think I solved the rebase conflict and addressed @yched's and some of @tstoeckler's suggestions. For example skipping computed fields.

The interdiff is against the last working version of the patch in #131 to ease the review.

Thanks @mkalkbrenner :-)
Fine by me if green.

@yched: thanks for your feedback!

So, back to RTBC?

Yup, back to "I'm out of objections" ;-)

(Also, despite my lack of enthusiasm for the approach, thanks a *ton* for sticking to this @mkalkbrenner, awesome work !)

Discussed with @plach at drupalcon. Thanks everyone for working towards the consensus.

Committed a451002 and pushed to 8.0.x. Thanks!

Thank you for adding the beta evaluation to the issue summary.