Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validation error for date_popup is missing the element title #6274

Closed
bugfolder opened this issue Oct 22, 2023 · 6 comments · Fixed by backdrop/backdrop#4560
Closed

Validation error for date_popup is missing the element title #6274

bugfolder opened this issue Oct 22, 2023 · 6 comments · Fixed by backdrop/backdrop#4560

Comments

@bugfolder
Copy link

bugfolder commented Oct 22, 2023

Description of the bug

For the date_popup element, when a required date is empty, the validation error is missing the element title.

image

Steps To Reproduce

To reproduce the behavior:

Add this element to a form.

  $form['my_date'] = array(
    '#type' => 'date_popup',
    '#date_format' => 'l F j, Y',
    '#title' => t('My date'),
    '#description' => t('Enter the date.'),
    '#required' => TRUE,
  );

Leave the field blank and submit it, which should create a form error for the missing #required value.

Actual behavior

The error message is "A valid date is required for .".

Expected behavior

The error message should be "A valid date is required for My date.".

Additional information

The error message is generated in function date_popup_validate(), via this bit of code:

    if ($element['#required']) {
      $message = t('A valid date is required for %title.', array('%title' => $label));
      form_set_error($error_field, $message);
      return;
    }

The variable $label is set via this code earlier in the function:

  $label = '';
  if (!empty($element['#date_title'])) {
    $label = t($element['#date_title']);
  }
  elseif (!empty($element['#title'])) {
    $label = t($element['#title']);
  }

So in this example, $label is set from the #title property. However, by the time this code is reached, #title has already been set to NULL.

This is because date_popup_element_process() creates child date and/or time parts, sets their title(s) from $element['#title'], and then clears $element['#title'] (lines 1013–1014 of date.elements.inc):

  // Remove the title from the overall element.
  $element['#title'] = NULL;

So it looks like either the element #title should not be cleared here so it remains available to the validation error, or the validation code should check for the child element title(s) when creating the validation error message.

@argiepiano
Copy link

argiepiano commented Oct 23, 2023

I can confirm this issue.

A possible solution is to assign the title to $element['#date_title'] right before assigning NIULL to $element['#title'] in date_popup_element_process(). This is what date_combo_element_process() does (a process function that is used for date fields). The function date_popup_validate() actually checks to see if #date_title has a value, and if it does, it uses that for the label.

As I was testing this, I found a fatal error for date fields that use a popup, when the date is left blank. I'll create a separate issue.

@bugfolder
Copy link
Author

bugfolder commented Nov 5, 2023

I've made the change you suggested @argiepiano, and that fixes this problem. But in testing, I found another related problem in validation for date_popup: if you put in a bogus date, there is missing space in the error message:

image

The at-fault code is also in date_popup_validate:

  if (empty($date) || !empty($date->errors)) {
    if (is_object($date) && !empty($date->errors)) {
      $message = t('The value input for field %field is invalid:', array('%field' => $label));
      $message .= count($date->errors) ? (' ' . implode('', $date->errors)) : ('<br />' . implode('<br />', $date->errors));
      form_set_error($error_field, $message);
      return;
    }
...

We can fix this by adding space to that first implode:

       $message .= count($date->errors) ? (' ' . implode(' ', $date->errors)) : ('<br />' . implode('<br />', $date->errors));

But wait. date_text_validate() and date_select_validate() both use theme('item_list', ...) to theme the error list. So it would seem that date_popup_validate() should theme the errors the same way, using item_list.

Oh, and the title is also missing for a date_text. So probably both of those should be fixed as part of this issue.

image

@bugfolder
Copy link
Author

The fix for the title for blank entries for date_text can be the same as the fix for date_popup, just made in function date_text_element_process().

@bugfolder
Copy link
Author

Here's a form snippet for testing all three at once:

  $form['my_popup_date'] = array(
    '#type' => 'date_popup',
    '#date_format' => 'l F j, Y',
    '#title' => t('My popup date'),
    '#required' => TRUE,
    '#size' => 40,
  );
  $form['my_text_date'] = array(
    '#type' => 'date_text',
    '#date_format' => 'l F j, Y',
    '#title' => t('My text date'),
    '#required' => TRUE,
    '#size' => 40,
  );
  $form['my_select_date'] = array(
    '#type' => 'date_select',
    '#date_format' => 'l F j, Y',
    '#title' => t('My select date'),
    '#required' => TRUE,
    '#size' => 40,
  );

With the changes described above, here's the result with blank entries:

image

And here's the result with bogus entries:

image

PR to follow.

@argiepiano
Copy link

@bugfolder thanks so much for providing this PR. I've tested, and it works for me. Code LGTM.

There are a couple of things I've noticed, which are unrelated to this fix and were present before patching:

  1. There is a lonely comma in the popup field after submitting invalid values (this is shown in your screen capture above)
  2. The red asterisk is not shown in the text input, even though the field is required

I'll submit two issues for this.

@quicksketch
Copy link
Member

Thanks @argiepiano and @bugfolder! I merged backdrop/backdrop#4560 into 1.x and 1.26.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants