Entity reference does not validate auto-created entities

Mulling on that a bit.

Entities can be autocreated :

- in the UI by EntityRef widgets. The amount of content those entities can contain on creation time depends on the "power" of the widget, AFAIK core widgets only let you assign a label, but there could be more in contrib.

- in code :

$ref = entity_create('node', $values);
$host->field_ref->entity = $ref;
$host->save(); // -> $ref gets saved as well 

No automatic validation of the $host is done on save anyway, so no reason we should validate $ref either.
But If the code choses to perform validation before saving $host, then $ref should get validated as well.

- Not sure about REST : can you do an equivalent of the code above (describe a new entity in the reference field of a host entity, and have it created in the same request that creates/updates the host entity) ?
Since REST would validate the host entity before saving it, the referenced entity would need to be validated as well.

Side note: entity_ref.module is not the right "component" for this issue, since this applies to the EntityReference field type, that is in Core. As I wrote in #2429191-4: Deprecate entity_reference.module and move its functionality to core, I think we should have a dedicated component for that, not just "entity system" or "field system".

Opened #2438267: Create an "EntityReference system" component

Here we go.

Test added.

I was talking this through with @claudiu.cristea at Drupal Camp Cluj and I think the current behavior of not validating autocreated entities is "by design", even if it's a broken design :)

The problem is that the validation will fail as soon as the autocreated entity type has a required field with no default value. We didn't have this problem in D7 because we only had form-level validate handlers, but in D8 we have constraints so it will fail even for the taxonomy terms with additional required fields.

Referring to #6. I still think that we should validate the auto-created entities and we should change the actual design (knowing that is broken). What happens if there are required fields on auto-created entities? Well, if they are well designed by the site builder those fields will get the default_value in the Entity::create() phase. Next question: What if there's no default_value set on the field? We have to fail —­ this is my opinion. We need to show not hide this field design inconsistency.

Passing to @yched for an evaluation.

EDIT: Duplicated by mistake the previous comment.

#6 and #7 are both good points :-)

Leaving entity_ref aside, and considering the case of a required field without a default value.

- Creating a entity in code without a value means the entity will fail validation (if you validate it).
- In form/UI-based creation, we currently only validate fields that are present in the form_mode, so :
If the field is in the form mode, the (form) field is required, leaving it empty raises a form error.
If the field is *not* in the form mode, we silently let you create the broken entity, without a value in the required field.

This last case is exactly as broken as the case @claudiu.cristea points in #6 - "autocreation of an entity without a value for a required". In fact, you could conceptually see the "autocomplete with autocreate" widget as a sort of form mode that lets you edit only one field (the label), which means both cases are conceptually the same.

So I guess one way to consider the "autocomplete" aspect for this issue here could be :
- The current practice is to only validate fields that are actually in the form. In accordance with that:
- The "autocomplete with autocreate" widget allows creating an entity and provides a UI to set its label, thus it's the widgets's responsibility to run validate() *on the label field* - the entity might have special constraints on its label field, like "no less that 3 characters", we need to validate them somewhere.
- A hypothetical contrib widget that lets you enter more content that just the label for autocreated entities would also be responsible of validating these additional fields it lets you edit. I guess Inline Entity Forms, whatever its D8 incarnation looks like, will use form modes and regular form validation for that, but that's the same idea.

And for the more general issue of "some forms let you create entities where some required fields are empty", well, that's a separate and pre-existing issue, we can punt on that for now here ? :-)

Unassigning myself - would be good to get @fago's opinion too ;-)

I think #9 is a great summary of the problem and I'm +1 to validate only the label field.

Let's see if we can also get feedback from @fago :)

The ER field should return somehow the list of fields he expose in form_mode, in the virtual form (e.g. in our case a list containing only the label). I'm thinking to an unified mode to implement this validation. So any implementation with more complex widgets, having other fields will automatically validate.

@claudiu.cristea: I fear this might be over-engineering ? An ER widget knows which fields it allows for edition, so let it handle it in its own validate FAPI callback ? Hypothetical contrib widgets can figure out their own complex needs themselves ?

Alright, let's do that.

#2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay is in.

Here's a patch with validations filtered by label.

