Content entity form validation does not respect the #limit_validation_errors property from field widgets

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -179,10 +179,28 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +    if ($limit_validation_errors = $form_state->getLimitValidationErrors()) {
   +      foreach ($limit_validation_errors as $section) {
   +        $field_name = reset($section);
   +        if ($entity->hasField($field_name)) {
   +          $edited_fields += [$field_name];
   +        }
   +      }
   +    }
   +    else {
   +      $edited_fields = $this->getEditedFieldNames($form_state);
   +    }
   +
   

   Nice. @amateescu++

   Wondering if we could make this a bit shorter by always starting with the edited field names, an array filter on the limit validation errors and then an array intersect but that might not actually be easier to understand and quite possibly slower as when we have limit validation errors, it is almost always for a single field. So this seems fine.

2. +++ b/core/tests/Drupal/FunctionalTests/Entity/ContentEntityFormFieldValidationFilteringTest.php
   @@ -0,0 +1,159 @@
   +
   +  use TestFileCreationTrait {
   +    getTestFiles as drupalGetTestFiles;
   

   do we care about renaming the method if we specifically use the trait, I think we usually just do that for BC in the base classes?

3. +++ b/core/tests/Drupal/FunctionalTests/Entity/ContentEntityFormFieldValidationFilteringTest.php
   @@ -0,0 +1,159 @@
   +  /**
   +   * Tests field widgets with #limit_validation_errors.
   +   */
   +  public function testFieldWidgetsWithLimitedValidationErrors() {
   +    $assert_session = $this->assertSession();
   

   Does it make sense to also verify that we do get validation errors when e.g. trying to save now with an empty required field?

   I guess we have plenty of tests that already do that, but might not hurt to do it in the context of this scenario too?

Looks good to me and fix tons of issues.

subscribe

I temporarily solved this removing the following 4 lines:

/*
    // Remove violations of inaccessible fields and not edited fields.
    $violations
      ->filterByFieldAccess($this->currentUser())
      ->filterByFields(array_diff(array_keys($entity->getFieldDefinitions()), $this->getEditedFieldNames($form_state)));

    $this->flagViolations($violations, $form, $form_state);
*/        

Works great then.

But removing this 4 lines (see #10) is much easier.
Hoping for a solution in core.

@amateescu suggested that #2784015: Widget validation crashes on ItemList violations for widgets with a custom errorElement() implementation would be fixed by this issue but the latest patch here doesn't fix that so either this needs some more work or that other issue can be reopened.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -179,10 +179,28 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+          $edited_fields += [$field_name];

Minor but what's wrong with $edited_fields[] here? Do we really need the explicit +=, and if so could use a comment explaining why.

Thanks that's a bit more explicit.

Committed 12a28a5 and pushed to 8.5.x. Thanks!

We're in a freeze for the 8.4.0 release, I think this is backportable for 8.4.1 though so leaving RTBC to cherry-pick later.

Can we get this reopened/RTBC so it doesn't fall off the radar for 8.4.1? Just ran into this issue and see it is auto-closed. Thanks!