Reusing initialized constraint validators overwrittes validation errors

1. +++ b/core/lib/Drupal/Core/Validation/ConstraintValidatorFactory.php
   @@ -25,12 +25,13 @@ public function __construct(ClassResolverInterface $class_resolver) {
   +    // Constraint validator instances should always be initialized newly and
   +    // never shared, because the current validation context is getting injected
   +    // into them through setter injection and in a case of a recursive
   +    // validation where a validator triggers a validation chain leading to the
   +    // same validator the context of the first call would be exchanged with the
   +    // one of the subsequent validation chain.
   +    return $this->classResolver->getInstanceFromDefinition($class_name);
   

   As suggested in #2438017-65: Entity reference does not validate auto-created entities.2 we might need to get right of this class. Or if Symfony introduces a getter method on the constraint validator we might revert this change later and only temporarily exchange the context on each validator object. Please create a follow-up issue about this and reference it from here.

2. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Validation/Constraint/ReferenceRecursiveConstraint.php
   @@ -0,0 +1,19 @@
   + * Supports recursive validation of entity references.
   

   "Entity Reference valid reference constraint." would be a better fit.

3. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Validation/Constraint/ReferenceRecursiveConstraint.php
   @@ -0,0 +1,19 @@
   + *   id = "ReferenceRecursiveConstraint",
   ...
   +class ReferenceRecursiveConstraint extends Constraint {
   

   I think that TestRecursiveValidationConstraint would be a better fit as an ID and class name.

4. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Validation/Constraint/ReferenceRecursiveConstraint.php
   @@ -0,0 +1,19 @@
   +  public $message = 'Invalid referenced entity.';
   

   This looks mixed across core, but let's document the class variable like this:

     /**
      * The default violation message.
      *
      * @var string
      */
   

5. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Validation/Constraint/ReferenceRecursiveConstraintValidator.php
   @@ -0,0 +1,31 @@
   +  public function validate($value, Constraint $constraint) {
   ...
   +    foreach ($value as $delta => $item) {
   

   $value => items

6. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Validation/Constraint/ReferenceRecursiveConstraintValidator.php
   @@ -0,0 +1,31 @@
   +      if ($violations->count() !== 0) {
   

   Is this check really needed?

7. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Validation/Constraint/ReferenceRecursiveConstraintValidator.php
   index 97b8215330..6fca8d0b6e 100644
   --- a/core/tests/Drupal/KernelTests/Core/Entity/ValidReferenceConstraintValidatorTest.php
   
   --- a/core/tests/Drupal/KernelTests/Core/Entity/ValidReferenceConstraintValidatorTest.php
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/ValidReferenceConstraintValidatorTest.php
   

   This test file tests the ValidReferenceConstraint. But here we introduce a new test dealing with the recursive validation and therefore should be placed under \Drupal\KernelTests\Core\TypedData\RecursiveContextualValidatorTest

8. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ValidReferenceConstraintValidatorTest.php
   @@ -231,4 +231,41 @@ public function testPreExistingItemsValidation() {
   +   * Validation of referenced entity does not break validation of parent entity.
   

   Test recursive validation.

9. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ValidReferenceConstraintValidatorTest.php
   @@ -231,4 +231,41 @@ public function testPreExistingItemsValidation() {
   +
   

   Not needed empty line at the beginning. Here we should explain why we are creating the 2 fields and how the recursive validation is triggered.

10. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ValidReferenceConstraintValidatorTest.php
    @@ -231,4 +231,41 @@ public function testPreExistingItemsValidation() {
    +      'user_id' => ['target_id' => 0],
    ...
    +      'user_id' => ['target_id' => 0],
    

    Why is this needed?

11. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ValidReferenceConstraintValidatorTest.php
    @@ -231,4 +231,41 @@ public function testPreExistingItemsValidation() {
    +      //'string_required' => 'some string',
    

    This is unnecessary.

12. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ValidReferenceConstraintValidatorTest.php
    @@ -231,4 +231,41 @@ public function testPreExistingItemsValidation() {
    +    $this->assertCount(1, $violations);
    +    $this->assertEquals('field_test', $violations[0]->getPropertyPath());
    

    Document what we except here and why.

#5.1 still missing.

This test file tests the ValidReferenceConstraint. But here we introduce a new test dealing with the recursive validation and therefore should be placed under \Drupal\KernelTests\Core\TypedData\RecursiveContextualValidatorTest

I have moved it there. And now we have in that file two tests with ridiculously differing names testRecursiveValidate and testRecursiveValidation that test almost opposite functionality: first one tests that missing referenced entity doesn't break the validation and second that referenced entity not passing the validation will also raise violation at referencing one. I'm confused.

Then you can rename the new method to for example ::testRecursiveViolationPropagation().

1. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Validation/Constraint/TestValidatedReferenceConstraint.php
   @@ -0,0 +1,24 @@
   + *   id = "TestValidatedReferenceConstraint",
   + *   label = @Translation("Reference recursive constraint.")
   

   The label needs to be updated now as well.

2. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Validation/Constraint/TestValidatedReferenceConstraintValidator.php
   @@ -0,0 +1,29 @@
   + * Simple recursive validator.
   

   This should match the constraint - "Validates referenced entities".

3. +++ b/core/tests/Drupal/KernelTests/Core/TypedData/RecursiveContextualValidatorTest.php
   @@ -41,4 +42,49 @@ public function testRecursiveValidate() {
   +    // We create 2 fields: one to hold entity reference and the second required
   +    // string field which should trigger validation failure. To the entity
   +    // reference field we attach a constraint that triggers validation of
   +    // referenced entity.
   +    // We will set required field on parent entity and not set on referenced
   +    // one. If things work correct, validating of parent entity will trigger
   +    // validation of referenced one, which should fail and trigger failure on
   +    // parent entity with violation path of reference field. If not, the
   +    // correct validation of required field for entity 1 will cancel its failure
   +    // for entity 2 and there will not be validation errors on entity 1.
   

   When documenting tests we don't need to explain what happens when something does not behave like expected. We document only what should happen. The reason is that today it fails for one reason, but tomorrow it might fail for another the the "if things don't work there will be.." might not be correct anymore. What about the following a little bit shorter text:

   "We create an entity reference field with a constraint which will trigger the validation of the referenced entities. Then we add a required field and populate it only on the parent entity, so that the child entity fails the validation."

4. +++ b/core/tests/Drupal/KernelTests/Core/TypedData/RecursiveContextualValidatorTest.php
   @@ -41,4 +42,49 @@ public function testRecursiveValidate() {
   +    $entity_1 = EntityTest::create([
   ...
   +    $entity_2 = EntityTest::create([
   

   I think it makes sense to rename the entities to $parent and $child.

5. +++ b/core/tests/Drupal/KernelTests/Core/TypedData/RecursiveContextualValidatorTest.php
   @@ -41,4 +42,49 @@ public function testRecursiveValidate() {
   +
   ...
   +
   ...
   +
   

   The new lines aren't really needed.

6. +++ b/core/tests/Drupal/KernelTests/Core/TypedData/RecursiveContextualValidatorTest.php
   @@ -41,4 +42,49 @@ public function testRecursiveValidate() {
   +    $entity_1->field_test = $entity_2;
   

   Instead of doing this, we could first create the $child entity and then pass it to the values of $parent when creating it.

7. +++ b/core/tests/Drupal/KernelTests/Core/TypedData/RecursiveContextualValidatorTest.php
   @@ -41,4 +42,49 @@ public function testRecursiveValidate() {
   +    $violations = $entity_1->validate();
   

   Let's document this: "The child entity should fail the validation and the violation should propagate to the parent."

8. +++ b/core/tests/Drupal/KernelTests/Core/TypedData/RecursiveContextualValidatorTest.php
   @@ -41,4 +42,49 @@ public function testRecursiveValidate() {
   +    $this->assertEquals('field_test', $violations[0]->getPropertyPath());
   

   Also let's assert that the violation messages matches the expected message.

Looks good now. Thank you!

Yes, but only in the case of nested entity references, where the constraint validator of the one level triggers the validation of the next level.