Problem/Motivation

#2239299: Form errors should only be set during validation fixed all known instances of this problem.
But we have no way to enforce it yet.

Proposed resolution

Throw an exception if form errors are set from outside validation (i.e., during submit).

Remaining tasks

Write patch
Write tests

User interface changes

N/A

API changes

If #2239299: Form errors should only be set during validation gets a change record, then this won't need one.
I'd consider *this* the API change, since it's not enforced yet.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

We have a test, FormValidationTest::testNoDuplicateErrorsForIdenticalForm, which tests that form errors for two forms with the same ID are not displayed visually.
But it doesn't test that the validation of one form doesn't affect the duplicate, because it is still broken.

After discussing with @sun, he found #766146: Support multiple forms with same $form_id on the same page, which exactly describes this problem.
Therefore, the test is only giving us a false sense of security, since it is testing a visual hack, and not a true bug fix.

bleen’s picture

+++ b/core/tests/Drupal/Tests/Core/Form/FormValidationTest.php
@@ -26,37 +26,6 @@ public static function getInfo() {
-  public function testNoDuplicateErrorsForIdenticalForm() {

I'm probably just missing something, but why does throwing an exception on bad form error calls mean that we can kill this test?

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -806,7 +801,7 @@ public function prepareForm($form_id, &$form, &$form_state) {
+    if (!empty($form_state['validation_complete']) && empty($form_state['must_validate'])) {

That test fails because of this change.
But the test was implying that #766146: Support multiple forms with same $form_id on the same page was fixed, and it wasn't. So I'm removing the flawed test so that we can go about fixing it correctly.

sun’s picture

This looks very nice.

  1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -806,7 +801,7 @@ public function prepareForm($form_id, &$form, &$form_state) {
       public function validateForm($form_id, &$form, &$form_state) {
    -    if (isset($this->validatedForms[$form_id]) && empty($form_state['must_validate'])) {
    +    if (!empty($form_state['validation_complete']) && empty($form_state['must_validate'])) {
           return;
         }
    

    Hm. It looks like must_validate = TRUE can re-enter form validation, even if validation_complete is TRUE.

    → In that edge-case, if a form validation handler tries to set an error, then the form state still believes that validation is complete already + throws an exception.

    I think we need to force-reset validation_complete to FALSE when that early-return condition is not entered?

    As I wasn't sure what the use-case for must_validate is, I quickly grepped core... and the only usage is in ViewsExposedForm — not sure why that forces validation, but let's double-check + verify that resetting validation_complete to FALSE upon re-entering validation does not break anything.

  2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -1198,6 +1193,10 @@ public function executeHandlers($type, &$form, &$form_state) {
    +      throw new \BadMethodCallException('Validation errors cannot be set during submission handling.');
    

    I wonder whether the reference to "submission" correctly describes the constraint... how about this:

    "Form errors cannot be set after form validation."

tim.plunkett’s picture

FileSize
7.27 KB
3.89 KB

Good points both.
I separated the handling of must_validate from the validation_complete check to make it more explicit, and added a unit test that does expose the bug you described.

Also updated the exception message.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -1198,6 +1200,10 @@ public function executeHandlers($type, &$form, &$form_state) {
    +      throw new \BadMethodCallException('Form errors cannot be set after form validation has finished.');
    

    This is not the right exception: " * Exception thrown if a callback refers to an undefined method or if some
    * arguments are missing." Maybe invalidArgument of Logic matches better here.

  2. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
    @@ -17,6 +17,8 @@
    + * @coversDefaultClass \Drupal\Core\Form\FormBuilder
    
    @@ -219,6 +221,32 @@ public function testHandleRedirectWithResponse() {
       /**
    +   * Tests that form errors during submission throw an exception.
    +   *
    +   * @expectedException \BadMethodCallException
    +   * @expectedExceptionMessage Form errors cannot be set after form validation has finished.
    +   */
    +  public function testFormErrorsDuringSubmission() {
    

    Don't we test some methdo here as well?

+++ b/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;
-

I am a bit confused about the removal of the test here.

We have a test, FormValidationTest::testNoDuplicateErrorsForIdenticalForm, which tests that form errors for two forms with the same ID are not displayed visually.

So we do now loose test coverage that same IDs are not displayed visually now?

sun’s picture

  1. +1 to LogicException
  2. Well-spotted!
  3. testNoDuplicateErrorsForIdenticalForm() is a useless test, because it asserts the proper behavior of the bug described in #766146: Support multiple forms with same $form_id on the same page
tim.plunkett’s picture

FileSize
1.39 KB
7.29 KB

1 and 2 fixed, 3 ignored again :)

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: form-errors-2241735-8.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

8: form-errors-2241735-8.patch queued for re-testing.

sun’s picture

Status: Needs review » Reviewed & tested by the community
sun’s picture

8: form-errors-2241735-8.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

This looks like good clean-up. The one area i was concerned with is what would happen now when two forms on the same page had validation errors at the same time, but in manual testing I didn't see any difference before/after the patch.

Committed and pushed to 8.x. Thanks!

  • Commit 709c4ab on 8.x by webchick:
    Issue #2241735 by tim.plunkett: Throw an exception when form errors are...
tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Rob230’s picture

I have a multi-part form that gets rebuilt with an internal step variable keeping track of which step of the form you are on.

I get this error - Form errors cannot be set after form validation has finished - after a submit. When it rebuilds the form it seems to revalidate all the data? Is that expected? I would expect the validate and the submit callbacks to be fired exactly once.

Edit: never mind, I realised my problem lay elsewhere.