Updated: Comment #1

Problem/Motivation

The error methods for forms are bad. form_get_errors() uses global state and just returns the errors from the last submitted forms.
This needs cleaning up.
But first, we need better test coverage.

Proposed resolution

Add full test coverage and remove any weirdness that surfaces.

Remaining tasks

Explore possible solutions to removing the global-ness of form errors

User interface changes

N/A

API changes

file_save_upload() needs to be passed a $form_state. In all cases, it was already called from a form's context.
form_*_errors() need to be passed a $form_state. If it is not available, they can still see if there *were* errors by calling \Drupal::formBuilder()->getAnyErrors(), but that returns a Boolean only, not the errors themselves.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: +FormInterface, +PHPUnit
Related issues: +#2120841: Convert form_options_flatten() to a method on FormBuilder
FileSize
16.11 KB

Here is the first pass. This also adds a getMockForm() method to FormBuilderTest, there were inconsistencies in how the form was mocked.

This gives us 100% coverage of the form methods, and 78.64% overall.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -1238,23 +1253,20 @@ public function clearErrors() {
    -    $form = $this->setErrorByName();
    -    if (!empty($form)) {
    -      return $form;
    -    }
    +    return $this->errors;
    
    +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.php
    @@ -186,7 +186,7 @@ public function buildForm(array $form, array &$form_state, VocabularyInterface $
    -    $errors = form_get_errors() != FALSE ? form_get_errors() : array();
    +    $errors = form_get_errors();
    

    Much nicer, IMO

  2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -1238,23 +1253,20 @@ public function clearErrors() {
    -    $form = $this->setErrorByName();
    ...
    +      $errors = $this->getErrors();
    

    Grr, need to move this back up.

tim.plunkett’s picture

Related issues: +#569094: Get rid of form_set_error()
FileSize
178.22 KB
189.38 KB

chx pointed me to #569094: Get rid of form_set_error(), which this might supersede.

This has a lot more changes, which I'll explain, but I wanted to post this before I left work.

Status: Needs review » Needs work

The last submitted patch, 3: form-2131851-3.patch, failed testing.

tim.plunkett’s picture

FileSize
176.3 KB

I'm going to hold off on the static stuff, that's easy enough and could be done later.

Fixed a bad search-and-replace.

So, the idea is to work towards making FormBuilder not stateful (storing stuff in instance properties like $this->errors).
To do so, we move it to $form_state. This means that anyone checking for or setting an error needs the $form_state.

Thankfully, in all but 3 places, it is available.
Two are comment_preview() and node_preview(), but those functions are called from a form, and it is trivial to pass in.

The third is _form_set_attributes(), which is called from theme functions and pre_render callbacks, but that has the element, so we can briefly store the errors on there.

That plus the unit tests I wrote, and this is pretty much done!
Unless it fails testing. I have a sneaking suspicion that search.module was doing some weird stuff...

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: form-2131851-5.patch, failed testing.

larowlan’s picture

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.09 KB
180.5 KB

Here are some more hacks for the calls that are outside the context of $form_state. We'll need to clean them up at some point, but this is a good start.

Status: Needs review » Needs work

The last submitted patch, 9: form-error-2131851-9.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
7.6 KB
181.7 KB

I think I have the next set of fixes figured out, it has to do with errors on nested elements. More later.

Status: Needs review » Needs work

The last submitted patch, 11: form-error-2131851-11.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
180.88 KB
1.98 KB

Definitely a problem with errors on nested elements. Gotta figure this out...

Status: Needs review » Needs work

The last submitted patch, 13: form-error-2131851-13.patch, failed testing.

tim.plunkett’s picture

Title: Refactor FormBuilder error methods and add test coverage » Form errors must be specific to a form and not a global
Priority: Normal » Critical
Issue summary: View changes
Status: Needs work » Needs review
FileSize
17.87 KB
189.08 KB

Found the bug with nested forms, it was a flaw in the original proposal I stole from #569094: Get rid of form_set_error().

Also removed all of my $form_state hacks.

If this passes, I think I'm happy with it.

sun’s picture

I've to catch up with some recent changes, but overall, this looks good to me.

I like that the function signatures of form_error() and form_set_error() become more or less the same with this.

In fact, after this change has landed, I think we should explore whether we cannot merge the two functions into a single, whereas the first argument can either be a string (denoting the input name) or an array (denoting the #parents). In turn, there'd only be one function for setting errors. (The two functions always confused me.)

There's only one detail that could use some love from my perspective:

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
+  protected function doCheckErrors(array &$elements, array &$form_state) {

Can we find a better name for this method?

It's not a (pure) getter, not a (pure) setter, and it also doesn't really "check" (as in "validate") the form for errors. It would be helpful if the method name would make clear what the function is doing.

tim.plunkett’s picture

doCheckErrors() definitely needs a better name. That was the last bug fix, and I was so frustrated/relieved after hours of debugging that I just put it in quickly to ensure it was the right fix.

  1. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
    @@ -513,14 +513,14 @@ public function executeHandlers($type, &$form, &$form_state);
    -   * This causes the Form API to not execute the form submit handlers, and
    +   * This causes the Form API to not execute the form submit handlers, $form_state, and
    ...
    -   * limit validation errors to only certain elements. For example, pressing the
    +   * limit validation errors to only certain elements. For example, $form_state, pressing the
    

    These are bad changes from the search/replace.

  2. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
    @@ -566,11 +566,11 @@ public function executeHandlers($type, &$form, &$form_state);
    +   * calls to form_set_error('step1', $form_state, $message) or
    +   * form_set_error('step1][choice', $form_state, $message) will prevent the submit handlers
        * from running, and result in the error message being displayed to the user.
    -   * However, calls to form_set_error('step2', $message) and
    -   * form_set_error('step2][groupX][choiceY', $message) will be suppressed,
    +   * However, calls to form_set_error('step2', $form_state, $message) and
    +   * form_set_error('step2][groupX][choiceY', $form_state, $message) will be suppressed,
    

    This needs to be rewrapped.

I'll get to this later today, leaving for more reviews in the meantime.

tim.plunkett’s picture

FileSize
4.3 KB
188.32 KB

Renamed to setElementErrorsFromFormState, and fixed the docs issues.

dawehner’s picture

  1. +++ b/core/modules/ban/lib/Drupal/ban/Form/BanAdmin.php
    @@ -105,13 +105,13 @@ public function buildForm(array $form, array &$form_state, $default_ip = '') {
    -      form_set_error('ip', $this->t('This IP address is already banned.'));
    +      form_set_error('ip', $form_state, $this->t('This IP address is already banned.'));
    

    We should open a follow up to always inject the form builder into the form base.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/FormTest.php
    @@ -103,7 +103,6 @@ function testRequiredFields() {
    -          form_clear_error();
    

    What could this be dropped?

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/FormTest.php
    @@ -237,9 +236,6 @@ function testRequiredCheckboxesRadio() {
    -    $form = $form_state = array();
    -    form_test_validate_required_form_no_title($form, $form_state);
    

    Any reason we drop this here?

tim.plunkett’s picture

1) I'm thinking we should move the error stuff to its own class, maybe a trait!? and then FormBase can just use it.
Or even if not a trait, it should be a separate interface/class. FormBase doesn't need any of the other methods.

2) Because the previous line has $form_state = form_state_defaults();, which is where the errors are stored anyway. This is all because this test calls internal functionality. It should be converted to a unit test eventually.

3) Those variables are never used. I have *no idea* why they ever existed.

dawehner’s picture

Or even if not a trait, it should be a separate interface/class. FormBase doesn't need any of the other methods.

I think it is no problem to call the formBuilder all the time if you need some of those methods.

3) Those variables are never used. I have *no idea* why they ever existed.

In that case, remove the @see for this method and the actual method.

tim.plunkett’s picture

I'll open the followup.

I meant the variable was never used, not the method. It is called via $this->drupalPostForm('form-test/validate-required-no-title', $edit, 'Submit');, 3 lines later in the function.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Got it, thank you.

tim.plunkett’s picture

FileSize
197.04 KB
14.41 KB

We should open a follow up to always inject the form builder into the form base.

I thought about this more, and I think the first step is to separate these methods onto a separate interface. Then when we inject it, we can typehint to that specific interface, and all of the other form building methods will not be available.

It's the difference between $this->formBuilder->OMG_I_CAN_DO_ANYTHING() and $this->formErrors->setError()

Ran this by @dawehner and @sun in IRC, @sun and I discussed it and there was no real pushback.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: form-2131851-24.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

24: form-2131851-24.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Random fail.

tim.plunkett’s picture

Assigned: tim.plunkett » alexpott

@webchick suggested that @alexpott review this, but I'm not picky :)

tim.plunkett’s picture

FileSize
55.28 KB
196.5 KB

Here is a version of the patch without all of the additions of $form_state, to better show the actual scope of the issue.

I've also rerolled (with no changes) after the recent core commits.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: form-2131851-29.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

29: form-2131851-29.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

HEAD failed. Retesting this passed.

tim.plunkett’s picture

FileSize
198.34 KB

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 33: form-2131851-33.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review

33: form-2131851-33.patch queued for re-testing.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Back again after HEAD issues

webchick’s picture

Title: Form errors must be specific to a form and not a global » Change notice: Form errors must be specific to a form and not a global
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Approved API change, +Needs change record

alexpott already gave tacit approval of this (tagging as such), and since he's trying to stay focused on CMI I'm OK committing this, esp now that we have some runway before the next alpha. But it's definitely a *huge* API change that will affect basically every module developer who's ported their code to 8.x so far. :( :( Please turn around the change notice as quickly as you can.

One question, doesn't need to hold up commit:

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -1061,7 +1055,7 @@ protected function doValidateForm(&$elements, &$form_state, $form_id = NULL) {
-        $this->setErrorByName(NULL, '', $form_state['triggering_element']['#limit_validation_errors']);
+        $form_state['limit_validation_errors'] = $form_state['triggering_element']['#limit_validation_errors'];

Why the move from OO code to ArrayAPIs here? Shouldn't we be using a method wrapper for setting this like we do for getting?

Committed and pushed to 8.x. Thanks!

tim.plunkett’s picture

Status: Active » Needs review

Thanks!

Added https://drupal.org/node/2142817

To answer your question, we were previously storing that information in $this->errorSections, which made the FormBuilder class stateful (meaning it tracked something about the form, not just processed it). We moved that information from the FormBuilder class into the $form_state, so that it is tracked by the form, and FormBuilder is one step closer to being stateless.

larowlan’s picture

Title: Change notice: Form errors must be specific to a form and not a global » Form errors must be specific to a form and not a global
Priority: Major » Critical
Status: Needs review » Fixed
Issue tags: -Needs change record

Change notice is good, Tim also expanded the earlier FormBuilder change notice to show that form_set_error etc is deprecated (it was missing from there)

Status: Fixed » Closed (fixed)

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