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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
4.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

+++ 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

Sure thing.

Gábor Hojtsy’s picture

Non-empty version :D

Status: Needs review » Needs work

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

twistor’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
5.71 KB

Status: Needs review » Needs work

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

twistor’s picture

Status: Needs work » Needs review
FileSize
5.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

Is there any test coverage for this somewhere?

twistor’s picture

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

Gábor Hojtsy’s picture

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

Status: Reviewed & tested by the community » Fixed

Looks good.

Committed and pushed to 8.x. Thanks!

webchick’s picture

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

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.