Datetime form element cannot validate when using a format without seconds

+++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
@@ -76,7 +76,7 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
+      if (!empty($time_input) && strlen($time_input) == 5 && strrpos($time_format, ':s') == strlen($time_format) - 2) {

In this case, you assume everyone will use H:i:s format. Some country could use dot instead of colon. (or another format).

I think the all condition is wrong. Basing on a 5 length is also wrong, this depends of format. If format is 'H : i', strlen will be > 5.

I'm not even sure this condition should be there. If format is H:i:s user should input seconds. If it doesn't, a message is displayed.
If we take care of this specific case, why don't we take care of missing minutes ? (i don't think we should neither)

Sorry to answer review by other questions and put the issue in another way.

#8, sorry, I was away this weekend.

This is a head scratcher. I think this is OK, but I want to look at the test in context and see what is going on with the debugger.

I am leaning towards fixing issue issue here, and creating a followup to handle the fact that this is a fragile piece of code that makes some bold assumptions about the time format.

[Sorry, I found some small revisions to this after I posted.]

Played with this for a while applied...

1. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
   @@ -76,7 +76,7 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   -      if (!empty($time_input) && strlen($time_input) == 5) {
   +      if (!empty($time_input) && strlen($time_input) == 5 && strrpos($time_format, ':s') == strlen($time_format) - 2) {
            $time_input .= ':00';
   

   I'm not sure this is doing what you think it is doing. When `$time_format = 'H;s', then `strrpos('H:i', ':s') === FALSE`, so this final clause will always be FALSE, so the seconds are never appended. In fact, I can remove this `if` and the test passes. DrupalDateTime::createFromFormat() (which essentially calls \DateTime::createFromFormat()), is pretty unforgiving with formats. I'm not sure why that `if()` was there (I added the related issues, but didn't slog through them).

2. +++ b/core/modules/datetime/src/Tests/DateFormTest.php
   @@ -0,0 +1,33 @@
   +    $errors = $this->xpath('//div[contains(@class, "messages--error")]');
   +    $this->assertFalse($errors, 'No error message shown.');
   +  }
   

   I think it would be safer to check for the "Success" message and that all of the elements have the expected values. Instead of using date(), just hardcode some values so you can explicitly test expectations.

3. +++ b/core/modules/datetime/tests/modules/datetime_test/datetime_test.routing.yml
   @@ -0,0 +1,7 @@
   +    _access: 'TRUE'
   \ No newline at end of file
   diff --git a/core/modules/datetime/tests/modules/datetime_test/src/Form/DateForm.php b/core/modules/datetime/tests/modules/datetime_test/src/Form/DateForm.php
   

   Nit, needs newline.

4. +++ b/core/modules/datetime/tests/modules/datetime_test/src/Form/DateForm.php
   @@ -0,0 +1,50 @@
   +    $form['datetime_without_seconds'] = [
   +      '#type' => 'datetime',
   +      '#title' => $this->t('Datetime without seconds'),
   +      '#default_value' => new DrupalDateTime(),
   +      '#date_time_element' => 'text',
   +      '#date_time_format' => 'H:i',
   +    ];
   

   `'#default_value'` should be NULL. In your submit handler, use `$form_state->setRebuild();` so the submitted values override this. Then you have something to explicitly test against.

5. +++ b/core/modules/datetime/tests/modules/datetime_test/src/Form/DateForm.php
   @@ -0,0 +1,50 @@
   +  }
   +}
   

   Nit, need blank line before final brace. Also in DateFormTest.

Not sure if the test really belong in the datetime.module (maybe system.module would be better), but here is my take on the proper fix and expanded test coverage. The change to the Datetime class is best read applied so you can see it in context of what is going on and why removing that if() statement is OK.

1. +++ b/core/modules/datetime/src/Tests/DateFormTest.php
   @@ -0,0 +1,44 @@
   +    $edit = [
   +      'datetime_with_seconds[date]' => '2016-01-02',
   +      'datetime_with_seconds[time]' => '13:14:15',
   +      'datetime_without_seconds[date]' => '2016-03-04',
   +      'datetime_without_seconds[time]' => '16:17',
   +    ];
   +    $this->drupalPostForm(NULL, $edit, 'Submit');
   

   Maybe add something with non traditional separators to make sure this works?

