Improve the documentation of WidgetBase::errorElement() for mapping violation property paths to form elements

@fago: Is it ? I'm afk, bit iirc that other patch just paases ViolationList objects directly as a param to widget::flagErrors() instead of passing it through $form_state, but otherwise doesn't change anything regarding error paths ?

This is blocking #2509268: Inline errors repeated on child elements in module uninstall form, bumping.

Wow, so $violation->arrayPropertyPath doesn't actually exist on the violation object, we just randomly assign it there in WidgetBase.
Didn't even know PHP allowed that.

Here's an initial patch. It stops passing $violation->arrayPropertyPath, and introduces a new method argument instead.
It also returns the correct error element.
No test work yet.

Friday afternoon intelligence. This time without the syntax error.

$error and $violation were inconsistently used in the method signature, and I made it worse. Now fixed.

This one should be green, but we'll want to introduce additional test coverage.

Then we should stop assigning $violation->arrayPropertyPath altogether, rather than assigning it and then unassigning it a couple lines below ?

Would mean building $violations_by_delta as

[delta] => [ 
  ['violation' => $violation, 'property_path" => $property_path],
  ['violation' => ..., 'property_path' => ...],
  ...
]  

rather than

[delta] => [
  $violation_with_a_temp_arrayPropertyPath,
  $violation_with_a_temp_arrayPropertyPath,
  ...
]

But more generally, it feels weird to have a method with params :
- $property_path (as an array skipping the delta)
- $violation that has a getPropertyPath() method that returns something slightly different (a string, starting with the delta)

Maybe we could remove the new param, and instead provide a dedicated helper method in WidgetBase, $this->getLocalErrorPath($violation), that implementations of errorElement($violation) would call ?

Sorry for letting this slip. Just wanted to also point out that we cannot dereference arrayPropertyPath/propertyPath, since it is somethings empty.
#2553983: Required textarea with summary breaks ajax events for other fields.

Removed $violation->arrayPropertyPath and added WidgetBase::getLocalErrorPath(ConstraintViolationInterface $violation).
Included fix from https://www.drupal.org/node/2553983.

Ideally we would have test coverage for some of the following cases: a) a widget handling multiple form elements b) a widget on a field with single cardinality b) a widget on a field with multiple cardinality, so we are sure that its passed along properly to all of them.

Agreed with #19, otherwise patch looks good to me :-)

dbolinovski has started work on tests, I've told him to upload his progress the next time he's online.

TextareaWithSummaryWidget::errorElement() changes. Added test cases.

1. +++ b/core/modules/system/src/Tests/Validation/WidgetBaseErrorElementTest.php
   @@ -0,0 +1,224 @@
   + * Definition of Drupal\system\Tests\Validation\WidgetBaseErrorElementTest.
   

   Should be 'Contains \Drupal ...'

2. +++ b/core/modules/system/src/Tests/Validation/WidgetBaseErrorElementTest.php
   @@ -0,0 +1,224 @@
   + * Tests if the validation violation property paths are properly passed on to widgets.
   

   80 chars

3. +++ b/core/modules/system/src/Tests/Validation/WidgetBaseErrorElementTest.php
   @@ -0,0 +1,224 @@
   +    // Post 3 email addresses, set errorneous email address in second field and check form errors.
   

   80 chars

4. +++ b/core/modules/system/src/Tests/Validation/WidgetBaseErrorElementTest.php
   @@ -0,0 +1,224 @@
   +    // Now set errorneous email address in first and third fields and check form errors.
   

   80 chars

I will address feedback in #24.

 /**
  * @file
- * Definition of Drupal\system\Tests\Validation\WidgetBaseErrorElementTest.
+ * Definition of \Drupal\system\Tests\Validation\WidgetBaseErrorElementTest.
  */

"Definition of" still needs to become "Contains" :)

+ * Tests if the validation violation property paths are properly passed on to
+ * widgets.
  *
  * @group address
  */

We can go with something shorter perhaps. "Tests the validation violation property paths."
And the @group is clearly bogus.

dbolinovski was unsure about the test location and naming, would appreciate a comment there.

Feedback from #27 has been addressed except for the invitation to comment on the location and name of the test.

Addressing Bojan’s feedback, changed variable names to snake_case, removed assertion from setUp(), plugin annotation changes.

Drupal\system\Tests\Validation is not ideal IMO, the tests in there are about our validation API, this is about field API widgets.
So I'd suggest putting it in Drupal\system\Tests\Field\Widget.

Other than that, looks good :-)

Addressing #37, test moved in Drupal\system\Tests\Field\Widget.

Addressing #37, test moved in Drupal\system\Tests\Field\Widget.

Wait, actually this is wrong :

