Add a validation constraint to check if an entity has a field

Here's a quick review :)

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityFieldConstraint.php
   @@ -0,0 +1,56 @@
   + *   id = "EntityField",
   

   How about renaming this to EntityHasField?

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityFieldConstraint.php
   @@ -0,0 +1,56 @@
   + *   type = { "entity", "entity_reference" },
   

   Not sure why we need "entity_reference" in there, this constraint doesn't seem to do anything with references.

3. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityFieldConstraint.php
   @@ -0,0 +1,56 @@
   +  public function getFieldOption() {
   +    // Support passing the field name as string, but force it to be an array.
   +    return (array) $this->field_name;
   +  }
   

   If the output is an array, shouldn't the method name be getFieldOptions()?

4. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityFieldConstraintValidator.php
   @@ -0,0 +1,30 @@
   +    if (!($entity instanceof FieldableEntityInterface)) {
   +      return;
   +    }
   

   Shouldn't we expose this state as a constraint violation? Something like "The entity is not fieldable."..

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityFieldConstraintValidatorTest.php
   @@ -0,0 +1,103 @@
   +   * @param string|string[] $field
   ...
   +  public function testValidation($field, array $expected_violations) {
   ...
   +      ->addConstraint('EntityField', $field);
   

   Let's rename this argument to $field_names and always pass an array :)

Looks great, this is all I got:

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityHasFieldConstraint.php
   @@ -0,0 +1,63 @@
   +  /**
   +   * The field name option.
   +   *
   +   * @var string|array
   +   */
   +  public $field_name;
   ...
   +  public function getFieldOptions() {
   +    // Support passing the field name as string, but force it to be an array.
   +    return (array) $this->field_name;
   +  }
   

   I think the feedback was intended as: lets update the constraint to only accept a list of fields instead of both. If we do indeed support both a string and a list, we should have coverage for it.

   If we do take that direction though, EntityHasFields is probably a better name. I question the need for this at all though, surely it's trivial to add multiple constraints to something in pretty much all contexts where they are used?

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityHasFieldConstraintValidatorTest.php
   @@ -0,0 +1,123 @@
   +/**
   + * Tests validation constraints for EntityHasFieldConstraintValidator.
   + *
   

   I like connecting dots here with a @coversDefaultClass, which pretty much makes the comment redundant.

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityHasFieldConstraintValidatorTest.php
   @@ -0,0 +1,123 @@
   +    $node = $this->container->get('entity_type.manager')
   +      ->getStorage('node_type')
   +      ->create(['type' => 'article']);
   

   Super nit, this variable is probably more accurate as $node_type. Also any reason not to simply NodeType::create()?

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityHasFieldConstraintValidator.php
   @@ -0,0 +1,31 @@
   +    if (!($entity instanceof FieldableEntityInterface)) {
   

   We should have an early return before this check, something like:

       if (!isset($entity)) {
         return;
       }
   

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityHasFieldConstraintValidatorTest.php
   @@ -0,0 +1,123 @@
   +    $node = $this->container->get('entity_type.manager')
   +      ->getStorage('node')
   +      ->create(['type' => 'article']);
   

   We could use NodeCreationTrait and $this->createNode() here :)

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityHasFieldConstraintValidatorTest.php
   @@ -0,0 +1,123 @@
   +    $node = $this->container->get('entity_type.manager')
   +      ->getStorage('node_type')
   +      ->create(['type' => 'article']);
   

   Related to @Sam152's point above, we should use $this->createContentType() instead.

I'm also wondering about #7.1: does this constraint really need the ability to handle multiple field names?

This patch would solve the issue.

The patch looks much better now. Just a little note about the test coverage:

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityHasFieldConstraintValidatorTest.php
@@ -0,0 +1,106 @@
+    // Create a typed data definition with an EntityHasField constraint.
+    $definition = DataDefinition::create('entity_reference')
+      ->addConstraint('EntityHasField', $field_name);
...
+    // Create a typed data definition with an EntityHasField constraint.
+    $definition = DataDefinition::create('entity_reference')
+      ->addConstraint('EntityHasField', 'body');

It's a bit weird to add this constraint to a data definition of an entity reference field, isn't the constraint supposed to live on the entity type level?

I would suggest to follow the same approach as \Drupal\KernelTests\Core\Entity\EntityTypeConstraintsTest for testing this, and use the existing entity_test_constraints module :)

That looks much better indeed!

Should this also check for the field type? Or if not, what is going to check that?

Also this seems like it could potentially help with #2981835: Forums hard-codes the comment_forum comment field, but doesn't validate it.

i'm wasn't 100% on committing this without field type checking, but the layout example uses a very clearly namespaced field, and we already have issues with forum module relying on a field name and doing no checking at all.

So this is an improvement, and phenaproxima updated the change record to note the limitations.

Committed 28be9f7 and pushed to 8.6.x. Thanks!