2. +++ b/core/modules/datetime/src/Tests/DateFormTest.php
   @@ -0,0 +1,44 @@
   +    $success = $this->xpath('//div[contains(@class, "messages--status")]');
   +    $this->assertTrue($success, 'Success message found.');
   

   Should check for both the message div and the message itself.

3. +++ b/core/modules/datetime/src/Tests/DateFormTest.php
   @@ -0,0 +1,44 @@
   +      $input = $this->xpath('//input[@name=:name and @value=:value]', [
   +        ':name' => $name,
   +        ':value' => $value
   +      ]);
   

   Nit, needs trailing comma after $value.

4. +++ b/core/modules/datetime/tests/modules/datetime_test/datetime_test.routing.yml
   @@ -0,0 +1,7 @@
   +    _access: 'TRUE'
   \ No newline at end of file
   diff --git a/core/modules/datetime/tests/modules/datetime_test/src/Form/DateForm.php b/core/modules/datetime/tests/modules/datetime_test/src/Form/DateForm.php
   

   Oops.

So I guess we are back to the approach in #2?

Reworked the patch to remove the dependency on datetime since this is about the form element. Also aligned it with #2840220: "Date and time" Form API element allows entry beyond min/max values so they will merge better (need to look at these closer for naming).

This can probably still go against 8.2.x since it is a bug, but leaving at 8.3.x for now.

+++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
@@ -76,7 +76,7 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
       // Seconds will be omitted in a post in case there's no entry.
