Bad dates in Select List widget throw an exception

The attached patch fixed this. Tested manually on 8.0.x and 8.2.x versions.

Thanks for the report and the fix.

This is a bug, and we can get this into Drupal 8.1, probably in a point release after 8.1.0.

We need a test to demonstrate the problem, and that the patch address it. Also not 100% sure the raw exception message is the most appropriate to use in this case; "The array contains invalid values" doesn't made sense to show a user when they have invalid form input. Open to suggestions here.

Please find new patch which:
- have automated test, when content type with datetime selector is created and used;
- error message changed to "Selected combination of day and month is not valid." since this seems to be the only reason of datetime cannot be created. Other problems like missing year/month etc already handled outside of issue scope.

@dpovshed, again, thanks for helping out with this.

Typically, with bugs that introduce new tests, we post a test-only patch that shows the bug (ie, a failing test), and then the patch that fixes the big (and then comes up green). As long as you attach the test-only patch first, the status is OK.

Also, we try to post interdiffs with new patches, to make it easier to see what has changed.

@mpdonadio, thanks for cooperation and your help to tune up my work to comply Drupal standards.

As I see now we do have failed test only (#7) and test with a fix (#6). Just order is a different if compare to ideal Drupal process. If you agree with proposed message to the user - maybe you can move forward with #6 manually?

Please advice!

Yup, out of order patches happens all the time. I will look at #6 closer tonight (prefer final review applied, so I can see everything in context), but I think

+++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
@@ -66,7 +66,12 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
+        try {
+          $date = DrupalDateTime::createFromArray($input, $timezone);
+        }
+        catch (\Exception $e) {
+          $element['#errors'][] = t("Selected combination of day and month is not valid.");
+        }
         if ($date instanceof DrupalDateTime && !$date->hasErrors()) {

message looks good, but think it should be static::t() to avoid using the global (nearly sure that trait should be on that class).

@mpdonadio, I just looked more carefully for translation in the class. There are a several more usages of t() across the file - for examlpes, see the processDatelist(), validateDatelist() functions.

So I propose to accept patch from #6 if there are no other issues, and to create separate task to migrate t() to static::t() . Surely, this may be simple enough for you to do so right in the code by yourself. I just want to stress that if we go for replacement t() with static::t() withing current issue scope this could be kind a mixing the subjects.

Let me know what you think!

Typically, when we introduce new code, we try to follow best practices even if other places in the code don't do this. So, in this case we would do the static::t() in this patch in the usage you introduce, and then do a followup to replace t() with static::t().

I had a last minute deploy last night, so I didn't get to look at this closely. I want to be double sure the test coverage is OK so prevent future regressions. I will try to do so tonight.

@mpdonadio, OK, take your time to test that, I will wait for your feedback.

1. +++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
   @@ -66,7 +66,12 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   +          $element['#errors'][] = t("Selected combination of day and month is not valid.");
   +        }
   

   Let's change this to static::t()

2. +++ b/core/modules/system/src/Tests/System/DateTimeTest.php
   @@ -161,4 +168,66 @@ function testDateFormatConfiguration() {
   +    $this->drupalPostForm('node/add/page_with_date', $edit, t('Save'));
   +    $this->drupalGet("node/1");
   +    $this->assertResponse(404, "Node not created because the date is invalid.");
   +
   

   I don't think this test is valid; something else could potentially cause the fail here unrelated to this patch and end up with the node not being created. I would skip the $this->drupalGet("node/1"); and just check for the error message.

This is a nit, but as much as possible try to use single quoted strings unless you need double-quoted ones.

Can we also add coverage for a datetime field configured for date-only?

Thanks for the review!

I'd appreciate hints for the following:

1) could you please point me to correct migration from t() to static::t() ? By simple replacement or by trying to mimic approach from Views.php (the only file I found usage of static::t), I had no success ended up with some Exceptions.
2) for some reason I was unable to detect error message text with assertText() - probably it is outputted via AJAX or some other way. Any ideas?

OK, let's bail on the static:t(). Views is defining its own. My bad.

The test is failing because that string is coming from a %placeholder, so it is wrapped in an em (inspect element and look at it). However, it also looks like your specific error message is being ignored, and the generic one is used instead. I think you need to update Datelist::validateDatelist() to check the #errors, and use it if set (maybe join('
')?; not much precedence in core that I can see). Then in your test you can check for that specific error message.

Let's try this.

@mpdonadio, I reviewed your version and tested it locally both manually and via running the tests.

Thanks for finding time to fine tune the stuff!

I like what I see so I am voting for going forward with #17.

Unfortunately, with the review policy for patches, we have both had our hands in this patch now, so it wouldn't be appropriate for either of us to set RTBC. I will see if @jhedstrom can take a look.

This looks good. I've also manually tested this patch to verify the bug and fix.

Committed 69d9901 and pushed to 8.1.x and 8.2.x. Thanks!