Core relies on undocumented feature of FormStateInterface::setErrorByName()

A big plus one for addressing this. Currently it's such a confusing way to mark the form as having an error without targeting a single element. I strongly prefer solution #2, otherwise we make something confusing official.

Thinking aloud:
Hopefully such a new method won't be misused in the way that people will just skip marking elements as erroneous when it becomes a bit difficult.

For example:
There are cases in which a combination of inputs causes an error but you don't want to show error messages for all input fields.
Then someone can just use this new method and show the user a generic error message, however the problematic fields aren't highlighted in this case (bad accessibility).
By the way, if you would do it right and mark elements as erroneous without adding a message, you will see there is some rendering cleanup to do (Found and fixed in this issue: #2455933-45: Error highlighting and reporting problems for the current password on the user profile form).

So if we add a new method I'd like to see a note in the docs that people should not forget marking the problematic fields as having an error.

This is not different from what done in Drupal 7 core, which used code like form_set_error('', t('This book has been modified by another user, the changes could not be saved.'));. I don't see the need of having a new method, when it is enough to document what happens when $name is an empty string.

It looks to me like a simple documentation update would be the right thing to do. I've created a simple patch that adds the documentation to the $name parameter.

I would rather put it at the beginning.

I'm at drupalcorn sprint. I'm going to review this.

my inclination is that in this issue we can do the documentation, and make a different issue for making a new method. [edit: new issue #2780343: Add a method to FormStateInterface to set the error message on the whole form ]

"Making the undocumented feature official by documenting it, marking the $name as optional, and providing a test."

But I would interpret it as "documenting the current behavior".

While talking to @KarlKedrovsky they had a good point, that if it was actually made optional and defaulted to the empty string (behavior), then it would be possible for both arguments to be the empty defaults, and there would be an empty error on the whole form, which is not particularly useful... so making that behavior too easy, might not be what we want to encourage. making it a default is also an API change (well kinda). I think documenting the current behavior will also make an other issue to change it easier to do.

do we know for sure there are no tests already for the empty name?

I'll look and see if I can find (related) test coverage.

----

about the order in the docs, we might want to put them in the order of "most common usage" to least? We might be able to use formatting to make it easier to read.

I'll take a stab at that.

I checked coding standards https://www.drupal.org/node/1354#lists ... didn't do the "usual" colon. hm.

patch and interdiff https://drupal.org/documentation/git/interdiff (even though the interdiff is mostly like the patch)

gonna look for tests next.

looked at ag -B6 "setErrorByName\(\\\$name" core results
(to see if maybe $name might have been set to be '' before the call and didn't (quickly) see any)

ag "setErrorByName\(\'\'" core
finds setErrorByName(''

ag "setErrorByName\(\'\'" core | wc -l
counts 13

---

ag "setErrorByName\(\'\'" core | grep -i test

yields

core/modules/system/tests/modules/form_test/src/Callbacks.php:44: $form_state->setErrorByName('');
in method validateName()

      // Trigger a form validation error to see our changes.
      $form_state->setErrorByName('');

Not testing what happens with '', just calling it to.... "trigger error". hm.

core/modules/system/tests/modules/form_test/src/Form/FormTestValidateForm.php:63: $form_state->setErrorByName('');
same. hm.

---

ag "@covers.*setErrorByName" core

core/tests/Drupal/Tests/Core/Form/FormStateTest.php
102:   * @covers ::setErrorByName

three calls, name is not empty

core/tests/Drupal/Tests/Core/Form/FormStateTest.php
134:   * @covers ::setErrorByName

one call, name not empty

core/tests/Drupal/Tests/Core/Form/FormStateDecoratorBaseTest.php
1310:   * @covers ::setErrorByName

name is 'foo'

core/tests/Drupal/Tests/Core/Form/SubformStateTest.php
297:   * @covers ::setErrorByName

has two calls, both not empty names.

---

ok. so.

suppose that confirms no test for the behavior of setting the error on the form when the $name is ''.
next: figure out where to place such a test.

guessing...
and just a little tiny start at a test (not sure how to assert the "correct" behavior of the error being *on the whole form* and not on an element)

run with
../vendor/bin/phpunit tests/Drupal/Tests/Core/Form/FormStateTest.php --filter=testSetErrorByName

---
also updated the issue summary to not lose track of the little detail about "what does/should happen when the name is *invalid*"

make test pass, kinda. looks for the error that is "on the form" and not an element, but not sure how to assert the error is on the form and not on an element.
not sure how to test for an invalid name for a form element... taking that out for now.

I'm probably done with this for a while, so open to reviews.

Copying my comment from #2780343: Add a method to FormStateInterface to set the error message on the whole form

Given this code:

$form_state->setError($form, 'This error is for the whole form');

Internally that calls:

$form_state->setErrorByName('', 'This error is for the whole form');

The first approach is much clearer, IMO.
But both work, and because one calls the other, they both have to work.

Re-rolled for 9.2

This patch is the same as #28 but with a tests-only patch. If the tests fails I'll mark RTBC

Would help if I uploaded the files

Moving back to needs work as the tests patch should have failed.

As it says in the issue summary, I raised this issue seven years ago as part of #2509268-6: Inline errors repeated on child elements in module uninstall form. My comments there were not completely transcribed, and it appears that an important point I made there has been lost:

To be clear, this is not just a documentation issue; if '' is supposed to produce a known result, then setErrorByName() should explicitly check $name == '' rather than fall-through to some non-specified behavior.

It's not sufficient to just put a comment in setErrorByName() and call it a day. The fundamental assumption in FormState is that the internal array $errors will only hold errors keyed by the form element those errors belong to. (That's also not documented, but that's how the code is written). Specifically, there is no indication in any of the FormState source code that the keys of the $errors array can be anything other than form element names, and there is no indication or awareness in the code that the empty string is a "special" key used for a special purpose. The get/set methods for this internal array are supposed to be the API we use to manage that internal array, but they don't deal with this special key at all and (relevant to this issue) do not document, even after the patch, that the empty string is a special value in the $errors array.

The fact that passing $name = '' to the setErrorByName() method currently works is an accident, it is NOT by design. Documentation doesn't fix the design. And the current patch doesn't even document all the methods that need to know that this $name key can be an empty string.

This is one of the reasons I suggested new methods to get/set an error on the entire form - that suggestion was spawned off into yet another issue #2780343: Add a method to FormStateInterface to set the error message on the whole form which has not made any progress either. There are also other ways this can be handled, but just documenting one of the many methods affected is not one of those ways.

If you look at the source code for setErrorByName(), if $name is an empty string there is no explicit behavior - the method only works because all the other code in FormState is ignorant of what to do with an empty string key, not because there is an intention to use an empty string key for a specific purpose.

The assumption is that $form_state->setError($form, "Form error") is always equivalent to $form_state->setErrorByName('', "Form error"), while that is true IIF $form['#parents'] is an empty array, because in that case implode('][', $element['#parents']) (used by FormState::setError()) will return an empty string.
That is a fragile assumption: $form['#parents'] isn't required to be an empty array. For a sub-form, it would make sense for $form['#parents'] to contain at least an item.