Update content entity changed timestamp on UI save

I think pressing save in the UI should result in the time being updated. Unless we don't save the entity and reject the save and inform the user nothing has changed so not saving.

1. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -221,6 +221,14 @@ public function getChangedTime() {
      /**
       * {@inheritdoc}
       */
   +  public function setChangedTime($timestamp) {
   +    $this->set('changed', $timestamp);
   +    return $this;
   +  }
   
   +++ b/core/modules/comment/src/Entity/Comment.php
   @@ -531,6 +531,14 @@ public function getChangedTime() {
      /**
       * {@inheritdoc}
       */
   +  public function setChangedTime($timestamp) {
   +    $this->set('changed', $timestamp);
   +    return $this;
   +  }
   
   +++ b/core/modules/file/src/Entity/File.php
   @@ -120,6 +120,14 @@ public function getChangedTime() {
      /**
       * {@inheritdoc}
       */
   +  public function setChangedTime($timestamp) {
   +    $this->set('changed', $timestamp);
   +    return $this;
   +  }
   
   +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -142,6 +142,14 @@ public function getChangedTime() {
      /**
       * {@inheritdoc}
       */
   +  public function setChangedTime($timestamp) {
   +    $this->set('changed', $timestamp);
   +    return $this;
   +  }
   
   +++ b/core/modules/node/src/Entity/Node.php
   @@ -228,6 +228,14 @@ public function getChangedTime() {
      /**
       * {@inheritdoc}
       */
   +  public function setChangedTime($timestamp) {
   +    $this->set('changed', $timestamp);
   +    return $this;
   +  }
   
   +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestConstraints.php
   @@ -54,4 +54,11 @@ public function getChangedTime() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setChangedTime($timestamp) {
   +    $this->set('changed', $timestamp);
   +    return $this;
   +  }
   
   +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulChanged.php
   @@ -80,4 +80,11 @@ public function getChangedTime() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setChangedTime($timestamp) {
   +    $this->set('changed', $timestamp);
   +    return $this;
   +  }
   
   +++ b/core/modules/taxonomy/src/Entity/Term.php
   @@ -197,6 +197,14 @@ public function getChangedTime() {
      /**
       * {@inheritdoc}
       */
   +  public function setChangedTime($timestamp) {
   +    $this->set('changed', $timestamp);
   +    return $this;
   +  }
   
   +++ b/core/modules/user/src/Entity/User.php
   @@ -385,6 +385,14 @@ public function getChangedTime() {
      /**
       * {@inheritdoc}
       */
   +  public function setChangedTime($timestamp) {
   +    $this->set('changed', $timestamp);
   +    return $this;
   +  }
   

   If we agree on providing the function setChangedTime() it should be part of the EntityChangedTrait. BTW it seems that getChangedTime() could be moved to this trait as well now. That could be added to this patch from my point of view.

2. +++ b/core/modules/node/src/Entity/Node.php
   @@ -486,6 +494,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      ->setDefaultValue('')
   

   Is this the right issue to fix that?

3. +++ b/core/modules/node/src/Tests/NodeFormSaveChangedTimeTest.php
   @@ -0,0 +1,74 @@
   +class NodeFormSaveChangedTimeTest extends WebTestBase {
   

   This test only covers Node and its UI. But the patch introduces a behavior for all entities implementing the EntityChangedInterface.

   Therefor I recommend to extend
   ContentEntityChangedTest first which is supposed to test the functions declared by EntityChangedInterface on the API level.

   The UI should be covered by an additional function of ContentTranslationUITestBase. This way it will cover all the entities that are already affected by this patch.

Why should we create an test exactly in ContentTranslationUITestBase? At the moment my changes have nothig to do with the content translation module.

It's a suggestion because the tests implemented there are automatically executed for all entity types that are affected by this patch while the test included in the patch only covers nodes!

I gave a framework manager point of view in #4

Agreed with alexpott in #4.

+++ b/core/modules/content_translation/src/ContentTranslationHandler.php
@@ -579,6 +579,10 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
+    // Each ContentEntityForm will register also an entity builder method to
+    // update the changed time of the entity, but here we have to guarantee that
+    // the metadata field is updated as well, if not the entity's own changed
+    // field is being used for the metadata.
     $metadata->setChangedTime(REQUEST_TIME);

This is wrong.

Changing the changed time breaks the changed validation that we have on forms that happens *later*. This means that an old entity is always considered up to date. We *must* use the submitted, original changed time for that validation and only change it when the validation succeeded.

This is hiding the currently broken EntityChangedConstraintValidator that is comparing the saved changed date of all translations with the changed date of the current translation, which makes it impossible to save old translations.

Changing the changed time breaks the changed validation that we have on forms that happens *later*. This means that an old entity is always considered up to date. We *must* use the submitted, original changed time for that validation and only change it when the validation succeeded.

@berdir is right.

Whenever an entity is saved via the UI it's original changed timestamp must checked if it's still the newest (or if a different user modified the entity in the meantime).
That's what the EntityChangedConstraint normally does. Therefor this issue here is also indirectly blocked by #2513094: ContentEntityBase::getTranslatedField and ContentEntityBase::__clone break field reference to parent entity.

Nevertheless we need to have a look at Content Translation if it deals correctly with that situation.

1. +++ b/core/modules/node/src/Tests/NodeFormSaveChangedTimeTest.php
   @@ -0,0 +1,74 @@
   +class NodeFormSaveChangedTimeTest extends WebTestBase {
   

   I still don't like that we only add test coverage for nodes. The patch modifies ContentEntityForm.

As explained earlier I copied the test case to ContentTranslationUITestBase.
Without really changing the test we now have a great coverage of content entities in combination with content_translation. That's correct because module is also affected by this patch.

Let's see which side effects the additional test might discover.

As @berdir suggested in https://www.drupal.org/node/2534512#comment-10156016 I merged that patch to see how the tests work here now.

The thing is that the other issue won't be RTBC'd without test coverage. test coverage that this issue provides, at least in the form integration tests.

As I suggested in #2534512-13: EntityChangedConstraint needs to compare changed time of all translations, if we merge that issue into this, then we have a better chance of it getting committed I think. It's a one line fix, and we can explain in the issue summary why that is needed and close the other issue as a duplicate.

PS: You want to reference to other issues with "[ #2534512 ]" (without the spaces). That way, you get things like automatic coloring. If you are using dreditor (which you should) then you can just paste the issue URL and then press Tab and it will convert it automatically. I'm always irritated that your links don't have the color I'd expect, took me a while that you are probably building them by hand.

The changes proposed here make sense to me, the issue summary is up to date and @Berdir's concerns have been resolved. Let's get this in, so we can fix #2480921: Make the node entity's revision_log untranslatable too finally :)

Sorry if this is just piling on to what you already know but i was desperate for this fix, I've tested this patch on 8.0.x head and it worked for me without error :)

A few nitpicks, if this happens to be rerolled:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -269,4 +278,18 @@ public function updateFormLangcode($entity_type_id, EntityInterface $entity, arr
   +    if ($entity->getEntityType()->isSubclassOf('Drupal\Core\Entity\EntityChangedInterface')) {
   
   +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
   @@ -531,4 +532,30 @@ protected function doTestTranslationChanged() {
   +    if ($entity->getEntityType()->isSubclassOf('Drupal\Core\Entity\EntityChangedInterface')) {
   

   We can use .class here, I think

2. +++ b/core/modules/node/src/Tests/NodeFormSaveChangedTimeTest.php
   @@ -0,0 +1,74 @@
   +  protected function setUp() {
   

   Missing PHP doc

I meant EntityChangedInterface::class :)

All minor but a couple of real questions.

1. +++ b/core/lib/Drupal/Core/Entity/EntityChangedTrait.php
   @@ -28,4 +28,26 @@ public function getChangedTimeAcrossTranslations() {
   +  public function getChangedTime() {
   

   Nice adding this here and removing from the entity classes :)

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -579,7 +579,12 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
   +    // the entity validation has ran. But if we set an metadata field for the
   

   a metadata

3. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -579,7 +579,12 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
   +    // translation metadata we have to update it here.
   

   Why do we have to?

4. +++ b/core/modules/node/src/Tests/NodeFormSaveChangedTimeTest.php
   @@ -0,0 +1,77 @@
   +    $this->assertEqual($changed_timestamp, $node->getChangedTime(), 'The entity\'s changed time wasn\'t updated after API save without changes.');
   

   Double quotes?

5. +++ b/core/modules/node/src/Tests/NodeFormSaveChangedTimeTest.php
   @@ -0,0 +1,77 @@
   +    debug($node->getChangedTime(), 'second time');
   

   Left over debug?

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

   Why sleep(2) here and sleep(1) in the other test?

#64 seems to address @catch's review.

Yep. Committed/pushed to 8.0.x, thanks!

This was an API break, so needs a change record.
Anyone with an entity implementing EntityChangedInterface would now get fatals until they add the method or the trait.

Thanks

thanks!

published