EntityFormDisplay::validateFormValues sets form state errors by empty name string instead using the provided form element

I did a bit of digging around this code and it seems that the ->setErrorByName() call with an empty string as the first argument was introduced in #2395831-102: Entity forms skip validation of fields that are not in the EntityFormDisplay, where it replaced a call on an actual element path: $form_state->setErrorByName(str_replace('.', '][', $violation->getPropertyPath()), $violation);.

So I think that the patch is correct by assigning the error to the $form element instead, because this indeed takes care of complex use cases and allows embedded entity forms to report errors correctly.

I think it is worth having test coverage here because

this indeed takes care of complex use cases and allows embedded entity forms to report errors correctly.

The test looks great! Just a few minor things that should be fixed before RTBC:

1. +++ b/core/modules/field/src/Tests/NestedFormTest.php
   @@ -166,4 +166,35 @@ function testNestedFieldForm() {
   +  function testNestedEntityFormEntityLevelValidation() {
   

   Missing member visibility, i.e. 'public' :)

2. +++ b/core/modules/field/src/Tests/NestedFormTest.php
   @@ -166,4 +166,35 @@ function testNestedFieldForm() {
   +      ->create(['id' => 1]);
   +    $entity_1->enforceIsNew();
   ...
   +      ->create(['id' => 2]);
   +    $entity_2->enforceIsNew();
   

   This enforceIsNew() is not really needed, you can just call the id() method on both entities when you need it below.

3. +++ b/core/modules/field/src/Tests/NestedFormTest.php
   @@ -166,4 +166,35 @@ function testNestedFieldForm() {
   +    $this->assertFieldByName('entity_2[changed]', 0, 'Entity 2: changed value appears correctly is the form.');
   

   "in" the form :)

4. +++ b/core/modules/field/tests/modules/field_test/src/Form/NestedEntityTestForm.php
   @@ -33,16 +34,26 @@ public function buildForm(array $form, FormStateInterface $form_state, EntityInt
   +      // @see Drupal\field\Tests\NestedFormTest::testNestedEntityFormEntityLevelValidation.
   
   @@ -65,7 +76,16 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +    // @see Drupal\field\Tests\NestedFormTest::testNestedEntityFormEntityLevelValidation.
   

   I think our coding standards say that we need to put () at the end of method names in code references.

Awesome, looks great now :)

Committed and pushed 505eb55 to 8.3.x and 6633476 to 8.2.x. Thanks!

1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
   @@ -236,7 +236,7 @@ public function validateFormValues(FieldableEntityInterface $entity, array &$for
        foreach ($violations->getEntityViolations() as $violation) {
          /** @var \Symfony\Component\Validator\ConstraintViolationInterface $violation */
   -      $form_state->setErrorByName('', $violation->getMessage());
   +      $form_state->setError($form, $violation->getMessage());
        }
   

   This is not introduced here but you only set one error per element - see \Drupal\Core\Form\FormState::setErrorByName(). So I think this code is flawed but it is less flawed than before.

2. +++ b/core/modules/field/src/Tests/NestedFormTest.php
   @@ -52,7 +52,7 @@ protected function setUp() {
   -  function testNestedFieldForm() {
   +  public function testNestedFieldForm() {
   

   Whilst correct, this change is out of scope. Reverted on commit.

We should have a follow-up for #22.1