-      if (!empty($time_input) && strlen($time_input) == 5) {
+      if (!empty($time_input) && strlen($time_input) == 5 && strrpos($time_format, ':s') == strlen($time_format) - 2) {

I had already made the patch, but if we should fix this comment before commit to make it more clear what this is really for.

#20. Thought about that on was to work this morning. One thought is a comment. Another is something like

+++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
@@ -76,7 +76,7 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
       // Seconds will be omitted in a post in case there's no entry.
-      if (!empty($time_input) && strlen($time_input) == 5) {
+      if (!empty($time_input) && strlen($time_input) == 5 && strrpos($time_format, ':s') !== FALSE && strrpos($time_format, ':s') == strlen($time_format) - 2) {

to explicitly check for ':s'. Otherwise, I am not really sure how we can make this bulletproof, but am open to ideas.

+++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
@@ -76,7 +76,7 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
       // Seconds will be omitted in a post in case there's no entry.
-      if (!empty($time_input) && strlen($time_input) == 5) {
+      if (!empty($time_input) && strlen($time_input) == 5 && strrpos($time_format, ':s') == strlen($time_format) - 2) {

I wonder if we should also (or instead of) check `'#date_time_element' === 'time'` since it looks like this was a Chrome problem w/ the HTML5 time element? If the input is 'text' then we can use it as-is?

Maybe

+++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
@@ -76,7 +76,7 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
       // Seconds will be omitted in a post in case there's no entry.
-      if (!empty($time_input) && strlen($time_input) == 5) {
+      if (!empty($time_input) && $element['#date_time_element'] === 'time' && strlen($time_input) == 5) {

is the better change here?

OK, change in #22 and make the test a BTB.

+++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
@@ -75,8 +75,9 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
+      // Ensure the time has a seconds portion in case it was omitted or
+      // stripped out by the browser.
+      if (!empty($time_input) && $element['#date_time_element'] === 'time' && strlen($time_input) == 5) {
         $time_input .= ':00';
       }

+++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestDatetimeTimeTest.php
@@ -0,0 +1,70 @@
+      '#date_time_element' => 'text',
...
+      '#date_time_element' => 'text',
...
+      '#date_time_element' => 'text',

The fix is checking explicitly for time, but all the tests are using text. That indicates we need more test coverage I think...

Nice catch.

I'll work on this

I will not work on this issue any more.

I will take this one.

According to #27 here is a try changing the tests checking for the correct #date_time_element.

Ok, I think we are chasing our tail here.

When you use 'date' and 'time' for '#datetime', the formats needs to be 'Y-m-d' and 'H:i:s'. When you use 'text', you can specify your own.

 /*
   * Optional properties include:
   *   - #date_date_format: A date format string that describes the format that
   *     should be displayed to the end user for the date. When using HTML5
   *     elements the format MUST use the appropriate HTML5 format for that
   *     element, no other format will work. See the format_date() function for a
   *     list of the possible formats and HTML5 standards for the HTML5
   *     requirements. Defaults to the right HTML5 format for the chosen element
   *     if a HTML5 element is used, otherwise defaults to
   *     DateFormat::load('html_date')->getPattern().
   */

We need to account for that, and Chrome stripping the :00 in both the patch and the test coverage.

#38

+++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
@@ -75,8 +75,9 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
-      // Seconds will be omitted in a post in case there's no entry.
-      if (!empty($time_input) && strlen($time_input) == 5) {
+      // Ensure the time has a seconds portion in case it was omitted or
+      // stripped out by the browser.
+      if (!empty($time_input) && $element['#date_time_element'] === 'time' && strlen($time_input) == 5) {
         $time_input .= ':00';
       }

I think we need to rethink the logic around this change and what we are actually solving. It could just be that we need to update the tests to test both 'text' and 'time', and then simulate what Chrome is going to do. And then document the change better. Need to think about this again.

Can anyone confirm the behavior in #17? I can't in Chrome 57.0.2987.110 on OSX.

Awesome.

OK, lets update the test to cover four scenarios:

1. HTML5 time w/ hours, minutes, second in the user input
2. HTML5 time w/ hours, minutes, in the user input (w/ comment that this is simulating Chrome)
3. Text input w/ hours, minutes, seconds in the user input in a H:i:s format
4. Text input w/ hours, minutes in the user input in a non H:i format

That should make sure this works as intended.

#42 @mpdonadio I hope these changes cover all needs. I'm not sure if the Chrome comment is in the best possible place, please, could you review it?

Note that when using the HTML 5 element, the format is up to the browser but the backend decides to display the error with a format that might be different:

          $form_state->setError($element, t('The %field date is invalid. Please enter a date in the format %format.', ['%field' => $title, '%format' => static::formatExample($format)]));

Which may result in a [dd][mm][yyyy] input (HTML5 element) with a warning to enter the date in the [yyyy][mm][dd] format (the date format for the field).

#44 is exactly my issue. We have a Dutch website, where a date is entered as DD/MM/YYYY, however if a user enters an invalid date, the error message tells the user to enter the date in a YYYY/MM/DD format. Which is impossible due the the formatting of the HTML5 element.

I can't figure out how to localize the html_date value (which is used by the getHtml5DateFormat() callback).

Patch #43 worked for me thank ,you.

But the time values in dropdown dont get the right format if I am on a multilingual site with translated formats it will always get the default format.

Correct format in form element.

False format in dropdown.

Rerolled for latest dev

The previous failed do to drupal_set_message() being deprecated. This uses the messenger service instead.

Module https://www.drupal.org/project/datetimehideseconds works great.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Using the test case that was added.

Can confirm the issue in question is fixed. When I don't include seconds it auto adds 00

It seems to be fixing but would like to have a followup to update the widget so that the seconds option isn't there.

Added patch against #57 in version 10.1

Hiding patch #65 as it's incomplete.

@Deshna Chauhan Please stop uploading incomplete and broken re-roll patches without an interdiff.

@Deshna When you notice that you patch is 1.66 KB instead of 6.79 KB a previous patch is, you should wonder if your patch does not miss some parts.
It happened to me too, and I learned to first check my patch size, before uploading it.

@Deshna Chauhan, I am removing credit for the unhelpful patch per How is credit granted for Drupal core issues.