Datetimelist element only works when all the fields are filled

No longer applies.

Exception shown in issue description no longer appear, so I think this issues no longer relevant.

Confirmed the bug. Edited IS to clarify; you need to use the Select List widget.

Depending on how you have your system configured, the actual error display may be different. I just got a generic

The website encountered an unexpected error. Please try again later.

but my PHP error log had

Uncaught PHP Exception Exception: "The array contains invalid values." at /Users/matt/Documents/PhpStorm/drupal-8.0.x/core/lib/Drupal/Component/Datetime/DateTimePlus.php line 143

.

Resolved above test fails by replacing date_increment_round with correct method.
Added simpletest checking for response to not filling out all fields; test only patch should fail, combined patch should pass.
Added a phpunit test for the DateTimePlus::checkArray() method. Although this method is not affected by the patch or involved in causing the above issue, additional phpunit test coverage is always helpful. We can remove it if not needed.

After applying patch #12, exception no longer appears. With some more testing it is good to make RTBC.

OK, spent some time with this.

+++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
@@ -317,7 +326,7 @@ public static function validateDatelist(&$element, FormStateInterface $form_stat
         // If the input is invalid, set an error.
         else {
-          $form_state->setError($element, t('The %field date is invalid.'));
+          $form_state->setError($element, t('The %field date is invalid.', array('%field' => !empty($element['#title']) ? $element['#title'] : '')));
         }
       }

We need to figure out a way to get a better error message in here for a partially filled in form. I think that just saying the field is invalid doesn't help the user with why the field is invalid. Maybe move the filled keys logic to a protected static the gets called from both the value and validate callbacks?

Separated out filled key logic into protected static as suggested in comment #15, updated to provide key of non-filled value. Updated message that is below the field to indicate which part of the element is invalid (not selected). Only the first empty part is reported in this iteration.

This looks good, except the message at the top of the form doesn't match the error under the form element. See the attached image. If we can fix that, I think this is good to go.

Raising this to Critical per the "Cause PHP fatal errors" and "Cause loss of data" clauses in the issue priorities. The data loss would be user input not being saved. I wouldn't object to this going back down to Major if the core committers don't think this warrants being Critical.

Thanks @mpdonadio!

I think this is major: when the user uses this one particular kind of field and forgets to fill out part of it, they get an exception (or, with error reporting off, an unhelpful error) instead of the expected validation error. There is no data loss because (as far as I can tell), the form submission just doesn't work at all, and the user does receive an error.

This kind of bug used to fit https://www.drupal.org/core/issue-priority#major-bug but catch added "non-fatal" there recently in https://www.drupal.org/node/45111/revisions/view/8664850/8729606 -- so I'll leave this critical for now until we discuss that (we'll triage it later this week). "Race condition or database deadlock" doesn't seem to apply to this sort of fatal during form validation, but I could be mistaken. Edit: see next comment. :)

But, in general, I don't think lost input with a big flaming error is data loss, because the user will know the submission failed. :) It'd be data loss if the bug also deleted existing data, or perhaps if it failed silently but seemed to work, saving data different from what the user submitted.

Wait. If it's a cleanly thrown exception, it's not a "PHP fatal". Just checked the code and confirmed this. So downgrading. Thanks!

Was just reading this again.

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
@@ -533,4 +549,32 @@ public function providerTestDateTimestamp() {
+
+      // Checks for invalid time pairings
+      array(FALSE, array('year' => 2013, 'month' => '12', 'day' => '11', 'hour' => '10', 'minute' => '61')),
+      array(FALSE, array('year' => 2013, 'month' => '12', 'day' => '11', 'hour' => '25', 'minute' => '09')),
+    );

These may be flagged as out of scope changes since they are unrelated to empty fields.

+++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
@@ -317,13 +324,37 @@ public static function validateDatelist(&$element, FormStateInterface $form_stat
+   * Checks the input array for empty values.
+   *
+   * Input array keys are checked against values in the parts array. Elements
+   * not in the parts array are ignored. If no empty values are found, null is
+   * returned. Otherwise, returns the first key in the input array that has an
+   * empty value.
+   *
+   * @param array $input
+   *
+   * @param array $parts
+   *
+   * @return string
+   *
+   */

The @param and @return should have descriptions, and the @return should be string|null.

I'm also wondering if we can rework checkEmptyInputs() to return the empty keys. Then the is_null() in valueCallback() could become empty(), and we could use all of the empty keys in the error message in validateDatelist()?

Will remove the unrelated tests and update the docblock. We can return all of the empty keys as an array or such and adjust the setError to have the message at the top match the empty fields, I agree it is a better user experience if we do that. However, this requires that we update the element processing:

public static function processDatelist(&$element, FormStateInterface $form_state
         '#attributes' => $element['#attributes'],
         '#options' => $options,
         '#required' => $element['#required'],
-        '#error_no_message' => TRUE,
+        '#error_no_message' => FALSE,
       );
     }

The format of the resulting error notifications is a little messy though:

