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

Files: 
CommentFileSizeAuthor
#8 entity-changed-validator-8.patch5.41 KBtwistor
PASSED: [[SimpleTest]]: [MySQL] 58,450 pass(es).
[ View ]
#6 entity-changed-validator-6.patch5.71 KBtwistor
FAILED: [[SimpleTest]]: [MySQL] 58,828 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#6 interdiff.txt1.38 KBtwistor
#4 entity-changed-validator-3.patch6.06 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 54,213 pass(es), 265 fail(s), and 617 exception(s).
[ View ]
#3 entity-changed-validator-3.patch0 bytesGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#3 interdiff.txt1.07 KBGábor Hojtsy
#1 entity-changed-validator.patch4.73 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity-changed-validator.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new4.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity-changed-validator.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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

StatusFileSize
new1.07 KB
new0 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Sure thing.

StatusFileSize
new6.06 KB
FAILED: [[SimpleTest]]: [MySQL] 54,213 pass(es), 265 fail(s), and 617 exception(s).
[ View ]

Non-empty version :D

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.38 KB
new5.71 KB
FAILED: [[SimpleTest]]: [MySQL] 58,828 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new5.41 KB
PASSED: [[SimpleTest]]: [MySQL] 58,450 pass(es).
[ View ]

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

Is there any test coverage for this somewhere?

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

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 :)

Status:Reviewed & tested by the community» Fixed

Looks good.

Committed and pushed to 8.x. Thanks!

Title:Generalize node changed constraint to entity changed constraintChange notice: Generalize node changed constraint to entity changed constraint
Priority:Normal» Major
Status:Fixed» Active
Issue tags:+Needs change record

Hm. Actually.

Title:Change notice: Generalize node changed constraint to entity changed constraintGeneralize 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.