Fix DateTimeType' HTML input format according to HTML specs. CurrentlyDateTimeType produces html with formatyyyy-MM-dd'T'HH:mm:ssZ but the HTML5 spec expectsyyyy-MM-dd'T'HH:mm:ss (i.e. noZ). Chrome presents an empty date picker meaning edits or having a default date are broken.

Also the reverseTransform was expect to have a timezone attached, which it does not - and incorrectly marks it as being a UTC time in this case, instead of using the Transformers output TZ.

This is same as@franzwilding #27254 but with change to just straight use ofDateTime::format and handling TZ in reverseTransform

carsonbot added Status: Needs Review Form Bug labels

Sep 5, 2018

mcfedr force-pushed thedatetime-html5-fix branch 7 times, most recently from374dfbf to67a95f4Compare

September 5, 2018 18:37

nicolas-grekas added this to the2.8 milestone

Sep 6, 2018

nicolas-grekas reviewed

Sep 6, 2018

View reviewed changes

src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php Outdated

		useSymfony\Component\Form\Extension\Core\DataTransformer\ArrayToPartsTransformer;
		useSymfony\Component\Form\Extension\Core\DataTransformer\DataTransformerChain;
		useSymfony\Component\Form\Extension\Core\DataTransformer\DateTimeToArrayTransformer;
		useSymfony\Component\Form\Extension\Core\DataTransformer\DateTimeToHTML5DateTimeLocalTransformer;

Copy link

Member

nicolas-grekasSep 6, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

case mismatch (see failing tests): should beHtml5

Copy link

ContributorAuthor

mcfedrSep 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Oops! Thanks.

mcfedr force-pushed thedatetime-html5-fix branch from67a95f4 to8839532Compare

September 6, 2018 17:11

nicolas-grekas approved these changes

Sep 7, 2018

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM with just one minor comment