Updated patch that produces the validation notifications as displayed in #22

Additionally,
Removed the unrelated test (unit test of DateTimePlus::checkArray). Made minor update to the applicable simpletest to cover the singular and plural form of the validation message.
Updated docblock for the added method Datelist::checkEmptyInputs()

Uploading the modified test-only patch, which should fail.

+++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
@@ -317,13 +326,38 @@ public static function validateDatelist(&$element, FormStateInterface $form_stat
+   * @param array $parts
+   *   Array to check input against, ignoring elements not in this array
+   * @return array
+   *   Array of keys from the input array that have no value, may be empty
+   */

Minor nit that can be fixed on commit, but per https://www.drupal.org/node/1354, the different sections in a docblock should have a blank line between them, so there should be one between the last @param and the @return.

The code looks good, and I did a fair amount of manual testing with it. The formatting on the error message is unfortunate, but I can't think of any other options (and I tried several).

I'm going to leave this at Needs Review for a few days to see if anyone has a better idea for how to handle this. If we don't get any more comments, I'll RTBC it.

Looking good, couple of observations/questions/suggestions

1. +++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
   @@ -55,19 +55,21 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   +          if ($input['ampm'] == 'pm' && $input['hour'] < 12) {
   ...
   +          elseif ($input['ampm'] == 'am' && $input['hour'] == 12) {
   

   Should these be using ===?

2. +++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
   @@ -309,6 +312,12 @@ public static function validateDatelist(&$element, FormStateInterface $form_stat
   +        foreach ( $all_empty as $value ){
   

   nit: should be no space after ( or before ), but should be one after )

3. +++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
   @@ -309,6 +312,12 @@ public static function validateDatelist(&$element, FormStateInterface $form_stat
   +          $message = t('A value must be selected for %part.', array('%part' => $value));
   +          $form_state->setError($element[$value], $message);
   

   Any reason this can't be one line?

4. +++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
   @@ -317,13 +326,38 @@ public static function validateDatelist(&$element, FormStateInterface $form_stat
   +   *   Array of individual inputs to check for value
   ...
   +   *   Array to check input against, ignoring elements not in this array
   ...
   +   *   Array of keys from the input array that have no value, may be empty
   

   nit:If we're re-rolling to add a newline above the @return, can we put full stops on the end of these at the same time?

5. +++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
   @@ -317,13 +326,38 @@ public static function validateDatelist(&$element, FormStateInterface $form_stat
   +  protected static function checkEmptyInputs($input, $parts) {
   

   I think this can be done with array_diff

   array_diff($parts, array_keys($input));
   

   Might need an array_filter if some keys are there but missing values

6. +++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
   @@ -317,13 +326,38 @@ public static function validateDatelist(&$element, FormStateInterface $form_stat
   +      if (empty($input[$part])) {
   

   Isn't 0 a valid value?

7. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
   @@ -478,6 +478,33 @@ function testDatelistWidget() {
   +    $date_value = array('year' => 2012, 'month' => '', 'day' => '', 'hour' => '', 'minute' => '');
   

   Can we test what happens if 0 is submitted for hour or minute, the empty() above will report it as missing I suspect

8. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
   @@ -478,6 +478,33 @@ function testDatelistWidget() {
   +    $this->assertTextPattern('/(error has)|(errors have) been found/', 'Date validation error found');
   

   Can we make this more specific?

Thanks larowlan for these.

Setting back to needs work since #27.6 points out a failure in the existing logic (hour and minutes can be 0) and tests should be expanded to check for these as noted. Additional comments should be addressed as well since the patch will need to be updated.

While working on this, a related issue was found.

If the submitted date (in datelist format) fails validation, such as not selecting one of the date elements, selected values that are 0 become unselected after the validation error is displayed.

This image should describe what is happening. Note that the minute field is selected as "00" by the user and is now blank after validation.

The cause of this is a related usage of empty()

public static function processDatelist(&$element, FormStateInterface $form_state, &$complete_form) {
....
$default = !empty($element['#value'][$part]) ? $element['#value'][$part] : '';

Also note that is corrected when the submitted values pass validation by the subsequent line:

$value = $date instanceOf DrupalDateTime && !$date->hasErrors() ? $date->format($format) : $default;

I should have an update patch for the previous issues sometime today. Any input on this new issue is welcome.

Updated patch and tests. All comments have been addressed except #27.1, I think the == is sufficient here since the input options are known.

-Modified logic in Datelist::checkEmptyInputs()
-Added tests to check the selection of 0 as hour and minutes
-Replaced empty() in processDatelist, resolving the bug noted in #29, and added test for this scenario to ensure selected value is not deselected
-Additional style updates as noted in #25 & #27

Upload modified test only patch, which should fail.

I think the comments in #27 have been addressed, as well as the additional in-scope bug in #29.

We have a good IS, a Beta Eval, a test that demonstrates the problem, and a good patch. I also manually tested the latest patch.

I think this is good to go.

Committed 3843053 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.