Enhance formErrorHandler to include children errors on RenderElements

using the combined patch from 2346773#57 and errorstyle.module (github) this does pass a manual test run. Details open with children error in Safari 9.0.3

Ok, so can we remove the "Needs tests" tag now? I'm a bit confused by https://www.drupal.org/files/issues/interdiff-2754977-3-9-render_element...

What's the easiest way to test this? Can we get some screenshots up?

@dmsmidt
Fixing @Todo point and adding #array_parent as key.

Whats this "Undefined index: #array_parents"?

Hello,

I worked a bit on it during DrupalCon Dublin 2016. I checked the Test fail messages and added the '#array_parents' key to the tests. As I'm quite new to the difference between the #parents/#array_parrents, I simply copy-pasted the values. In case for test purposes other values should be used instead, please let me know.

I also tried to write a test by asserting the 'children_errors' property on a parent element on the form (in FormErrorHandlerTest under the module's test class) but I keep having "Undefined index" errors.

Hope it helps anyways

Changing status to trigger test bot

See #279246-3: FAPI #array_parents is not documented

+++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
@@ -43,21 +44,64 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
+  protected function setElementErrorsFromFormState(array &$form, FormStateInterface $form_state, array &$elements = array()) {

Is it possible to typehint $form and $elements with interfaces?

+++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
@@ -43,21 +44,64 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
+  protected function setElementErrorsFromFormState(array &$form, FormStateInterface $form_state, array &$elements = array()) {
+    if (empty($elements)) {
+      $elements =& $form;
+    }
+
     // Recurse through all children.
     foreach (Element::children($elements) as $key) {
       if (isset($elements[$key]) && $elements[$key]) {
...
+        $child =& $elements[$key];
+
+        // Call self to traverse up the form tree.
+        $this->setElementErrorsFromFormState($form, $form_state, $child);

What do you think of chainElement...?
or setElementErrorsOnForm($form, ...)

Don't get it yet with $elements, $child and $form

+++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
@@ -43,21 +44,64 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
     // Recurse through all children.
     foreach (Element::children($elements) as $key) {
...
+        $child =& $elements[$key];
...
+          $elements['#children_errors'] = $child['#children_errors'];
...
+          $parents = implode('][', $child['#array_parents']);
+          $elements['#children_errors'][$parents] = $child['#errors'];
...
+          $parents = explode('][', $child['#group']);
...
+            $group_element['#children_errors'] = array_merge($group_element['#children_errors'], $elements['#children_errors']);
...
+          NestedArray::setValue($form, $parents, $group_element);

Can we document the end-result? What will be the structure of the final array?

BTW Only read the code, did not tested yet with your other patches. Stay tuned :)

Some minor remarks on the (otherwise excellent) inline docs:

1. +++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
   @@ -43,21 +44,112 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
   +   * Children errors are always keyed by the #array_parents of the element
   +   * with the error.
   

   I'm afraid this line of documentation is hard to understand.

2. +++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
   @@ -43,21 +44,112 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
   +   * @param array $elements
   +   *   An associative array containing the part of the form structure that will
   +   *   be processed while traversing up the tree. This should be left empty in
   +   *   the case this method isn't called by itself.
   

   The "in the case the method isn't called by itself" confused me, I had to read it 5 times before I got it. I suggest a more direct instruction: "An associative array containing the part of the form structure that will be processed while traversing up the tree. For recursion only; leave empty when calling this method."

Coming up: actual testing (test results will be in #2346773: Details form element should open when there are errors on child elements because they must be tested together).

I found a problem, posted it in #2346773-77: Details form element should open when there are errors on child elements, but the problem turns out to be in this patch here. New patch coming up.

Here's the patch with just the bugfix, I'll see what I can do for the tests.

Re #37, I added a test for this in #2346773-83: Details form element should open when there are errors on child elements. It really was a case of different form elements interacting with each other because of a mistake in the code; I'm not sure if / how that could be covered by the unit tests.

All tests for this change need to be in this issue, we can't rely on a follow-up for that.

I agree, but afaic the separation of those two related issues doesn't help anyway. Can't we merge the issues and fix it in one go?

Marking NW for #41. Thanks all!

As a reminder, this issue is a blocker for Inline Form Errors being marked stable, because it blocks a required fix for that module. We should aim to make progress on this issue before February 1 since it must change stable core code, and therefore can't be changed in patch releases the way that IFE itself can. See #2504847: [meta] Roadmap for stabilizing Inline Form Errors module (IFE) for background.

Looks really good from my point of view

Reviewed it and applied. Also applied patch from https://www.drupal.org/node/2346773#comment-11897477 and so far it's working as expected. More details on https://www.drupal.org/node/2346773#comment-11897477

The bot should test in in 8.4. I wasn't able to retest it from #44 so setting this to Needs review to see if that helps bump things. No concerns with the RTBC.

So that test only patch SHOULD fail. Patch itself is green. Back to RTBC!

+++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
@@ -43,21 +44,114 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
     foreach (Element::children($elements) as $key) {
-      if (isset($elements[$key]) && $elements[$key]) {
...
+      if (!empty($elements[$key])) {

This is the only weird part of the patch that jumps out at me, when would Element::children ever return something that wouldn't pass this check? But it's part of the original code anyway, so not a big deal.

Everything else looks good to me, I feel comfortable signing off on this as a subsystem maintainer.
Thanks for the incredible work and persistence @dmsmidt, and everyone else who pushed this along!

Yes as I mentioned, I can assure it's working as well, I'll try to add a bit more comment to here too. I tested it last Saturday for #SprintWeekend. I tested it manually.

I also went ahead and tested issue https://www.drupal.org/node/2346773 which is postponed by this current issue. I also can ensure that patch has worked and so far we didn't see anything weird in the code. It seems we are getting closer.

Thanks guys for the hard work.

I tried to re-arrange the code to not change the signature of the protected method and to keep it simple - but recursive methods are never simple :). The approach in the patch looks okay and has the sign-off from a sub system maintainer.

Committed and pushed 01ac763 to 8.4.x and 8d8bb34 to 8.3.x. Thanks!

diff --git a/core/lib/Drupal/Core/Form/FormErrorHandler.php b/core/lib/Drupal/Core/Form/FormErrorHandler.php
index 4efe593..9783cb5 100644
--- a/core/lib/Drupal/Core/Form/FormErrorHandler.php
+++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
@@ -44,50 +44,51 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
   }
 
   /**
-   * Stores errors and a list of children errors directly on the element.
+   * Stores errors and a list of child element errors directly on each element.
    *
    * Grouping elements like containers, details, fieldgroups and fieldsets may
-   * need error info of their children to be able to accessibly show form
-   * problems to a user. A good example is a details element which should be
-   * opened when children have errors.
+   * need error info from their child elements to be able to accessibly show
+   * form error messages to a user. For example, a details element should be
+   * opened when child elements have errors.
    *
-   * Groupings example:
+   * Grouping example:
    * Assume you have a 'street' element somewhere in a form, which is displayed
    * in a details element 'address'. It might be:
    * @code
-   * $form['street'] = array(
+   * $form['street'] = [
    *   '#type' => 'textfield',
    *   '#title' => $this->t('Street'),
    *   '#group' => 'address',
    *   '#required' => TRUE,
-   * );
-   * $form['address'] = array(
+   * ];
+   * $form['address'] = [
    *   '#type' => 'details',
    *   '#title' => $this->t('Address'),
-   * );
+   * ];
    * @endcode
    *
    * When submitting an empty street field, the generated error is available to
    * the different render elements like so:
    * @code
    * // The street textfield element.
-   * $element = array(
+   * $element = [
    *   '#errors' => {Drupal\Core\StringTranslation\TranslatableMarkup},
-   *   '#children_errors' => array(),
-   * );
+   *   '#children_errors' => [],
+   * ];
    * // The address detail element.
-   * $element = array(
+   * $element = [
    *   '#errors' => null,
-   *   '#children_errors' => array(
+   *   '#children_errors' => [
    *      'street' => {Drupal\Core\StringTranslation\TranslatableMarkup}
-   *   ),
-   * );
+   *   ],
+   * ];
    * @endcode
    *
-   * The list of children errors of an element is an associative array. A child
-   * error is keyed with the #array_parents value of the respective element.
-   * The key is formed by imploding this value with '][' as glue.
-   * E.g. a value ['contact_info', 'name'] becomes 'contact_info][name'.
+   * The list of child element errors of an element is an associative array. A
+   * child element error is keyed with the #array_parents value of the
+   * respective element. The key is formed by imploding this value with '][' as
+   * glue. For example, a value ['contact_info', 'name'] becomes
+   * 'contact_info][name'.
    *
    * @param array $form
    *   An associative array containing a reference to the complete structure of
@@ -99,7 +100,7 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
    *   be processed while traversing up the tree. For recursion only; leave
    *   empty when calling this method.
    */
-  protected function setElementErrorsFromFormState(array &$form, FormStateInterface $form_state, array &$elements = array()) {
+  protected function setElementErrorsFromFormState(array &$form, FormStateInterface $form_state, array &$elements = []) {
     // At the start of traversing up the form tree set the to be processed
     // elements to the complete form structure by reference so that we can
     // modify the original form. When processing grouped elements a reference to
@@ -111,14 +112,14 @@ protected function setElementErrorsFromFormState(array &$form, FormStateInterfac
     // Recurse through all element children.
     foreach (Element::children($elements) as $key) {
       if (!empty($elements[$key])) {
-        // Get the child by reference so that we can update the orignial form.
+        // Get the child by reference so that we can update the original form.
         $child = &$elements[$key];
 
         // Call self to traverse up the form tree. The current element's child
         // contains the next elements to be processed.
         $this->setElementErrorsFromFormState($form, $form_state, $child);
 
-        $children_errors = array();
+        $children_errors = [];
 
         // Inherit all recorded "children errors" of the direct child.
         if (!empty($child['#children_errors'])) {

Some comment fixes on commit. Reviewed with @dmsmidt on IRC.