@fago, @yched, @amateescu,

There's a (big) problem here: We cannot filter the validations on a label that is not explicitelly declared as label in annotation. Such a case is the User entity. This type of entity doesn't have the name specified as la label key in its annotation. Thus, we cannot obtain the label field for such entities using $entity->getEntityType()->getKey('label') to filter the violations on it.

Reverted back changes to User entity type just to reveal this bug/issue. I used @fago's solution from #14 but in this case the test shows the failure to filter the violations of a field that is stored (the user account name) but not declared in the entity type annotation as key (in this case the name of the account is not declared as label. I see no way to get the label name of an entity other than

$label_field_name = $entity->getEntityType()->getKey('label');

I'm passing this to @fago, as he pointed me in this direction.

@fago, we hardcode the 'name' column in the entity query, see Drupal\user\Plugin\EntityReferenceSelection\UserSelection::buildEntityQuery().

This is a really tricky issue, maybe we should just add the 'name' field as label entity key for users while keeping the label_callback implementation. That way, we can rely on the label key for querying and validation purposes yet $user->label() will still use label_callback for its output.

@fago,

how does that work with the autocompletion then?

Here's an answer #2281533: Entity Reference default selection plugin ignores matches if an entity type has no label key.

Yeah, I don't think ValidReferenceConstraintValidator should contain logic about the label specifically. The Autocomplete widget is the one that lets users input a label for a new entity, so it should be the one validating the label in a FAPI #element_validate of its own.

As for ValidReferenceConstraintValidator, IMO it should either :
- validate the referenced entity as a whole, making no assumptions on which fields need validation or not
- or don't bother validating the entity at all, and leave that to the code that assigns the new entity to the reference field (which is what the Autocomplete widget would do)

I think I'd lean towards the former. Validation is always left to the caller, you can save an entity without validating it.

Also, regarding users not having a "label" key : looking at EntityAutocomplete::createNewEntity() (the class providing the 'entity_autocomplete' FAPI #type) it seems like the "autocreate" widget is pretty much broken in that case anyway.

So, since validating the label of autocreated entities (and reporting the violations if any) is the job of the autocomplete widget, and that autocreate feature isn't supported for entity types that don't have a 'label' key anyway, I guess we're good ?

I guess the autocreate code would need some cleanup to properly account for the "no label key" case, and should even disable that option in the widget config UI for the corresponding referenced entity types, but that sounds like a separate issue.

Discussed with @claudiu.cristea and @amateescu in Barcelona.
Turns out the "auto-creation" feature provided by the underlying EntityAutocomplete FAPI #type can only ever works if the entity type provides a label key anyway (other than that, we cannot assign the input string as the "label" of the new entity).

So I guess "entity types that don't provide a label key" are out of scope anyway.

Why do we need validation? Drupal is responsible of creating it. Why would it be not valid? We are only getting title form user and there is no validation on title of entities anywhere in core. Am I missing something here?

This is now handled in EntityAutocomplete::validateEntityAutocomplete() and EntityAutocomplete::createNewEntity() form element it has nothing to do with ER field now IMO. Can someone please update IS and title as I presume the scope has been changed since the issue is created.

Is this patch finally abandoned in favor of an other issue?
I've tried to apply the latest patch against 8.0, 8.1, 8.2, but it didn't apply to any of them.

This would normally need to be backported to D7, but #2584225: [PP-1] Taxonomy autocomplete does not validate for term name length is already opened to do that.

Rerolled against 8.1.x

The rerolled patch could be applied, thanks

After reading the thread again, and reviewing the code it seems to me:
* The source code is so old that it doesn't seem to be usable (too much changes in core since than)
* We should close this issue as won't fix

Maybe we should link documentation how it's intended to be solved (if I understand we should implement form validators instead)

We still have the issue where the length of the autocomplete isn't validated, causing a PDO exception. We can either re-open this one or move that work back into #2584225: [PP-1] Taxonomy autocomplete does not validate for term name length

I would prefer open a separate issue for D8, since this code is not interchangeable with D7's.

It should be solved somewhere in these functions as mentioned above:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...
or
https://api.drupal.org/api/drupal/8/search/validateReferenceableNewEntities

Then let's re-open this to continue that work. And keep the D7 issue open to only work on a D7 backport.

Okay.
I think it's easier to start from scratch (based on up-to-date code), so I'm hiding current patches.

Moving to right component

Why do we need validation? Drupal is responsible of creating it. Why would it be not valid? We are only getting title form user and there is no validation on title of entities anywhere in core. Am I missing something here?

Potentially arbitrary validations have been registered, of which some might require a certains scheme for the title.

As I stumbled today above this issue, I attached a new patch for 8.4.x-dev.

Use case here is having a tagging field and having a constraint on the term name. On the field you can auto create all kind of terms, but when creating term manually constraint works. This constraint should also be used here.

The patch is still not working 100%.
I debugged a while and see that the violation is added, but form is still saved and user doesn't see an error message.

Maybe somebody more clever than me can have a look here.

Ups. 0 byte patch. Here now the patch.

This took me a while to understand ... I've encountered a big problem with the recursive validation, where a constraint validator is triggering the validation chain of another entity leading to executing the same validator for the referenced entity inside the context of the current one...

Drupal and Symfony both have the optimization of sharing the constraint validators, which in the case of recursive validation is not correct, as down the chain the execution context would be changed, which will break the whole validation. This could be observed in the patches where no violations are returned if the constraint validators are being shared.

I'll open a new issue to deal with this problem only, as the current one seems to have a wider scope and not a final decision if only the label field of an entity or the whole entity should be validated. My personal opinion is that the whole entity should be validated, but there have been changes in core leading to restricting the validation failures a user could see only to the fields that are in the form display and the user has access to.

The current approach introduced an endless validation loop for the case where two entities are referencing each other, therefore in the new patch I am introducing a termination condition.

The patch should be ready for a review now :).

