Display 'required' validation errors for form elements that do not provide a #title

This does not meet the criteria for a critical bug.

I think form submission of data failing silently is critical. I can't believe this bug existed all this time.
Displaying the form ID which always exists and is descriptive despite it contains the hyphens I think is acceptable.

RTBC - looks great to me!

This actually affects Drupal 8 too - the impact is lessened there due to HTML5 validation, but I assume a decent number of people will want to turn off HTML5 validation on real sites anyway (since with current browsers it's pretty annoying).

I don't think this is critical really - there is still some visual indication that the form didn't submit (just really bad visual indication) and overall nothing breaks, it's just bad user experience. Plus as recommended in the code comment, you really shouldn't have a form element with no #title anyway...

The patch looks like an improvement, but it's adding a new translatable string for no reason (t('!name...') would work just as well as t('!id...')). And displaying the ID will have almost no meaning to end users here and will probably confuse them. If we're willing to add a new translatable string to fix this (which I think we are) then why not something like "The highlighted field is required", "A required field was not filled out", or similar?

True, I guess for things like radios or checkboxes there might not be any visual indication currently.

Here's a patch for Drupal 8 to move this along. I would still lean towards something like "A required field was not filled out" myself, but this continues to use the ID in the message for now.

I will take on the task of adding test coverage for this.

#id is also not mandatory and we need to check that it is set before we try to use it. This patch addresses this but if the aim of this issue is to ensure that an error message is sent it might be better to use a generic error message such as 'Field is required.'

@stefan.r I did some debugging around the failing test in #11 to determine what the #id was for the field and there was no #id set. I will investigate some more around the code that you mentioned in #17.

formBuilder does not seem to be triggered in Drupal\Tests\Core\Form\FormValidatorTest::testRequiredErrorMessage()

The form is manually prepared and passed directly to the FormValidator.

Given that a form element can be passed to a validator without the #id being set then I still think we should check that it exists before attempting to display it.

I've added a test case where the #id is set.

Thanks guys for your effort.
Can I suggest we have two short comments similar to:
// Identify the required form element by its title.
and
// Otherwise, identify the required form element by its ID.

Even more accurate:
Include the element title in the error message.
and
Otherwise, display the element ID in the error message.

This is a pretty nasty bug, so I'd like to see this get fixed. My only change here was to add the comments suggested in #24/#25.

I think this is RTBC-able, but since I made *any* kind of patch, I probably should not be the one to set that status. ;-)

RTBC - looks great to me.

Just tweaking the comments. Looks like a reasonable change to FormValidator.

Looking at the "after" screenshot up there, that definitely isn't going to fly. There is no way for a content author to understand what field that is when we're exposing HTML ID details that would only be visible to them in view source.

