Problem/Motivation

In #2074191: [META] Add changed timestamp tracking to content entities, several entities in core are getting a changed timestamp. We should also expand changed timestamp checking constraints, so the cross-editing overwrite feature from core node forms also extends to these entities.

Proposed resolution

Generalize the node changed timestamp validator to the entity level. Ensure it only attempts to work with entities supporting EntityChangedInterface.

Remaining tasks

TBD.

User interface changes

None.

API changes

None.

#2074191: [META] Add changed timestamp tracking to content entities

CommentFileSizeAuthor
#8 entity-changed-validator-8.patch5.41 KBtwistor
#6 entity-changed-validator-6.patch5.71 KBtwistor
#6 interdiff.txt1.38 KBtwistor
#4 entity-changed-validator-3.patch6.06 KBgábor hojtsy
#3 entity-changed-validator-3.patch0 bytesgábor hojtsy
#3 interdiff.txt1.07 KBgábor hojtsy
#1 entity-changed-validator.patch4.73 KBgábor hojtsy

Comments

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Active » Needs review
StatusFileSize
new entity-changed-validator.patch4.73 KB

Here is a quick untested proof of concept :) After talking to Berdir about this in IRC, he suggested we generalize the validation, so, it would look something like this I think.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Validation/Constraint/EntityChangedConstraint.php
@@ -2,24 +2,24 @@
  *

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
@@ -21,11 +21,10 @@ class NodeChangedConstraintValidator extends ConstraintValidator {
+      // entity object.
...
+      $saved_entity = entity_load($entity->type, $entity->id());

entity_load_unchanged()

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
StatusFileSize
new interdiff.txt1.07 KB
new entity-changed-validator-3.patch0 bytes

Sure thing.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
StatusFileSize
new entity-changed-validator-3.patch6.06 KB

Non-empty version :D

Status: Needs review » Needs work

The last submitted patch, entity-changed-validator-3.patch, failed testing.

twistor’s picture

twistor commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.38 KB
new entity-changed-validator-6.patch5.71 KB

Status: Needs review » Needs work

The last submitted patch, entity-changed-validator-6.patch, failed testing.

twistor’s picture

twistor commented
Status: Needs work » Needs review
StatusFileSize
new entity-changed-validator-8.patch5.41 KB

Move the validator from the entity module to Drupal\Core\Validation\Plugin\Validation\Constraint to be with the other validators.
Added EntityChangedInterface use statement.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Is there any test coverage for this somewhere?

twistor’s picture

twistor commented

NodeValidationTest checks for this explicitly. That was one of the failures in #5.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

That sounds great actually. So we have a test proving this works even on a general level now. IMHO we don't need a test to ensure all implementations, we don't test fieldability on all entities or other similar common APIs either. Let's get this in then :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Looks good.

Committed and pushed to 8.x. Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Generalize node changed constraint to entity changed constraint » Change notice: Generalize node changed constraint to entity changed constraint
Priority: Normal » Major
Status: Fixed » Active
Issue tags: +Needs change record

Hm. Actually.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Title: Change notice: Generalize node changed constraint to entity changed constraint » Generalize node changed constraint to entity changed constraint
Priority: Major » Normal
Status: Active » Fixed
Issue tags: -Needs change record, -sprint

I think I wrote https://drupal.org/node/2085445 which is IMHO as complete as such a change notice can get. So closing.

Automatically closed -- issue fixed for 2 weeks with no activity.