I've created a dedicated issue for the problem with the recursive validation of entities - #2935405: Recursive validation is broken.

I've just realized that referenced entities might have been already validated somewhere else, which we are doing in our custom module for inline references and don't want that new entities are being validated twice. Therefore the current issue becomes dependent on #2890333: Allow access to CEB::$validated from outside of the entity and also needs an additional method for reseting the validated flag.

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

   1. I'm afraid this will need dedicated test coverage
   2. I don't quite understand this reasoning, and it's not 100% accurate at the very least. In particular the context does not get "injected" in the usual sense of constructor injection. RecursiveContextualValidator contains this:
      

            $validator = $this->constraintValidatorFactory->getInstance($constraint);
            $validator->initialize($this->context);
            $validator->validate($value, $constraint);
      

      This seems as though it would prevent exactly what you are describing as a problem here. So (assuming this is in fact needed) the comment needs to more clearly describe how a validator can become stale.

2. +++ b/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceItemTest.php
   @@ -94,6 +96,14 @@ protected function setUp() {
   +    CommentType::create([
   
   @@ -391,6 +401,10 @@ public function testAutocreateValidation() {
   +    NodeType::create([
   

   Minor, but I think it would be more consistent if both were in setUp()

3. +++ b/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceItemTest.php
   @@ -477,6 +491,7 @@ public function testAutocreateValidation() {
   +      'field_name' => 'field_test_comment',
   

   This is actually not correct. This should be the field name of an actual comment field (i.e. CommentItem), so we should add one of those to EntityTest and then use the name of that here instead.

   The reason that still works is that the field_name field is not properly validated currently, but as soon as we fix that this would start to fail.

4. +++ b/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceItemTest.php
   @@ -485,6 +500,10 @@ public function testAutocreateValidation() {
   +    // Attach the entity this comments belongs to, otherwise the validation will
   +    // fail as the entity_id field is required.
   +    $comment->set('entity_id', ['entity' => $entity]);
   

   I think this actually points to a larger issue. Comment implements something like a "backreference" but that is inherently not compatible with the validation of unsaved, in-memory entities that is being introduced here. In this particular case we are lucky because $entity has already been saved, but that's not necessarily the case. Maybe $entity itself was just created at runtime as part of an entity reference field of a $parent_entity. Assuming that the inner entity reference field - field_test_comment - in this case is required, we have an invalidatable entity: If we don't provide a comment entity, the required field is empty and fails validation; if we do provide a comment it needs to be valid which means it needs its parent ID, which does not yet exist.

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -102,6 +104,74 @@ public function validate($value, Constraint $constraint) {
            $valid_new_entities = $handler->validateReferenceableNewEntities($new_entities);
            $invalid_new_entities = array_diff_key($new_entities, $valid_new_entities);
   ...
   +        // The entities which could be created should run through the entity
   +        // validation before being saved.
   +        foreach ($valid_new_entities as $delta => $entity) {
   

   I think these variables should be renamed and possible we should have some code explaining the new situation: We now have 2 notions of "valid" in the same method: 1. The selection handler says that the entity meets its restrictions 2. The entity is inherently valid according to its own constraints.

   Currently it is very confusing to have something called $valid_new_entities be run through validation.

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -102,6 +104,74 @@ public function validate($value, Constraint $constraint) {
   +          $parent_entity->recursiveValidationRunning = TRUE;
   

   I think the "recursive" part of the name is not necessary, $validationRunning is explanatory enough, in my opinion. If we do need this, though, we should make it "offical":

   1. Add a real property with a getter method in ContentEntityBase
   2. Set it appropriately in ContentEntityBase::validate() before and after the actual validation

3. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -102,6 +104,74 @@ public function validate($value, Constraint $constraint) {
   +            // If the referenced entity has been validated somewhere else then
   +            // we don't need to validate it again, as code validating the entity
   +            // and leaving the validated flag set to TRUE is required of taking
   +            // care of the errors.
   +            if ($entity->isValidated()) {
   +              continue;
   +            }
   

   It's not clear to me whether this is just a performance optimization or whether it is actually needed. If the former, could we maybe split it to a separate issue? If the latter, maybe we could describe in more detail why this is needed.

4. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -102,6 +104,74 @@ public function validate($value, Constraint $constraint) {
   +            else {
   

   Minor, but the else is not needed. Removing it actually makes it slightly clearer that $violations will always be defined below, which I did not realize at first.

5. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -102,6 +104,74 @@ public function validate($value, Constraint $constraint) {
   +          if ($violations->count() != 0) {
   

   Minor, but we could use strict equality here.

6. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -102,6 +104,74 @@ public function validate($value, Constraint $constraint) {
   +            // Concatenate the violations to show them as a single violation.
   

   Why do we do this? Would it not make more sense to prepend the current path to the violation and then add it to the current context? I.e.

   $this->context->buildViolation(...)
     ...
     ->atPath($delta . '.entity.' . $violation->getPath());
   

   or something like that.

7. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -102,6 +104,74 @@ public function validate($value, Constraint $constraint) {
   +            $reasons = implode("\n", $reasons);
   

   Does the "\n" end up in the UI, as well? If so, that seems problematic.

8. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -102,6 +104,74 @@ public function validate($value, Constraint $constraint) {
   +            // If the entity supports bundles, then add the bundle label to the
   +            // type label.
   

   This logic feels a bit out-of-place here, to be honest. Can we not just use $entity_type->getBundleLabel() or something like that?

9. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -102,6 +104,74 @@ public function validate($value, Constraint $constraint) {
   +                $type_label .= ' ' .$bundle_field->getString();
   

   There should be space after the concatenation.

10. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
    @@ -102,6 +104,74 @@ public function validate($value, Constraint $constraint) {
    +              ->atPath((string) $delta . '.entity')
    

    Minor, but the casting is not necessary here.

Re #57:

1.1. Yes, we would really need additional test coverage for that.

1.2. Well I consider $validator->initialize($this->context); as an injection and to be specific it is a setter injection, but I don't mind being more specific. What I mean is, that the context is being inserted into the validator through the setter injection. This is where the problem occurs when the validator object is reused, because if an entity validator itself executes the validate method on a referenced entity, then that same validator will be used for the validation of the referenced entity and during the validation of the referenced entity the context will be exchanged / newly set / re-initialized, and when the validation of the referenced entity is finished and the call stack is back to the place where we've started the validation of the referenced entity then the validator doesn't have the original context, but the one that has been set during the validation of the referenced entity. I hope that this is more clear now and if you still doesn't like the comment probably you could try to paraphrase it?

2. Done, at least because of the re-roll :).

3.

This is actually not correct. This should be the field name of an actual comment field (i.e. CommentItem), so we should add one of those to EntityTest and then use the name of that here instead.

Hm, but we already do in \Drupal\Tests\field\Kernel\EntityReference\EntityReferenceItemTest::setUp():
$this->createEntityReferenceField('entity_test', 'entity_test', 'field_test_comment', 'Test comment entity reference', 'comment');

4.

I think this actually points to a larger issue. Comment implements something like a "backreference" but that is inherently not compatible with the validation of unsaved, in-memory entities that is being introduced here. In this particular case we are lucky because $entity has already been saved, but that's not necessarily the case. Maybe $entity itself was just created at runtime as part of an entity reference field of a $parent_entity. Assuming that the inner entity reference field - field_test_comment - in this case is required, we have an invalidatable entity: If we don't provide a comment entity, the required field is empty and fails validation; if we do provide a comment it needs to be valid which means it needs its parent ID, which does not yet exist.

While you are right, I don't think that this case is a problem of the current issue. Actually I am not sure if it is a real problem at all. If both fields are required - the comment field holding a reference to the parent entity and the field of parent the entity holding a reference to the comment, then no matter how you look at it you cannot save, especially if you specify the field in the DB to be NOT NULL, in which case the current issue should properly fail the validation, otherwise there will be an exception thrown by the storage.

Re #58:

1.

I think these variables should be renamed and possible we should have some code explaining the new situation: We now have 2 notions of "valid" in the same method: 1. The selection handler says that the entity meets its restrictions 2. The entity is inherently valid according to its own constraints.

Currently it is very confusing to have something called $valid_new_entities be run through validation.

I've renamed the variables:

$valid_new_entities => $referenceable_new_entities
$invalid_new_entities => $non_referenceable_new_entities

2. Done.

3.

It's not clear to me whether this is just a performance optimization or whether it is actually needed. If the former, could we maybe split it to a separate issue? If the latter, maybe we could describe in more detail why this is needed.

We need this because if the entity has been validated somewhere else e.g. an inline reference widget or something else, then the place where the entity was validated should take care of showing the errors. If we ignore the fact that the entity has been validated, then we might show the errors twice. The behaviour was already documented as part of the method documentation of \Drupal\Core\Entity\FieldableEntityInterface::validate():

* Note that by calling this method the entity will be flagged as validated
* and validation logic might be skipped, because ::isValidated() would return
* TRUE. To prevent this the validated flag could be reset by calling
* ::setValidated(FALSE).

Still I've explained this inline in more detail.

4. Done.

5. Done, but I am not sure that it makes a difference here.

6. Thank you, this makes much more sense.

7. Not important anymore due to the change in 6..

8.

This logic feels a bit out-of-place here, to be honest. Can we not just use $entity_type->getBundleLabel() or something like that?

Well here we need the name of the bundle entity itself and not the label for the bundle, which in case of the node entity type is "Content type". If you have any other suggestion, I'll be happy to implement it. Unfortunately I don't think that we have an API for this.

9. Done.

10. Done.

Actually we have the test already which proves that the validation errors of the referenced entities do not propagate through the recursive validation :). I've added more comments to the test to describe that.

Now the patch should be ready for a review :). I've updated the issue summary.

1. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityInterface.php
   @@ -212,12 +212,44 @@ public function onChange($field_name);
   +   * Note that by calling this method the entity will be flagged as validated
   +   * and validation logic might be skipped, because ::isValidated() would return
   +   * TRUE. To prevent this the validated flag could be reset by calling
   +   * ::setValidated(FALSE).
   

   If we want to mention this, I think we should go into a bit more in detail, because "might be skipped" does not really help much, I think. The thing is that it will not be validated even if setValidationRequired() will be called (e.g. by a form).

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -91,25 +93,87 @@ public function validate($value, Constraint $constraint) {
   +            if ($entity->isValidationRunning()) {
   +              continue;
   +            }
   

   It's very unfortunate to be adding this API just for this use-case. I'm wondering if something similar to FieldFormatter\EntityReferenceEntityFormatter::RECURSIVE_RENDER_LIMIT
   could work as well? It's certainly not as nice or generic, but it seems something like that could still solve the use-case here.

3. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -91,25 +93,87 @@ public function validate($value, Constraint $constraint) {
   +            $violations = $entity->validate();
   ...
   +            $violations = $entity->getTypedData()->validate();
   

   Unless I am missing something, the latter call should work in both cases so can we just move it outside of the conditional? I think that should allow to get rid fo the else.

4. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -91,25 +93,87 @@ public function validate($value, Constraint $constraint) {
   +            // If the entity supports bundles, then add the bundle label to the
   +            // type label.
   +            if ($entity_type->hasKey('bundle')) {
   +              /** @var \Drupal\Core\Field\FieldItemListInterface $bundle_field */
   +              $bundle_field = $entity->{$entity_type->getKey('bundle')};
   +              if ($bundle_field instanceof EntityReferenceFieldItemList) {
   +                $type_label .= ' ' . $bundle_field->entity->label();
   +              }
   +              else {
   +                $type_label .= ' ' . $bundle_field->getString();
   +              }
   

   This is a bit weird, in my opinion. First of all the check for EntityReferenceFieldItemList is a bit unfortunate. But also $bundle_field->getString() is strange. The documentation mentions the "bundle label", so should this be $entity_type->getBundleLabe()? Or maybe the label of the bundle field (definition)?

5. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -91,25 +93,87 @@ public function validate($value, Constraint $constraint) {
   +        unset($parent_entity->recursiveValidationRunning);
   

   This is the only mention of this variable in the patch. So, either this is unnecessary or there is missing test coverage ;-)

6. +++ b/core/lib/Drupal/Core/Validation/ConstraintValidatorFactory.php
   @@ -25,12 +25,13 @@ public function __construct(ClassResolverInterface $class_resolver) {
   -    if (!isset($this->validators[$class_name])) {
   -      $this->validators[$class_name] = $this->classResolver->getInstanceFromDefinition($class_name);
   -    }
   -
   -    return $this->validators[$class_name];
   +    // 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);
   

   Seems there is no dedicated test for this.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -478,9 +485,34 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
   +  public function isValidated() {
   +    return $this->validated;
   +  }
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setValidated($validated) {
   +    $this->validated = $validated;
   
   +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -91,25 +93,87 @@ public function validate($value, Constraint $constraint) {
   +            // If a referenced entity is currently being validated then we've
   +            // encountered a cyclic reference and therefore should skip starting
   +            // the validation of the entity again in order to prevent an endless
   +            // loop.
   +            if ($entity->isValidationRunning()) {
   +              continue;
   +            }
   +
   +            // If the referenced entity has been validated somewhere else then
   +            // we shouldn't validate it again, as code validating the entity and
   +            // leaving the validated flag set to TRUE is required of taking care
   +            // of the validation errors. If we ignore the fact that the entity
   +            // is already validated, then we might show the same errors multiple
   +            // times. In order to prevent this whoever is validating the entity
   +            // is also responsible for showing the errors or resetting the
   +            // validated flag of the entity.
   +            // @see \Drupal\Core\Entity\FieldableEntityInterface::validate()
   +            // @see \Drupal\Core\Entity\FieldableEntityInterface::setValidated()
   +            if ($entity->isValidated()) {
   +              continue;
   +            }
   +            $violations = $entity->validate();
   

   May I ask whether we could put this logic into the entity itself? Could we check on the entity whether it was validated already. I guess we would need to invalidate the flag everytime onChange is called.

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   index 4602c206fb..df9064e6ab 100644
   --- a/core/lib/Drupal/Core/Validation/ConstraintValidatorFactory.php
   
   --- a/core/lib/Drupal/Core/Validation/ConstraintValidatorFactory.php
   +++ b/core/lib/Drupal/Core/Validation/ConstraintValidatorFactory.php
   
   +++ b/core/lib/Drupal/Core/Validation/ConstraintValidatorFactory.php
   +++ b/core/lib/Drupal/Core/Validation/ConstraintValidatorFactory.php
   @@ -25,12 +25,13 @@ public function __construct(ClassResolverInterface $class_resolver) {
   
   @@ -25,12 +25,13 @@ public function __construct(ClassResolverInterface $class_resolver) {
       */
      public function getInstance(Constraint $constraint) {
        $class_name = $constraint->validatedBy();
   -
   -    if (!isset($this->validators[$class_name])) {
   -      $this->validators[$class_name] = $this->classResolver->getInstanceFromDefinition($class_name);
   -    }
   -
   -    return $this->validators[$class_name];
   +    // 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);
   

   At this point the base class doesn't give us anything anymore. Would it make sense to get rid of it? Maybe in a followup?

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ValidReferenceConstraintValidatorTest.php
   @@ -231,4 +232,74 @@ public function testPreExistingItemsValidation() {
   +   * Tests the validation of new entities on an entity reference field.
   +   *
   +   * Additionally the test covers the recursive validation. The constraint
   +   * validator "ValidReferenceConstraintValidator" will be executed for the
   +   * parent entity and will trigger the validation of the referenced entities,
   +   * which will lead to executing the same validator for each referenced entity.
   +   * In order for the validation errors of the referenced entities to propagate
   +   * it shouldn't happen that the same constraint validator object of the parent
   +   * is used for the referenced entities. If the constraint validator object is
   +   * reused during the recursive validation then its execution context will be
   +   * exchanged when validating the referenced entities, which will not allow for
   +   * their validation errors to propagate.
   +   */
   +  public function testRecursiveValidation() {
   +    $this->installEntitySchema('entity_test_constraint_violation');
   

   👍Amazing test coverage

Rerolled for #2996789: Deprecate Drupal\field\Tests\EntityReference\EntityReferenceTestTrait.

I have tested the patch in #67 against Drupal 8.6.17 and it's working as expected. Thanks for the patch!

In my use case, I have an entity reference field to a Taxonomy Vocabulary "Mboxes". I have added a custom constraint to validate the term name of that vocabulary.

The only concern I have is about the error message. As the "invalidNewEntity" error use % emphasise showing the error message from other constraints. When % is also used in the other constraints, they will be printed as plain HTML with <em></em>.

  /**
   * Violation message when a new entity doesn't pass the validation.
   *
   * @var string
   */
  public $invalidNewEntity = 'This entity (%type: %label) cannot be created. Reason(s): "%reason".';

Here is my example

I am wondering if we could change %reason to :reason and remove double quote and the full stop?

Also if there is any way we could customise the error message. As that makes sense to developers but not intuitive to end users. In my case I just want to show my custom constraint error.

Queued test against 8.9. We should still fix this!

Test required a fix due to #2885809: The 'entity_type' and 'field_name' base fields on Comment are required

Replying #64:
1.

If we want to mention this, I think we should go into a bit more in detail, because "might be skipped" does not really help much, I think. The thing is that it will not be validated even if setValidationRequired() will be called (e.g. by a form).

Updated comment.

2.

It's very unfortunate to be adding this API just for this use-case. I'm wondering if something similar to FieldFormatter\EntityReferenceEntityFormatter::RECURSIVE_RENDER_LIMIT
could work as well? It's certainly not as nice or generic, but it seems something like that could still solve the use-case here.

Well, it's not always easily predictable, which API may prove useful. At the moment, yes, it use just within ValidReferenceConstraintValidator, and could be technically linked to it. But we cannot exclude the possibility, of, say, another reference validator, which will need to know if the entity is already being validated.
3.

Unless I am missing something, the latter call should work in both cases so can we just move it outside of the conditional? I think that should allow to get rid fo the else.

With this patch FieldableEntityInterface::validate() takes care also of setting and unsetting entity's in validation state, so those calls are different.
4.

This is a bit weird, in my opinion. First of all the check for EntityReferenceFieldItemList is a bit unfortunate. But also $bundle_field->getString() is strange. The documentation mentions the "bundle label", so should this be $entity_type->getBundleLabe()? Or maybe the label of the bundle field (definition)?

Technically, bundle field need not be entity reference field, though it normally is (see ContentEntityBase::baseFieldDefinitions()). And what you are talking here will get us bundle field name, not the bundle name, imho.
5.

This is the only mention of this variable in the patch. So, either this is unnecessary or there is missing test coverage ;-)

This seems to be a leftover from previous commits. Now FieldableEntityInterface::validate() takes care of setting and unsetting this flag, so I think it's safe to remove this line.
6.

Seems there is no dedicated test for this.

Now there's a dedicated issue for that ) #3096811: Reusing initialized constraint validators overwrittes validation errors And this part is removed from current patch.

+++ b/core/lib/Drupal/Core/Entity/FieldableEntityInterface.php
@@ -213,9 +213,10 @@
+   * and following attempts to validate, either by calling validate() directly
+   * or by setting setValidationRequired (e.g., by form). will be skipped,
+   * because ::isValidated() would return TRUE. To prevent this the validated
+   * flag could be reset by calling ::setValidated(FALSE).

"by calling validate() directly or by setting setValidationRequired (e.g., by form)".
validate() is the method we are currently talking about. Referencing to it like this makes it sound as if are talking about a different method.

I suggest something like this:

"and following attempts to validate will be skipped until ::isValidated() returns TRUE. Therefore flagging the entity for requiring validation through ::setValidationRequired() after it has been validated once will not have any effect. If a subsequent validation is desired then the validated state should be reset by calling ::setValidated(FALSE)."

However this makes me think whether we should call ::setValidated(FALSE) from within ::setValidationRequired(). It kind of makes sense to reset the validated flag if someone is flagging the entity for validation required even if it was validated already.

----
Edit:

I've just realized that we've only documented this on the method in the interface, but haven't changed the implementation accordingly. Thinking over it again I am sure that this will be a BC break.

If one doesn't want to repeat the validation then it should be first checked through ::isValidated() whether the entity has been validated already.

Let's remove this comment then.

Re #65:

1.

May I ask whether we could put this logic into the entity itself? Could we check on the entity whether it was validated already. I guess we would need to invalidate the flag everytime onChange is called.

The only possible way for this would be to add an optional parameter $skip_if_validated = FALSE. The documentation from my previous comment will then make sense when describing the TRUE value of this parameter. When set to TRUE then we should simply return an empty list.
And yes it makes sense to reset the validated flag in ::I change().

2.
We can discuss about the future of the class in the follow-up that has been created in #3096811: Reusing initialized constraint validators overwrittes validation errors - #3097071: Find out how to reuse validator objects in recursive validation.

3.
I am glad you like it :).

-----

Edit:
1) Unfortunately adding new parameters even optional is BC break as of PHP 7.2 because overrides are required to have the same definition.
What if we keep the code in the validator for now, reset the validated flag in ::onChange() and create a follow-up to add the parameter to ::validate() and update our code here in the follow-up?

Added resetting 'validated' flag to FALSE in onChange() and setValidationRequired(), and removed the comment on validate() method.

But, let me express my opinion. We need to skip validation on already validated for one main reason: to avoid reporting multiple violations at one place. For blocking circular references, isValidationRunning() is responsible. I would approach this by storing the results of last validation with the entity and filtering the violation list within ValidReferenceConstraintValidator to remove the duplicates. Changing the validation() method behavior and breaking its idempotency (hope I spelled it correct) is a bit of overkill and may have side-effects that are not obvious now.
UPD: created a separate issue #3097399: Store last validation result with the entity

#3096811: Reusing initialized constraint validators overwrittes validation errors  has been committed :). The patch should pass now.

Tested on Drupal 9.2.7, it works fine.

Finally my custom constranint for taxonomy term works in entity reference field. :)

The last patch(#77) failed to apply on 9.4, so re-rolled it for 9.4

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

+  public function isValidationRunning() {
+    return $this->validationRunning;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function isValidated() {
+    return $this->validated;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function setValidated($validated) {
+    $this->validated = $validated;
+    return $this;
   }

1. these functions should be typehinted

2. Will need a change record for the new functions on the interface (as they may be used by others)

Since this is adding a new error message tagging for usability review. Can bring up in the #ux channel in slack to bring up early.

Curious though if there's a good way to test this one?