Wrong validation messages in Datelist::validateDatelist()

Before this patch, the test of creating datetime field entity was passing nicely.

After this patch, Drupal is throwing an error (in \Drupal\datetime\Tests\DateTimeFieldTest::testDatelistWidget()) when testing creating an entity with datetime values:

This value should be of the correct primitive type.

Screenshot of the output given on failure:

Previous patch returned multiple errors instead of just one. I created this patch and ran tests for DateTimeFieldTest and everything seems to be ok.

I think the bug is in the datetime widget itself, because the "Authored on" form element on the node form works fine without any errors.

The test for TimeDateField has been update to include the missing field name. Current test skips over the first part of the error message to ignore this core bug.

Reassigning this to datetime.module as it looks like a bug with it. This may be related to a patch where some of the logic where the widget changed, but I need to dig to find the issue.

Just stripping out the test changes to make sure the changes do catch the bug.

Because of the patch this is an invalid test-only patch. Manual testing is needed to demonstrate the problem.

Back to NR.

Retesting to see where this lands w/ 8.1.x. May or may not conflict with #2696353: Bad dates in Select List widget throw an exception.

Back to NR. Will look closer at this, but

+++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
@@ -738,7 +739,7 @@ function testInvalidField() {
-    $this->assertText('date is invalid', 'Empty date value has been caught.');
+    $this->assertText(format_string('The @field_name date is invalid', array('@field_name' => $field_label)), 'Empty date value has been caught.');
 

I think with the expected string we are testing for, we want to use t() instead of format_string(). Also elsewhere.

+++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
@@ -746,7 +747,7 @@ function testInvalidField() {
-    $this->assertText('date is invalid', format_string('Invalid year value %date has been caught.', array('%date' => $date_value)));
+    $this->assertText(format_string('The @field_name date is invalid', array('@field_name' => $field_label)), format_string('Invalid year value %date has been caught.', array('%date' => $date_value)));

I think the format_string() in the messages is overkill. We can just say ''Invalid year failed validation.' or similar. Also elsewhere.

+++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
@@ -738,7 +739,7 @@ function testInvalidField() {
-    $this->assertText('date is invalid', 'Empty date value has been caught.');
+    $this->assertText(format_string('The @field_name date is invalid', array('@field_name' => $field_label)), 'Empty date value has been caught.');

The expected strings should use t() and not format_string(). That is the general convention in tests. This happens several times in the patch.

While we are at it, we should update DateTimeFieldTest::testInvalidField() to check for the updated error message on date-only fields to prevent regressions.

This one is a little hard to test. Using FF and disabling Javascript to prevent the datepicker polyfill is the best way to manually tests this.

Re-roll against 8.2.x.

+++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
@@ -311,7 +311,7 @@ public static function validateDatelist(&$element, FormStateInterface $form_stat
-        $form_state->setError($element, t('The %field date is required.'));

Wow, that's a pretty big oversight indeed!

Fix looks good to go.

I agree this is a bug and should be considered for a minor release of 8.2.x.

Thanks all!

+++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
@@ -736,7 +737,7 @@ function testInvalidField() {
-    $this->assertText('date is invalid', format_string('Invalid hour value %time has been caught.', array('%time' => $time_value)));
+    $this->assertText(format_string('The @field_name date is invalid', array('@field_name' => $field_label)), format_string('Invalid hour value %time has been caught.', array('%time' => $time_value)));

Instead of using the deprecated format_string(), we can just concatenate strings inline, since this is an assertion. See #2549805: Remove all usage of FormattableMarkup in tests apart from explicit tests of that API. (Also, a separate message assertion parameter is usually not useful with assertText() anyway since it is redundant and obscures the actual validation message, but that's not introduced here.)

+++ b/core/includes/theme.inc
@@ -568,9 +568,11 @@ function template_preprocess_datetime_form(&$variables) {
+  $element['#title_display'] = isset($element['#title_display']) ? $element['#title_display'] : 'before';
 
   if (!empty($element['#title'])) {
     $variables['title'] = $element['#title'];
+    $variables['title_display'] = $element['#title_display'];

+++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeWidgetBase.php
@@ -23,7 +23,6 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
-    $element['#theme_wrappers'][] = 'datetime_wrapper';

@@ -32,6 +31,8 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+      '#title' => $element['#title'],
+      '#title_display' => 'invisible',

Are these bits a required part of the fix?

This backs out the theme changes; the test changes are not in this.

Pretty sure it will fail b/c

+++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
@@ -311,7 +311,7 @@ public static function validateDatelist(&$element, FormStateInterface $form_stat
-        $form_state->setError($element, t('The %field date is required.'));
+        $form_state->setError($element, t('The %field date is required.', array('%field' => !empty($element['#title']) ? $element['#title'] : '')));

However,

+++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeWidgetBase.php
@@ -23,7 +23,6 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
-    $element['#theme_wrappers'][] = 'datetime_wrapper';
     $element['#attributes']['class'][] = 'container-inline';

entangles this with #2788359: datetime_wrapper improperly applied to DateTimeWidgetBase and/or #1918994: Improve Datetime and Daterange Widget accessibility. I think we may want to postpone on the first of those.

Postponing on #2788359: datetime_wrapper improperly applied to DateTimeWidgetBase

I needed this patch for Drupal 8.3. So i fixed the patch to work with that release.
I also added t() function to the placeholder %part in the setError message since this value was never translated to another language.
I know this issue is postponed but I needed this and maybe this patch will help someone =)

This is good to work on again, I missed it in my sweep of #2788359: datetime_wrapper improperly applied to DateTimeWidgetBase and #1918994: Improve Datetime and Daterange Widget accessibility.

The patch in #28 should be rerolled, as that has the tests. Then we can look at #32.

Re-rolled.

+++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
@@ -735,7 +736,7 @@ public function testInvalidField() {
-    $this->assertText('date is invalid', 'Empty date value has been caught.');
+    $this->assertText(format_string('The @field_name date is invalid', ['@field_name' => $field_label]), 'Empty date value has been caught.');

We can just sprintf() these since we are trying to avoid these formatting functions in tests now.

Didn't look into why we still have a fail. I suspect we also want to add test coverage to DateRangeFieldTest.

Rerolled #28 on to make it apply in 8.3.x

The test changes were unrelated to the applied fix, I removed them and the test now passes. Test was moved to PHPUnit as well in the mean time.

I do think we need test coverage for the change to verify the label is now shown, only setting to needs review now to see if the test-bot agrees with the patch passing.

Interestingly though the element label for the invalid dates is not shown either right now, the placeholder in the HTML is empty:

The <em class="placeholder"></em> date is invalid.

Is the issue bigger or the test-case not good enough?

Sorry, garbage patch.

Fix in #42 has lost the tests from above.

@jhedstrom That was on purpose as my comment stated: the test changes were unrelated because the changes tested another area. It still needs tests though but at the moment I'm too busy with the PHPUnit initiative.

Working on this. The current fix only covers part of the problem and am adjusting the tests.

Should be green. There is probably a better way to get the field name in the $element here, but I am spacing. Also not 100% sure this will interfere w/ plain FormAPI usage of this.

The path through the invalid date logic requires a form alter to get past the option validation in order to generate an invalid date via the widget. Don't think this is worth it.

This looks good to me. One tiny nit:

+++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
@@ -311,12 +312,14 @@ public static function validateDatelist(&$element, FormStateInterface $form_stat
+        //print_r($element);

Stray debugging code here.

Oops.

Back to NR. CI error was was from one of the know, but fairly rare, PHP segfaults.

From looking at the code it appears this uses the machine name of the field in the error message that is presented to the user. It will look something like:

The field_event_dates date is invalid.

This is confusing to end users since the text field_event_dates is not visible in the page, the actual title will be something human readable like "Event date". We should use the human readable field title if possible.

I looked into this but the machine name being used instead of the human readable name appears to be a separate issue that pervades the entire datetime module. The other field formatters and error messages are also affected by this. This hasn't been detected before since in DateTestBase the same name is used for both the machine name and the human readable name when the field configuration is created :(

I will make a separate issue to use the human readable name in all error messages.

This means this issue is good to go. I reviewed the code and it looks good. I wasn't entirely sure about the language used in the error message, I would have chosen "The date for the %field field is incomplete/invalid" instead of "The %field date is incomplete/invalid". But since this has been written by native English speakers my suggestion is probably worse :)

I had a go at #2876047: Correctly mention field labels in form validation error messages and it is actually possible to use the human readable names in this issue too without affecting the other field type, since they are in separate classes.

Fixed the patch to use the human readable field name rather than the machine name.

+++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
@@ -302,6 +302,19 @@ public static function processDatelist(&$element, FormStateInterface $form_state
+    $title = '';
+    if (!empty($element['#title'])) {
+      $title = $element['#title'];
+    }
+    else {
+      $parents = $element['#array_parents'];
+      array_pop($parents);
+      $parent_element = NestedArray::getValue($complete_form, $parents);
+      if (!empty($parent_element['#title'])) {
+        $title = $parent_element['#title'];
+      }
+    }

Sa-weet. Do we want to move this logic to DateElementBase so we can reuse it in #2876047: Correctly mention field labels in form validation error messages?

Oh yeah that is a really good idea, we can create a helper function for it and reuse the code!

I see that I missed one test too, it also needs to be updated to use the $field_label = $this->field->label(); when checking the label of the fieldset.

One more test fix.

I postponed #2876047: Correctly mention field labels in form validation error messages on this issue since so it will be able to reuse the method on DateElementBase that @mpdonadio proposed in #56.

I think this is good to go. Thanks!

+++ b/core/modules/datetime/tests/src/Functional/DateTestBase.php
@@ -107,6 +107,7 @@ protected function setUp() {
+    $field_label = Unicode::ucfirst(Unicode::strtolower($this->randomMachineName()));

I like the use of proper upper case style standards =)

Committed fde3369 and pushed to 8.4.x. Thanks!

I'm not 100% sure on a backport to 8.3.x here. The string changes/additions are probably fine because it's explicitly fixing broken strings, and the new protected static method should also be OK, moving to RTBC against 8.3.x for now and will have a think.

8.3 stil green, back to rtbc

Discussed about backporting this. It's a new method on a base class, so to backport to 8.3.x we'd have to inline that logic, which is a fair bit of work/thought for a backport compared to cherry-pick. Given that, moving back to 8.4.x and fixed.