Issue #2808335 by hchonov, tstoeckler, anish.a, Wim Leers, catch, Berdir:...

Issue #2808335 by hchonov, tstoeckler, anish.a, Wim Leers, catch, Berdir: Changed time not updated when only non-translatable fields are changed on a translated entity

core/lib/Drupal/Core/Entity/ContentEntityBase.php

@@ -1266,6 +1266,31 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
     return array();
   }
 
+  /**
+   * Returns an array of field names to skip in ::hasTranslationChanges.
+   *
+   * @return array
+   *   An array of field names.
+   */
+  protected function getFieldsToSkipFromTranslationChangesCheck() {
+    // A list of known revision metadata fields which should be skipped from
+    // the comparision.
+    // @todo Replace the hard coded list of revision metadata fields with the
+    // solution from https://www.drupal.org/node/2615016.
+    $fields = [
+      $this->getEntityType()->getKey('revision'),
+      'revision_translation_affected',
+      'revision_uid',
+      'revision_user',
+      'revision_timestamp',
+      'revision_created',
+      'revision_log',
+      'revision_log_message',
+    ];
+
+    return $fields;
+  }
+
   /**
    * {@inheritdoc}
    */
@@ -1299,13 +1324,17 @@ public function hasTranslationChanges() {
     // possible or be meaningless.
     /** @var \Drupal\Core\Entity\ContentEntityBase $translation */
     $translation = $original->getTranslation($this->activeLangcode);
+
+    // The list of fields to skip from the comparision.
+    $skip_fields = $this->getFieldsToSkipFromTranslationChangesCheck();
+
     foreach ($this->getFieldDefinitions() as $field_name => $definition) {
       // @todo Avoid special-casing the following fields. See
       //    https://www.drupal.org/node/2329253.
-      if ($field_name == 'revision_translation_affected' || $field_name == 'revision_id') {
+      if (in_array($field_name, $skip_fields, TRUE)) {
         continue;
       }
-      if (!$definition->isComputed() && (!$translated || $definition->isTranslatable())) {
+      if (!$definition->isComputed()) {
         $items = $this->get($field_name)->filterEmptyItems();
         $original_items = $translation->get($field_name)->filterEmptyItems();
         if (!$items->equals($original_items)) {

core/lib/Drupal/Core/Entity/ContentEntityInterface.php

@@ -25,9 +25,6 @@ interface ContentEntityInterface extends \Traversable, FieldableEntityInterface,
   /**
    * Determines if the current translation of the entity has unsaved changes.
    *
-   * If the entity is translatable only translatable fields will be checked for
-   * changes.
-   *
    * @return bool
    *   TRUE if the current translation of the entity has changes.
    */

core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php

@@ -129,14 +129,29 @@ public function testNodeRevisionsTabWithDefaultRevision() {
 
     // Create the node.
     $node = $this->drupalCreateNode();
+    $storage = \Drupal::entityTypeManager()->getStorage($node->getEntityTypeId());
 
+    // Create a new revision based on the default revision.
+    // Revision 2.
+    $node = $storage->load($node->id());
     $node->setNewRevision(TRUE);
     $node->save();
+
+    // Revision 3.
+    $node = $storage->load($node->id());
     $node->setNewRevision(TRUE);
     $node->save();
+
+    // Revision 4.
+    // Trigger translation changes in order to show the revision.
+    $node = $storage->load($node->id());
+    $node->setTitle($this->randomString());
     $node->isDefaultRevision(FALSE);
     $node->setNewRevision(TRUE);
     $node->save();
+
+    // Revision 5.
+    $node = $storage->load($node->id());
     $node->isDefaultRevision(FALSE);
     $node->setNewRevision(TRUE);
     $node->save();
@@ -147,17 +162,15 @@ public function testNodeRevisionsTabWithDefaultRevision() {
 
     // Verify that the default revision can be an older revision than the latest
     // one.
-    $this->assertLinkByHref('/node/' . $node_id . '/revisions/5/revert');
+    // Assert that the revisions with translations changes are shown: 1 and 4.
+    $this->assertLinkByHref('/node/' . $node_id . '/revisions/1/revert');
     $this->assertLinkByHref('/node/' . $node_id . '/revisions/4/revert');
+
+    // Assert that the revisions without translations changes are filtered out:
+    // 2, 3 and 5.
+    $this->assertNoLinkByHref('/node/' . $node_id . '/revisions/2/revert');
     $this->assertNoLinkByHref('/node/' . $node_id . '/revisions/3/revert');
-    $current_revision_row = $this->xpath("//table[contains(@class, :table_class)]//tbody//tr[3 and contains(@class, :class) and contains(., :text)]", [
-      ':table_class' => 'node-revision-table',
-      ':class' => 'revision-current',
-      ':text' => 'Current revision',
-    ]);
-    $this->assertEqual(count($current_revision_row), 1, 'The default revision can be a revision other than the latest one.');
-    $this->assertLinkByHref('/node/' . $node_id . '/revisions/2/revert');
-    $this->assertLinkByHref('/node/' . $node_id . '/revisions/1/revert');
+    $this->assertNoLinkByHref('/node/' . $node_id . '/revisions/5/revert');
   }
 
 }

core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php

@@ -154,7 +154,7 @@ protected function getExpectedNormalizedEntity() {
       ],
       'changed' => [
         [
-          'value' => '123456789',
+          'value' => (string) $this->entity->getChangedTime(),
         ],
       ],
       'default_langcode' => [

core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php

@@ -186,13 +186,6 @@ public function setUp() {
 
       // Set a default value on the field.
       $this->entity->set('field_rest_test', ['value' => 'All the faith he had had had had no effect on the outcome of his life.']);
-      // @todo Remove in this if-test in https://www.drupal.org/node/2808335.
-      if ($this->entity instanceof EntityChangedInterface) {
-        $changed = $this->entity->getChangedTime();
-        $this->entity->setChangedTime(42);
-        $this->entity->save();
-        $this->entity->setChangedTime($changed);
-      }
       $this->entity->save();
     }
   }

core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php

@@ -126,7 +126,7 @@ protected function getExpectedNormalizedEntity() {
       ],
       'changed' => [
         [
-          'value' => '123456789',
+          'value' =>  (string) $this->entity->getChangedTime(),
         ],
       ],
       'promote' => [

core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php

@@ -108,7 +108,7 @@ protected function getExpectedNormalizedEntity() {
       ],
       'changed' => [
         [
-          'value' => '123456789',
+          'value' =>  (string) $this->entity->getChangedTime(),
         ],
       ],
       'default_langcode' => [

core/modules/rest/tests/src/Functional/EntityResource/User/UserResourceTestBase.php

@@ -104,7 +104,7 @@ protected function getExpectedNormalizedEntity() {
       ],
       'changed' => [
         [
-          'value' => '123456789',
+          'value' =>  (string) $this->entity->getChangedTime(),
         ],
       ],
       'default_langcode' => [

core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulChanged.php

@@ -60,6 +60,10 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
       ->setDescription(t('The time that the entity was last edited.'))
       ->setTranslatable(TRUE);
 
+    $fields['not_translatable'] = BaseFieldDefinition::create('string')
+      ->setLabel(t('Non translatable'))
+      ->setDescription(t('A non-translatable string field'));
+
     return $fields;
   }
 

core/modules/taxonomy/src/Entity/Term.php

@@ -233,4 +233,17 @@ public function getVocabularyId() {
     return $this->bundle();
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  protected function getFieldsToSkipFromTranslationChangesCheck() {
+    // @todo the current implementation of the parent field makes it impossible
+    // for ::hasTranslationChanges() to correctly check the field for changes,
+    // so it is currently skipped from the comparision and has to be fixed by
+    // https://www.drupal.org/node/2843060.
+    $fields = parent::getFieldsToSkipFromTranslationChangesCheck();
+    $fields[] = 'parent';
+    return $fields;
+  }
+
 }

core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php

@@ -61,6 +61,7 @@ public function testChanged() {
     // Create a test entity.
     $entity = EntityTestMulChanged::create(array(
       'name' => $this->randomString(),
+      'not_translatable' => $this->randomString(),
       'user_id' => $user1->id(),
       'language' => 'en',
     ));
@@ -122,6 +123,25 @@ public function testChanged() {
       'Changed time of the German translation did not change.'
     );
 
+    // Update a non-translatable field to make sure that the changed timestamp
+    // is updated for all translations.
+    $entity->set('not_translatable', $this->randomString())->save();
+
+    $this->assertTrue(
+      $entity->getChangedTime() > $changed_en,
+      'Changed time of original language did change.'
+    );
+
+    $this->assertTrue(
+      $german->getChangedTime() > $changed_de,
+      'Changed time of the German translation did change.'
+    );
+
+    $this->assertEquals($entity->getChangedTime(), $german->getChangedTime(), 'When editing a non-translatable field the updated changed time is equal across all translations.');
+
+    $changed_en = $entity->getChangedTime();
+    $changed_de = $german->getChangedTime();
+
     $entity->setOwner($user2);
 
     $entity->save();