+++ b/core/lib/Drupal/Core/Field/WidgetBase.php
@@ -425,7 +425,6 @@ public function flagErrors(FieldItemListInterface $items, ConstraintViolationLis
           $property_path = explode('.', $violation->getPropertyPath());
           $delta = array_shift($property_path);
           $violations_by_delta[$delta][] = $violation;
-          $violation->arrayPropertyPath = $property_path;

@@ -500,10 +498,25 @@ public function settingsSummary() {
+  protected function getLocalErrorPath(ConstraintViolationInterface $violation) {
+    return array_filter(explode('.', $violation->getPropertyPath()));
+  }

Wait, actually that is wrong :)

The code in HEAD does an array_unshift(), which removes the "delta" part of the property path, so that the widget for delta N works with actual local path (that does not start with the delta).

The new code skips that, and getLocalErrorPath() will return a path that begins with "delta".

Mulling on that a bit.

Discussed with @tim.plunkett in Barcelona.

Initially this issue was opened as a task to cleanup a @todo for a not-too-nice but minor and internal hack (how WidgetBase::flagErrors() communicates violation paths that are local to each widget)

This was then risen to a major bug as it was thought to block #2509268: Inline errors repeated on child elements in module uninstall form, which had fails around that code area in WidgetBase, and that code area had a @todo pointing here.

- Fixing that @todo would however not actually fix the fails over there, which have more to do with how the specific UriWidget decides to implement its errorElement() logic (by just keeping the parent implementation), and the patch here wouldn't actually change that.

- The patch here would also be an API break at this point (WidgetInterface::errorElement() implementations couldn't rely on $violation->arrayPropertyPath as they currently do).

- Plus, the patch is currently broken, as it doesn't handle the deltas correctly :-)

So I'm very inclined to "won't fix" this at this point - thus, I'll temptatively do that. Feel free to argue otherwise :-)

So I'm very inclined to "won't fix" this at this point - thus, I'll temptatively do that. Feel free to argue otherwise :-)

And actually doing it...

I understand yched's argumentation, the widget should be responsible for mapping errors to elements because the form structure is not guaranteed to match the property path.

However, this is not obvious at all from the current code, and should be documented.

This problem started to show up more an more with various widgets and people keep trying to "fix" it by changing the errorElement() method of each widget :/

Ok let's see if this works.

Completely new approach which isn't API breaking at all. Added the test from #2614250: Number widget validation can break AJAX actions as test only, full patch and interdiff (which is just the "fix"). Not sure how the rest of the tests will react on this.

Ok, looks that was a little naieve, but also kind of impossible to figure out anyway after some more local testing ..

I tried to summarize why this is a major bug based on the two related issues. Probably could use a more complete summary update.

It makes sense for this to be major if indeed multiple widgets fail validation and cause fatals with AJAX because of it.

This issue is easily reproductible with the paragraphs module.

1. Create a paragraph type with a simple field
2. Add a paragraph field on your content type
3. Add a required number field on the same content type
4. While creating a content of this type, try to add a paragraph

Expected: paragraph type form is embedded in the content type creation form
Current: AjaX error

Latest patch still needs work

Reroll

I hit this just with the core number and core image field:

1. Add a file upload field (not required).
2. Add a numeric field (required) without a default value.
3. Create a node with these two fields.
4. Upload a file.
5. Observe that something goes wrong, ajax error in console, and fatal error in watchdog/syslog.

Luckily, in my case, the file still uploads/attaches properly, but it looks strange on the front end/admin UI.

The core committers and entity and field maintainers discussed this issue awhile back and agreed that it is major on account of the exceptions being triggered through the user interface. #2614250: Number widget validation can break AJAX actions is a specific case of this, also major, and currently postponed on this issue.

Thanks @geerlingguy for confirming the issue.

I've confirmed that #55 resolves this as well as #2614250: Number widget validation can break AJAX actions. It seems that the failing tests are unrelated to this particular patch.

Moving back to "needs review" to re-run tests, and also bumping target version back to 8.3.x since this should not be a disruptive fix.

Re-roll of #55, trying to get tests to run

Oh yeah, the patch in #45/55/62 really is broken. It "fixes" the immediate issue but goes too far and removes legitimate validation (allowing you to e.g. create blocks with duplicate descriptions). Hence the failing tests.

@Dane Powell: Moving this to 8.4.x-dev per my reading of the Backport Policy. Feel free to reset if I have misinterpreted. Also, hello/JIBA!

I believe the proposed fix in this issue will resolve the issue I just ran into, first reported by someone else in #2714347: No way to set error on a field from an entity constraint validator. Updating title to reflect increased impact of this fix (maybe someone can devise a more concise title) and updating the issue summary to reflect the additional major bug this will address.

Makes sense. When we finally get this merged, do I get to say that Jones Wins Again? :)

We're actually blocked into this issue too.

We reproduced the issue following the steps in #59, but the image in our case cannot be uploaded.
The casuistry to reproduce this issue is quite probable (only needs an entity with a file and a numeric field), and the issue prevents users to upload files when this happens. Shouldn't this issue be triaged as critical instead?

I am blocked on this issue as well. I can confirm that with Drupal 8.3.5 and address module 8.x-1.0 that the "AddressFormatConstraintValidator" is not properly throwing violations, even though it correctly calls core's buildViolation().

I have tested patch #62 with no success.

There's a really easy work around here, just make the Number (Integer) fields not-required and it instantly works again.
Not sure if other Number fields affect this.

I'm sorry that my comment from #44 derailed the initial purpose of this issue :/

I've opened a separate issue with a concise summary of the problem that has been discussed in the latest comments and a proposed fix: #2901943: Content entity form validation does not respect the #limit_validation_errors property from field widgets, so let's get this issue back to what it was about before my comment. That would be @bojanz's request from #43 to improve the documentation of \Drupal\Core\Field\WidgetBase::errorElement().

Note that the "inability to set error on a field from an entity constraint validator" part of the current issue title was already fixed by #2894634: Allow entity-level validations to flag errors on form elements.

I've read through this and all these related issues, and while we are making progress with documentation, and this issue does address the issue of setting the correct violation path, it still seems like the underlying issue is that the "if ($error_element !== FALSE) {" check in WidgetBase:L451 ends up letting NULL values through to the '$form_state->setError()' call on the next line.

Shouldn't we make sure that all implementations of errorElement actually return a valid element or FALSE? And/or change the "if ($error_element !== FALSE) {" check to "if ($error_element) {" to prevent NULL values from coming through, e.g. when an error element path can't be determined?

I'm trying to figure out which of all these issues to use to propose a new patch.

Making sure $error_element is actually an array, instead of just !== FALSE.

Let's see what testbot says.

#75 works for me :)

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.