Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Form] Fix DateTimeType html5 input format #28372

Merged
merged 3 commits into from Sep 17, 2018

Conversation

Projects
None yet
6 participants
@mcfedr
Copy link
Contributor

commented Sep 5, 2018

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/A

Fix 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 reverseTransform

@mcfedr mcfedr force-pushed the mcfedr:datetime-html5-fix branch 7 times, most recently from 374dfbf to 67a95f4 Sep 5, 2018

@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Sep 6, 2018

@@ -15,8 +15,8 @@
use Symfony\Component\Form\Extension\Core\DataTransformer\ArrayToPartsTransformer;
use Symfony\Component\Form\Extension\Core\DataTransformer\DataTransformerChain;
use Symfony\Component\Form\Extension\Core\DataTransformer\DateTimeToArrayTransformer;
use Symfony\Component\Form\Extension\Core\DataTransformer\DateTimeToHTML5DateTimeLocalTransformer;

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Sep 6, 2018

Member

case mismatch (see failing tests): should be Html5

This comment has been minimized.

Copy link
@mcfedr

mcfedr Sep 6, 2018

Author Contributor

Oops! Thanks.

@mcfedr mcfedr force-pushed the mcfedr:datetime-html5-fix branch from 67a95f4 to 8839532 Sep 6, 2018

@nicolas-grekas
Copy link
Member

left a comment

LGTM with just one minor comment

*/
class DateTimeToHtml5DateTimeLocalTransformer extends BaseDateTimeTransformer
{
const HTML5_FORMAT = "Y-m-d\TH:i:s";

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Sep 7, 2018

Member

should use single quotes

This comment has been minimized.

Copy link
@mcfedr

mcfedr Sep 7, 2018

Author Contributor

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 \\

@mcfedr mcfedr force-pushed the mcfedr:datetime-html5-fix branch from 8839532 to 253d0a6 Sep 7, 2018

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

This is a bit confusing:

@franzwilding

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

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

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Sep 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.

@franzwilding

This comment has been minimized.

Copy link
Contributor

commented Sep 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.

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

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

@mcfedr

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2018

I refered to the whatwg spec, which does not include Z - I think some of the confusion here is the difference between type=datetime and type=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 xabbuh referenced this pull request Sep 14, 2018

Closed

DateTimeType HTML5 Chrome #22063

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

This comment has been minimized.

Copy link
@xabbuh

xabbuh Sep 15, 2018

Member

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).

This comment has been minimized.

Copy link
@mcfedr

mcfedr Sep 15, 2018

Author Contributor

Makes sense, I've added something similar here.

@mcfedr mcfedr force-pushed the mcfedr:datetime-html5-fix branch from d6f0029 to e21a1a4 Sep 15, 2018

@xabbuh

xabbuh approved these changes Sep 15, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 17, 2018

Thank you @mcfedr.

@nicolas-grekas nicolas-grekas merged commit e21a1a4 into symfony:2.8 Sep 17, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Sep 17, 2018

bug #28372 [Form] Fix DateTimeType html5 input format (franzwilding, …
…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/A

Fix 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 reverseTransform

Commits
-------

e21a1a4 Added relevent links for parsing to the phpdoc
4f06f15 Add stricter checking for valid date time string
253d0a6 [Form] Fix DateTimeType html5 input format

@mcfedr mcfedr deleted the mcfedr:datetime-html5-fix branch Sep 18, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 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

nicolas-grekas added a commit that referenced this pull request Sep 18, 2018

minor #28504 [Form] Fix expected values in datetime-local test (mcfedr)
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/A

Commits
-------

5ed8b27 Fix expected values in datetime-local test

This was referenced Sep 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.