Issue #2241735 by tim.plunkett: Throw an exception when form errors are set outside of validation.

core/lib/Drupal/Core/Form/FormBuilder.php

@@ -105,13 +105,6 @@ class FormBuilder implements FormBuilderInterface {
    */
   protected $forms;
 
-  /**
-   * An array of validated forms.
-   *
-   * @var array
-   */
-  protected $validatedForms = array();
-
   /**
    * An array of options used for recursive flattening.
    *
@@ -301,6 +294,7 @@ public function getFormStateDefaults() {
         'files' => array(),
       ),
       'temporary' => array(),
+      'validation_complete' => FALSE,
       'submitted' => FALSE,
       'executed' => FALSE,
       'programmed' => FALSE,
@@ -431,6 +425,7 @@ protected function getUncacheableKeys() {
       'input',
       'method',
       'submit_handlers',
+      'validation_complete',
       'submitted',
       'executed',
       'validate_handlers',
@@ -806,7 +801,14 @@ public function prepareForm($form_id, &$form, &$form_state) {
    * {@inheritdoc}
    */
   public function validateForm($form_id, &$form, &$form_state) {
-    if (isset($this->validatedForms[$form_id]) && empty($form_state['must_validate'])) {
+    // If this form is flagged to always validate, ensure that previous runs of
+    // validation are ignored.
+    if (!empty($form_state['must_validate'])) {
+      $form_state['validation_complete'] = FALSE;
+    }
+
+    // If this form has completed validation, do not validate again.
+    if (!empty($form_state['validation_complete'])) {
       return;
     }
 
@@ -888,7 +890,7 @@ protected function finalizeValidation($form_id, &$form, &$form_state) {
     // After validation, loop through and assign each element its errors.
     $this->setElementErrorsFromFormState($form, $form_state);
     // Mark this form as validated.
-    $this->validatedForms[$form_id] = TRUE;
+    $form_state['validation_complete'] = TRUE;
   }
 
   /**
@@ -1198,6 +1200,10 @@ public function executeHandlers($type, &$form, &$form_state) {
    * {@inheritdoc}
    */
   public function setErrorByName($name, array &$form_state, $message = '') {
+    if (!empty($form_state['validation_complete'])) {
+      throw new \LogicException('Form errors cannot be set after form validation has finished.');
+    }
+
     if (!isset($form_state['errors'][$name])) {
       $record = TRUE;
       if (isset($form_state['limit_validation_errors'])) {

core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php

@@ -17,6 +17,8 @@
 /**
  * Tests the form builder.
  *
+ * @coversDefaultClass \Drupal\Core\Form\FormBuilder
+ *
  * @group Drupal
  * @group Form
  */
@@ -218,6 +220,34 @@ public function testHandleRedirectWithResponse() {
     $this->assertSame($response, $form_state['response']);
   }
 
+  /**
+   * Tests that form errors during submission throw an exception.
+   *
+   * @covers ::setErrorByName
+   *
+   * @expectedException \LogicException
+   * @expectedExceptionMessage Form errors cannot be set after form validation has finished.
+   */
+  public function testFormErrorsDuringSubmission() {
+    $form_id = 'test_form_id';
+    $expected_form = $form_id();
+
+    $form_arg = $this->getMockForm($form_id, $expected_form);
+    $form_builder = $this->formBuilder;
+    $form_arg->expects($this->any())
+      ->method('submitForm')
+      ->will($this->returnCallback(function ($form, &$form_state) use ($form_builder) {
+        $form_builder->setErrorByName('test', $form_state, 'Hello');
+      }));
+
+    $form_state = array();
+    $this->formBuilder->getFormId($form_arg, $form_state);
+
+    $form_state['values'] = array();
+    $form_state['input']['form_id'] = $form_id;
+    $this->simulateFormSubmission($form_id, $form_arg, $form_state, FALSE);
+  }
+
   /**
    * Tests the redirectForm() method when a redirect is expected.
    *
@@ -527,6 +557,47 @@ public function testSubmitForm() {
     $this->assertNotEmpty($errors['options']);
   }
 
+  /**
+   * Tests the 'must_validate' $form_state flag.
+   *
+   * @covers ::validateForm
+   */
+  public function testMustValidate() {
+    $form_id = 'test_form_id';
+    $expected_form = $form_id();
+
+    $form_arg = $this->getMock('Drupal\Core\Form\FormInterface');
+    $form_arg->expects($this->any())
+      ->method('getFormId')
+      ->will($this->returnValue($form_id));
+    $form_arg->expects($this->any())
+      ->method('buildForm')
+      ->will($this->returnValue($expected_form));
+    $form_builder = $this->formBuilder;
+    $form_arg->expects($this->exactly(2))
+      ->method('validateForm')
+      ->will($this->returnCallback(function (&$form, &$form_state) use ($form_builder) {
+        $form_builder->setErrorByName('test', $form_state, 'foo');
+      }));
+
+    $form_state = array();
+    // This submission will trigger validation.
+    $this->simulateFormSubmission($form_id, $form_arg, $form_state);
+    $errors = $this->formBuilder->getErrors($form_state);
+    $this->assertNotEmpty($errors['test']);
+
+    // This submission will not re-trigger validation.
+    $this->simulateFormSubmission($form_id, $form_arg, $form_state);
+    $errors = $this->formBuilder->getErrors($form_state);
+    $this->assertNotEmpty($errors['test']);
+
+    // The must_validate flag will re-trigger validation.
+    $form_state['must_validate'] = TRUE;
+    $this->simulateFormSubmission($form_id, $form_arg, $form_state);
+    $errors = $this->formBuilder->getErrors($form_state);
+    $this->assertNotEmpty($errors['test']);
+  }
+
   /**
    * Tests the flattenOptions() method.
    *

core/tests/Drupal/Tests/Core/Form/FormValidationTest.php

@@ -26,37 +26,6 @@ public static function getInfo() {
     );
   }
 
-  public function testNoDuplicateErrorsForIdenticalForm() {
-    $form_id = 'test_form_id';
-    $expected_form = $form_id();
-    $expected_form['test']['#required'] = TRUE;
-
-    // Mock a form object that will be built three times.
-    $form_arg = $this->getMockForm($form_id, $expected_form, 3);
-
-    // The first form will have errors.
-    $form_state = array();
-    $this->formBuilder->getFormId($form_arg, $form_state);
-    $this->simulateFormSubmission($form_id, $form_arg, $form_state);
-    $errors = $this->formBuilder->getErrors($form_state);
-    $this->assertNotEmpty($errors['test']);
-
-    // The second form will not have errors.
-    $form_state = array();
-    $this->simulateFormSubmission($form_id, $form_arg, $form_state);
-    $errors = $this->formBuilder->getErrors($form_state);
-    $this->assertEmpty($errors);
-
-    // Reset the form builder.
-    $this->setupFormBuilder();
-
-    // On a new request, the first form will have errors again.
-    $form_state = array();
-    $this->simulateFormSubmission($form_id, $form_arg, $form_state);
-    $errors = $this->formBuilder->getErrors($form_state);
-    $this->assertNotEmpty($errors['test']);
-  }
-
   public function testUniqueHtmlId() {
     $form_id = 'test_form_id';
     $expected_form = $form_id();