The core committers talked about this on our triage call, and while the bug would be great to fix, this isn't a regression from D7 (it's rather an existing bug in D7), so not something we need to prioritize during RC (we could just as easily fix this in a patch/minor release). Additionally, we need to solve it a different way (e.g. making #title required for #required fields, or having a known label property to get some kind of human-readable string).

Un-marking RC target and needs working.

1. +++ b/core/lib/Drupal/Core/Form/FormValidator.php
   @@ -296,8 +297,16 @@ protected function doValidateForm(&$elements, FormStateInterface &$form_state, $
   +        // If no title is set, display the element ID in the error message.
   +        elseif (isset($elements['#id'])) {
   +          $form_state->setError($elements, $this->t('A required field was not filled out.'));
   ...
            else {
              $form_state->setError($elements);
            }
   

   We're not actually displaying the element ID any more, so why not just improve the else case?

2. +++ b/core/lib/Drupal/Core/Form/FormValidator.php
   @@ -296,8 +297,16 @@ protected function doValidateForm(&$elements, FormStateInterface &$form_state, $
   +          // Log the field ID for debugging purposes. We implement this as a
   +          // trigger_error() as the ID may not be understandable to end users.
   +          trigger_error((string) new FormattableMarkup('@name field is required.', array('@name' => $elements['#id'])));
   

   Why do we need this here? Also if we add it, it should be tested.

I revised the patch #35.

1. Fixed the point 1 in comment #37.

        // If no title is set, display a vague error message.
        else {
          $form_state->setError($elements, $this->t('A required field was not filled out.'));
          // Log the field ID for debugging purposes. We implement this as a
          // trigger_error() as the ID may not be understandable to end users.
          if (isset($elements['#id'])) {
            trigger_error((string) new FormattableMarkup('@name field is required.', array('@name' => $elements['#id'])));
          }
        }

2. Added a test to test the trigger_error().

            trigger_error((string) new FormattableMarkup('@name field is required.', array('@name' => $elements['#id'])));

The test case became long to test the trigger_error()...

    // If no title is set and id is set, a PHP error is raised.
    if (!isset($element['#title']) && !isset($element['#required_error']) &&
      isset($element['#id'])) {
      // Change the PHP error handler temporarily for test.
      $error_checker = $this->getMockBuilder('stdClass')
        ->setMethods(array('tempErrorHandler'))
        ->getMock();
      $error_checker->expects($this->once())
        ->method('tempErrorHandler');
      set_error_handler(array($error_checker, 'tempErrorHandler'));

      $form_validator->validateForm('test_form_id', $form, $form_state);

      // Reset the PHP error handler.
      restore_error_handler();
    }
    else {
      $form_validator->validateForm('test_form_id', $form, $form_state);
    }

This test for trigger_error() may be unnecessary...

Anyway, I'd like this to be reviewed. Thank you.

I updated the screenshot in the summary as stefan.r told.

I reproduced this in 8.4.x-dev. Patches address the issue and still apply to 8.4.x-dev.

Thanks @djdevin for evaluating the issue and confirming it's still valid! @catch, @cilefen, @alexpott, @Cottser, @lauriii, and I discussed and agreed this issue is indeed a major bug due to the broad usability impact of the silent failure.

Re-roll for 8.6.x.

The above patch contains some coding standard errors.

This fixes the test failure.

Can we instead trigger the error when the form is built? It'll likely expose more problems with core we'll need to fix in this issue, but the errors will show up less randomly, and earlier on (fail fast). Nah, when forms are built we can't easily figure out which elements will need a title in the end.

I added some more details to the error message, explaining this new log entry site administrators will be seeing. It links to the change record I just wrote, which includes several code examples.

Working on this as part of DrupalCon Nashville!

I've been able to confirm that this patch works.

1. +++ b/core/lib/Drupal/Core/Form/FormValidator.php
   @@ -297,10 +298,19 @@ protected function doValidateForm(&$elements, FormStateInterface &$form_state, $
   +          $form_state->setError($elements, $this->t('A required field was not filled out.'));
   

   As users might see this message I think we need a usability review here. To me the current text is a little odd. It implies the field needs to be a be "filled out" but potentially this could be a tickbox or select box. I think also the present tense is better and matches the error when there is a title. I'd suggest A required field is empty.

2. +++ b/core/lib/Drupal/Core/Form/FormValidator.php
   @@ -297,10 +298,19 @@ protected function doValidateForm(&$elements, FormStateInterface &$form_state, $
   +            trigger_error((string) new FormattableMarkup('@name field is required, but because it has no #title, users cannot be shown exactly which element this is. See the documentation at https://www.drupal.org/node/2956364 for which changes must be made to the code.', [
   +              '@name' => $elements['#id'],
   +            ]));
   

   Why not just log an error using the log system?

Usability review

We discussed this at the Usability Meeting today (2018-12-18): https://www.youtube.com/watch?v=9GIdotOoVy8.

Almost any text that we use will be an improvement over the current state, so thanks for working on this.

Notice that the text will be repeated below each affected field if the inline_form_errors module is enabled: see the screenshot below. That means we should try to choose text that will work both at the top of the form and also repeated beneath the empty forms.

We like the suggestion in #59 better than the current patch, but we are not sure that "empty" is a good user-facing word to use. Both suggest that there is only one empty form field, when there might be more than one. Variants that allow for multiple empty fields tend to be awkward and longer than we like.

It would also be a good idea to tell the user how to fix the problem instead of describing the problem.

How about "Please enter something for each required field."? This works fairly well both at the top of the form and beneath each empty field. It tells the user how to fix the problem. It works fairly well whether there is a single empty field or several.

Here is a screenshot of the Umami theme with a custom form and my suggested text, with the inline_form_errors module enabled. (I used the browser tools to insert the recommended text.) My test form does not have titles for the input elements, but it does have descriptions: that is where the text "This input element is required and it does have a #title key." and "Same problem here!" comes from.

Other points

I am adding the "Needs issue summary update" tag because the summary does not explain how to reproduce the problem. I did the following:

• Install the Disable HTML5 validation module.
• Create a custom form with a required field that does not have a title.
• Test with and without the inline_form_errors module.

I spotted this being discussed in the #UX Slack channel. I haven't read the whole issue yet, just tagging for accessibility review.

It's a WCAG failure at level A, for sure See Error Identification: Understanding SC 3.3.1.

To satisfy that WCAG success criterion, we must inform the user which input failed validation, ideally using text. In the case of FAPI elements without a #title, that's awkward because we can't use the name of the element in the message the user sees. Nonetheless, I think this can still pass WCAG "Error Identification", but it would be good to assess it carefully for all the techniques documented in WCAG.

Test with and without the inline_form_errors module

Yes, that's very important. Some of the documented WCAG techniques may only apply when IFE is enabled.

It would also be a good idea to tell the user how to fix the problem instead of describing the problem.

Yes that's covered by some other WCAG success criteria. Basically all the SCs in the guideline group "Input Assistance" applies here:

• Labels or Instructions (level A). In the case of compund fields or filters, this might be satisfied by fieldset legends, but it needs a detailed look at the situations.
• Error Suggestions (level AA). Definitely relevant. It's important to know how to reach a successful submission.
• Error Prevention (Legal, Financial, Data) (level AA) - relates to accidental data loss/change. Probably not relevant to listing filters, but worth considering other possible situations on config/content forms.

Aside, how many FAPI elements lack a #title? The wider problem is avoiding that situation entirely. I think there may be a related issue sonewhere.

Aside, how many FAPI elements lack a #title? The wider problem is avoiding that situation entirely. I think there may be a related issue sonewhere.

It is certainly good practice to supply a #title element, and this issue is all about doing the best we can when people violate that standard. For example, here is what I used to test:

  public function buildForm(array $form, FormStateInterface $form_state) {
    $form['enter_text_or_else'] = [
      '#type' => 'textfield',
      // '#title' => $this->t('Enter text or else!'),
      '#description' => $this->t('This input element is required and it has no #title key.'),
      '#maxlength' => 64,
      '#required' => TRUE,
      '#size' => 64,
      '#weight' => '0',
    ];
    $form['submit'] = [
      '#type' => 'submit',
      '#value' => $this->t('Submit'),
    ];

    return $form;
  }

Is there an issue somewhere to make #title required?

To satisfy that WCAG success criterion, we must inform the user which input failed validation, ideally using text.

The FAPI does set name="enter_text_or_else" (in my example). So I think we can use a link to #enter_text_or_else (or whatever the key is in the form array). I think that @JacobRockowitz mentioned (during the Usability meeting) that the Webform module already does that. Maybe we could borrow some of his code.

I found the related issue about ensuring all FAPI elements have a #title.

Another WCAG success criterion, which I missed in #62, is "Name, Role, Value" (level A). It says all form inputs must have a programmatically-determinable accessible name. This is more strict than "tell the user how to fix the problem" - they still need to know what an input is called. The approach suggested here means we could satisfy the WCAG "Error Identification" and "Error suggestion" criteria, yet still fail "Name, Role, Value". The screenshot in #61 is an example of this - the user is aware of exactly which two inputs have errors, and that they must provide a value for both, but can't tell what each one is for.

I think it's still worth pursuing the related issue, and making FAPI #title mandatory, because it's the most reliable way to satisfy WCAG 4.1.2 "Name, Role, Value". The accessible name doesn't necessarily have to come via the FAPI #title - it could come from an aria-labelledby in #attributes, or a <label> created by a Twig template. But if #title isn't there, then this is a gap that module developers must fill by another means. A mandatory #title is way more robust for FAPI to ensure an associated label <label>, and wouldn't prevent themers taking different approach with aria attributes.

This is a tricky issue. On the one hand, it's worth doing because it adds a level of robustness for "Error Identification" in an unusual circumstance. Yet it still leaves a hole for "Name, Role, Value" and "Labels and Instructions", so I think ensuring a #title is the more important issue to tackle.

@gbirch and I worked on the steps to reproduce this issue at the ContributionWeekend2019 (Boston).

I am adding more explicit steps to reproduce, and I am summarizing Comments #62 and #64 in the issue summary.

Thanks Xano. Your patch works on my drupal 9.1.8

best regards,

Savage

Hi, all.
Drupal 9.1.10 - Could not apply patch #56

best regards,

Savage

Rerolled patch #56 for 9.5.x.

Fixing the custom command failed issue

Trying to fix failed tests of patch #79.

Applied patch in #77 in 9.5.x-dev. Adding results as screenshots

Tested the patch in #79

Confirmed the issue without the patch
Confirmed the patch fixed the issue.

Going to mark as reviewed but fyi there's https://www.drupal.org/project/drupal/issues/2568889#comment-14606806 to prevent the error from appearing all together because nothing was submitted.

There are two uncompleted tasks in the Issue Summary. Setting back to NW for those items.

This is tagged for an accessibility review and that has not happened. Can someone here take that on?

Tagging for WCAG 3.3.1.

#933004: Test that all form elements have a #title for accessibility needs review, and I think it will fix the problem in this issue.

Sounds like @andrewmacpherson also suggested that it will in #64.

I don't think this needs backport to D7 anymore.