...ny/Component/Form/Extension/Core/DataTransformer/DateTimeToHtml5DateTimeLocalTransformer.php Outdated

		*/
		class DateTimeToHtml5DateTimeLocalTransformerextends BaseDateTimeTransformer
		{
		constHTML5_FORMAT ="Y-m-d\TH:i:s";

Copy link

Member

nicolas-grekasSep 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

should use single quotes

Copy link

ContributorAuthor

mcfedrSep 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Fixed, I've also put '\T' in the middle, as it looks more correct, it works with single\ because\T is not a valid combo, but really for a literal\ it should be\\

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Sep 7, 2018

nicolas-grekas mentioned this pull request

Sep 7, 2018

[Form] Fix DateTimeType html5 input format.#27254

Closed

[Form] Fix DateTimeType html5 input format

253d0a6

mcfedr force-pushed thedatetime-html5-fix branch from8839532 to253d0a6Compare

September 7, 2018 13:54

Copy link

Member

javiereguiluz commentedSep 10, 2018

This is a bit confusing:

Resources like MDN never include theZ in the datetime format:https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/datetime-local
The official HTML5 spec seems to say that it's OK and valid to include it:https://www.w3.org/TR/2010/WD-html5-20101019/common-microsyntaxes.html#concept-datetime

Copy link

Contributor

franzwilding commentedSep 10, 2018

@javiereguiluz The problem is, that Chrome do not supportZ in datetime-local inputs, even if w3c says it is OK.

Copy link

Member

javiereguiluz commentedSep 10, 2018

I see. In the past, we usually ignored issues like this considering them as browsers bugs (I remember a problem with a Safari-only behavior). But this is frustrating for users and Chrome is the most popular browser ... so it will be a tough decision.

Copy link

Contributor

franzwilding commentedSep 10, 2018

@javiereguiluz I think in this case it is less a browser specific hack and more a more correct implementation of the datetime-local draft specs.

I think the whole idea of datetime-local is to be a datetime field without a timezone, this is also the description of the datetime-local field:https://www.w3.org/TR/2012/WD-html-markup-20120315/input.datetime-local.html

Also, when you see the reference of the date + time format (https://www.w3.org/TR/2012/WD-html-markup-20120315/datatypes.html#form.data.datetime-local) it clearly says that there should not be a timezone suffix.

Copy link

Member

javiereguiluz commentedSep 10, 2018

@franzwilding thanks for the details. It's more clear to me now. Approved it!

javiereguiluz approved these changes

Sep 10, 2018

View reviewed changes

Copy link

ContributorAuthor

mcfedr commentedSep 10, 2018

I refered to the whatwg spec, which does not includeZ - I think some of the confusion here is the difference betweentype=datetime andtype=datetime-local, the former is deprecated and not used by symfony. It allowed timezones, but the handling was inconsistent and that is why its been removed.

datetime-local
date time format

xabbuh mentioned this pull request

Sep 14, 2018

DateTimeType HTML5 Chrome#22063

Closed

xabbuh reviewed

Sep 15, 2018

View reviewed changes

...ny/Component/Form/Extension/Core/DataTransformer/DateTimeToHtml5DateTimeLocalTransformer.php

		}

		try {
		$dateTime =new \DateTime($dateTimeLocal,new \DateTimeZone($this->outputTimezone));

Copy link

Member

xabbuhSep 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think we should check the format of the submitted date first to avoid issues like#28455 (the approach could IMO be similar to what I propose in#28466).

Copy link

ContributorAuthor

mcfedrSep 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Makes sense, I've added something similar here.

mcfedr added2 commits

September 15, 2018 14:25

Add stricter checking for valid date time string

4f06f15

Added relevent links for parsing to the phpdoc

e21a1a4

mcfedr force-pushed thedatetime-html5-fix branch fromd6f0029 toe21a1a4Compare

September 15, 2018 11:34

xabbuh approved these changes

Sep 15, 2018

View reviewed changes

Copy link

Member

nicolas-grekas commentedSep 17, 2018

Thank you@mcfedr.

nicolas-grekas merged commite21a1a4 intosymfony:2.8

Sep 17, 2018

nicolas-grekas added a commit that referenced this pull request

Sep 17, 2018

bug#28372[Form] Fix DateTimeType html5 input format (franzwilding, …

8d90df7

…mcfedr)This PR was merged into the 2.8 branch.Discussion----------[Form] Fix DateTimeType html5 input format| Q             | A| ------------- | ---| Branch?       | 2.8| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#27233,#27254| License       | MIT| Doc PR        | N/AFix DateTimeType' HTML input format according to HTML specs. Currently `DateTimeType` produces html with format `yyyy-MM-dd'T'HH:mm:ssZ` but the HTML5 spec expects `yyyy-MM-dd'T'HH:mm:ss` (i.e. no `Z`). Chrome presents an empty date picker meaning edits or having a default date are broken.Also the reverseTransform was expect to have a timezone attached, which it does not - and incorrectly marks it as being a UTC time in this case, instead of using the Transformers output TZ.This is same as@franzwilding#27254 but with change to just straight use of `DateTime::format` and handling TZ in reverseTransformCommits-------e21a1a4 Added relevent links for parsing to the phpdoc4f06f15 Add stricter checking for valid date time string253d0a6 [Form] Fix DateTimeType html5 input format

mcfedr deleted the datetime-html5-fix branch

September 18, 2018 10:06

Copy link

Member

nicolas-grekas commentedSep 18, 2018

Could anyone please have a look at why this PR makes tests for 4.1 to fail?
See e.g.https://travis-ci.org/symfony/symfony/jobs/430138930

mcfedr mentioned this pull request

Sep 18, 2018

[Form] Fix expected values in datetime-local test#28504

Merged

nicolas-grekas added a commit that referenced this pull request

Sep 18, 2018

minor#28504[Form] Fix expected values in datetime-local test (mcfedr)

4879a75

This PR was submitted for the master branch but it was merged into the 4.1 branch instead (closes#28504).Discussion----------[Form] Fix expected values in datetime-local test| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#28372| License       | MIT| Doc PR        | N/ACommits-------5ed8b27 Fix expected values in datetime-local test

xabbuh mentioned this pull request

Sep 19, 2018

[Form] fail reverse transforming invalid RFC 3339 dates#28466

Merged

This was referencedSep 30, 2018

Release v2.8.46#28640

Merged

Release v3.4.16#28641

Merged

Release v4.1.5#28642

Merged