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

Gábor Hojtsy’s picture

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.

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

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

Sure thing.

Gábor Hojtsy’s picture

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.

twistor’s picture

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.

twistor’s picture

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.

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.