Form errors should only be set during validation

I 100% agree with this API "change". It is long overdue.

Not really an API change IMHO, this patch just makes code reflect reality. Much rather, this is a major bug fix in my book, because:

There have been plenty of security issues in contributed and custom modules in the past, in which developers assumed they'd do it right, because, "heck, there's a error message!" — but in reality, the code was dead-wrong and a security disaster.

Curious, do you see a way to go one step further and throw an \BadMethodCallException in the setError() methods, in case some code tries to set a form validation error outside of form validation?

Explicitly blowing up would make me even more happy.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -878,6 +875,25 @@ public function validateForm($form_id, &$form, &$form_state) {
   +    // Store all of the errors for this form at the top level.
   +    $form['#errors'] = $this->getErrors($form_state);
   

   Hm...

   It's not a good idea to mix state into the $form structure.

   I know we have lots of legacy code that is still doing that, but it really should not — $form_state is the only correct home for stateful data.

   It looks like this change is only related to the parent/originating issue and not actually needed here? — If so, can we skip that here?

2. +++ b/core/modules/file/file.module
   @@ -918,7 +918,7 @@ function file_save_upload($form_field_name, array &$form_state, $validators = ar
   -      form_set_error($form_field_name, $form_state, $message);
   +      drupal_set_message($message);
   
   @@ -928,7 +928,7 @@ function file_save_upload($form_field_name, array &$form_state, $validators = ar
   -      form_set_error($form_field_name, $form_state, t('File upload error. Could not move uploaded file.'));
   +      drupal_set_message(t('File upload error. Could not move uploaded file.'));
   

   (and elsewhere)

   drupal_set_message() $type defaults to 'status', not 'error' :-)

3. +++ b/core/modules/update/update.manager.inc
   @@ -696,7 +696,7 @@ function update_manager_install_form_submit($form, &$form_state) {
        // @todo: Fix me in D8: We need a way to set multiple errors on the same
        // form element and have all of them appear!
   

   Very interesting @todo in the diff context — I wasn't aware of that idea/requirement.

Happy to make the BadMethodCallException a follow-up.

Off-hand without studying, the test failures appear to be caused by a lack of a reset of $this->validatedForms, and because it's keyed by $form_id, those tests are failing because they submit the same form more than once.

Not sure why you went with that condition, but normally $form_state['submitted'] is the unique condition that FormBuilder::processForm() uses itself to determine whether to enter the submission phase, so theoretically a simple negated !$form_state['submitted'] should cut it.

Committed/pushed to 8.x, thanks!

Yay, thanks! :-)

This definitely needs a change notice though.

I'd say create the change record now and reference in both issues, since we need to ahem create it before we commit that issue anyway. ;)

Thanks @tim.plunkett!