[Form] DateTimeToLocalizedStringTransformer does not use timezone when using date only #21218

magnetik · 2017-01-09T17:35:27Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#21217
License	MIT

The DateTimeToLocalizedStringTransformer when used with a pattern that have only date (in DateType for instance) does not convert the result in the desired output

Reproduction steps:

Use DateType in a form
Set the model_timezone to Pacific/Tahiti, having default timezone to Europe/Berlin.
Enter the date 2017-01-10
Submit the form
Notice that the received data is 2017-01-10 00:00 Europe/Berlin, whereas it should be 2017-01-10 11:00 Europe/Berlin

HeahDude

Beside my comment 👍

HeahDude · 2017-01-09T23:28:56Z

...mfony/Component/Form/Extension/Core/DataTransformer/DateTimeToLocalizedStringTransformer.php

@@ -130,7 +130,10 @@ public function reverseTransform($value)
        try {
            if ($dateOnly) {
                // we only care about year-month-date, which has been delivered as a timestamp pointing to UTC midnight
-                return new \DateTime(gmdate('Y-m-d', $timestamp), new \DateTimeZone($this->inputTimezone));
+                $dateTime = new \DateTime(gmdate('Y-m-d', $timestamp), new \DateTimeZone($this->outputTimezone));
+                $dateTime->setTimezone(new \DateTimeZone($this->inputTimezone));


This line should be in the same if as below https://github.com/symfony/symfony/pull/21218/files#diff-a13339ab3c3fcaba6009f82a2b77ae01R149.

in fact this looks correct here, see L146-149, isn't it?

Yes these are the lines I'm talking about, but I don't get it what do you mean?

I'm not saying this is not correct here, but it may not be needed to set again the timezone, hence the check is missing.

understood! Then what about this patch?

--- a/src/Symfony/Component/Form/Extension/Core/DataTransformer/DateTimeToLocalizedStringTransformer.php +++ b/src/Symfony/Component/Form/Extension/Core/DataTransformer/DateTimeToLocalizedStringTransformer.php @@ -129,11 +129,11 @@ class DateTimeToLocalizedStringTransformer extends BaseDateTimeTransformer try { if ($dateOnly) { // we only care about year-month-date, which has been delivered as a timestamp pointing to UTC midnight - return new \DateTime(gmdate('Y-m-d', $timestamp), new \DateTimeZone($this->inputTimezone)); + $dateTime = new \DateTime(gmdate('Y-m-d', $timestamp), new \DateTimeZone($this->outputTimezone)); + } else { + // read timestamp into DateTime object - the formatter delivers a timestamp + $dateTime = new \DateTime('@'.$timestamp); } - - // read timestamp into DateTime object - the formatter delivers a timestamp - $dateTime = new \DateTime(sprintf('@%s', $timestamp)); // set timezone separately, as it would be ignored if set via the constructor, // see http://php.net/manual/en/datetime.construct.php $dateTime->setTimezone(new \DateTimeZone($this->outputTimezone));

Looks good!

magnetik · 2017-01-10T13:43:56Z

Thanks for the comments. I'll continue when the discussion about the related bug (#21217) is ended.
Some input are welcome there.

nicolas-grekas · 2017-01-12T12:58:29Z

@magnetik ready to update the PR?

magnetik · 2017-01-12T13:04:29Z

I'm willing to update it with the proposed change, but there are some ingoing discussion in #21217 about whether or not this PR is the good solution.
Could we have your feedback there?

nicolas-grekas · 2017-01-12T13:08:16Z

The rational is purely local. The code logic is currently broken, the patch #21218 (comment) fixes the situation by handling TZ as in other places.
So no need for more considerations on my side, let's move on :)

magnetik · 2017-01-12T13:25:12Z

Done.

It slightly differ from your diff as I've let the ->setTimezone in the else.

HeahDude · 2017-01-12T13:33:24Z

@magnetik the setter must be called after the else so it's called in both cases.

magnetik · 2017-01-12T13:36:31Z

Well in the $dateOnly case the DateTime is directly created with the outputTimezone in the construct.
Is there any point of calling setTimezone with the same timezone there?

HeahDude · 2017-01-12T13:55:42Z

Yeah this is what the comment below is talking about ;)

magnetik · 2017-01-12T14:02:48Z

...mfony/Component/Form/Extension/Core/DataTransformer/DateTimeToLocalizedStringTransformer.php

+                $dateTime = new \DateTime(sprintf('@%s', $timestamp));
+                // set timezone separately, as it would be ignored if set via the constructor,
+                // see http://php.net/manual/en/datetime.construct.php
+                $dateTime->setTimezone(new \DateTimeZone($this->outputTimezone));


@nicolas-grekas @HeahDude I'm sorry to bother you with such trivial change but I really don't understand why this line should be moved outside the else.
The line 139 and the comment line 137 and 138 only applies to the construct of line 136 because it's created with the '@' with a timestamp. From the doc:

The $timezone parameter and the current timezone are ignored when the $time parameter either is a UNIX timestamp (e.g. @946684800) or specifies a timezone (e.g. 2010-01-28T15:00:00+02:00).

Line 133 the timezone is already applied when constructed.

If this something about limiting the number of instruction in the else?

passing a TZ as constructor does not "set" it.
To set it, you need to call "setTimeZone".
passing a TZ as constructor does only tell the constructor that if the first arg contains no TZ info, then this should be the one to use while parsing.
That's why

I see why I'm confused: since we pass (gmdate('Y-m-d', $timestamp) as first argument in the construct line 133 I'm assuming that there will never be a timezone there and you don't. (am I right?)

I'll change accordingly.

tkleinhakisa · 2017-01-12T14:23:27Z

The rational is purely local.
@nicolas-grekas what do you mean ?

As i see the current fix you could still select 2017-01-10 and get back 2017-01-09, which seems very weird

nicolas-grekas · 2017-01-12T14:25:42Z

2017-01-10 and get back 2017-01-09

because of TZ config yes - not more weird than TZ themselves - just logical in fact.

tkleinhakisa · 2017-01-12T14:34:02Z

Not logical at all to me ... because there should not be any TZ config at all when working only with dates

…g only date

nicolas-grekas · 2017-01-12T14:48:55Z

because there should not be any TZ config at all when working only with dates

then that's a config issue. But when there is one, we must respect it.

tkleinhakisa · 2017-01-12T15:41:49Z

But when there is one, we must respect it.

When there is a wrong one shouldn't we fix it ?

nicolas-grekas · 2017-01-12T16:50:15Z

@tkleinhakisa config is config, there is no way to tell if a TZ setting is wrong or false

nicolas-grekas · 2017-01-12T16:56:50Z

...mfony/Component/Form/Extension/Core/DataTransformer/DateTimeToLocalizedStringTransformer.php

@@ -130,11 +130,11 @@ public function reverseTransform($value)
        try {
            if ($dateOnly) {
                // we only care about year-month-date, which has been delivered as a timestamp pointing to UTC midnight
-                return new \DateTime(gmdate('Y-m-d', $timestamp), new \DateTimeZone($this->inputTimezone));
+                $dateTime = new \DateTime(gmdate('Y-m-d', $timestamp), new \DateTimeZone($this->outputTimezone));


wait, @tkleinhakisa is right! this should be inputTimezone instead of outputTimezone!
when dateOnly is true, the DateTime should not depend on the current hour!

that's not my point ! for me it shoud be $dateTime = new \DateTime(gmdate('Y-m-d', $timestamp)) so that the date i write/select in my form is the same as the one i get back in the code

tkleinhakisa · 2017-01-12T17:05:11Z

I'm sorry i don't want to be boring, but it's not about telling if the given TZ is right or wrong (there is no such thing) but it's about removing this setting (or at least deprecate it)

TZ do not make sense when working with dates only, so why do we have these settings ?

nicolas-grekas · 2017-01-12T17:07:56Z

👍 again, I messed up with the previous comment :)
@tkleinhakisa maybe, but since we have this setting, we have to honor it.

stof · 2017-01-12T17:53:17Z

the issue is that removing the timezone setting entirely was already attempted in the past, and it broke things for projects, because PHP still has a time in the object (because there is no Date object but a DateTime one).
so we cannot simply remove it again (it would cause the same issues)

tkleinhakisa · 2017-01-12T18:03:57Z

From what i've read on this bug tracker, TZ options do not make sense on that field, see #12404 #12380 #12239 #7187

the changes suggested here has the potential to break someone's app in a very isidious way because it won't be a fatal error break that can be easily spotted, it will be a change in the behavior of the app.

@stof From what i've seen in the previous PR's the options were removed without any deprecation first, which should be the way to go so that people using it and relying on it have time to update their app

it broke things for projects

This PR has the same potential

stof · 2017-01-12T18:11:31Z

@tkleinhakisa a deprecation would not have helped much, as the issue was that stopping to take the timezone conversion into account for dates caused issues later when using the DateTime object in ways converting timezones.
The root issue is that DateTime should not be used to represent a Date alone. It does not represent this. All issues were coming from that. But PHP does not have a Date object.

tkleinhakisa · 2017-01-12T18:20:20Z

Well then maybe the deprecation should not only concern the options but the return type too. If we agree that DateTime is not a good object to represent a date only or time only then shouldn't we move onto something else (a string, a simple Date object or Time Object ?)

nicolas-grekas · 2017-01-12T18:24:43Z

Note that I don't see the issue you're talking about : if we have 12hours delta, "the day before" or "the day after" will always be accurate up to 12h. I like to think locally. And locally here, TZ handling is accurate. Changing for something else won't make things more accurate.
What matters is that forward+reverse transforms is the identity function, which is what this fix ensures.

stof · 2017-01-12T18:26:04Z

@tkleinhakisa but we first need to have a standard for that, so that other libraries agree on using it (for instance the Doctrine ORM). If the ORM still use DateTime, it would just move the issue to the place where you convert between your Date object and DateTime, which does not solve anything

fabpot · 2017-01-12T18:42:23Z

Thank you @magnetik.

…imezone when using date only (magnetik) This PR was merged into the 2.7 branch. Discussion ---------- [Form] DateTimeToLocalizedStringTransformer does not use timezone when using date only | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21217 | License | MIT The `DateTimeToLocalizedStringTransformer` when used with a pattern that have only date (in `DateType` for instance) does not convert the result in the desired output Reproduction steps: - Use `DateType` in a form - Set the `model_timezone` to `Pacific/Tahiti`, having default timezone to `Europe/Berlin`. - Enter the date `2017-01-10` - Submit the form - Notice that the received data is `2017-01-10 00:00 Europe/Berlin`, whereas it should be `2017-01-10 11:00 Europe/Berlin` Commits ------- 031d8c2 [Form] DateTimeToLocalizedStringTransformer does not use TZ when using only date

tkleinhakisa · 2017-01-12T18:52:10Z

@stof i agree it's only pushing the pb away, it becomes the responsability of the developper to make the right conversion, for exemple using explicitly the transformer. This is not good at all for DX and third party lib integration. The issue could be raised at "higher" levels (php itself, fig, other ?)

Having working intensively with a mix of date (only) and date-time i know very well how painfull it is, and how difficult it would be for someone to find spot a bug introduced by this change.

As this PR is now merged, it seems that's the way to go and i don't think my app will suffer from it at all, then so be it. In the mean time is there anything that could be done to improve the situation ? dispite the fact that the implementation cannot be changed, there seems to be a consensus that these options do not really make sense, and that we're lacking some date-only and time-only objects

carsonbot added Status: Needs Review Bug labels Jan 9, 2017

magnetik changed the base branch from master to 2.7 January 9, 2017 17:36

magnetik changed the title ~~Fix 21217~~ [Form] DateTimeToLocalizedStringTransformer does not use timezone when using date only Jan 9, 2017

HeahDude suggested changes Jan 9, 2017

View reviewed changes

nicolas-grekas added this to the 2.7 milestone Jan 10, 2017

magnetik force-pushed the fix-21217 branch from f29ef79 to 36a4e69 Compare January 12, 2017 13:24

magnetik force-pushed the fix-21217 branch from 36a4e69 to 1c98cb0 Compare January 12, 2017 13:26

magnetik commented Jan 12, 2017

View reviewed changes

[Form] DateTimeToLocalizedStringTransformer does not use TZ when usin…

031d8c2

…g only date

magnetik force-pushed the fix-21217 branch from 1c98cb0 to 031d8c2 Compare January 12, 2017 14:38

nicolas-grekas reviewed Jan 12, 2017

View reviewed changes

fabpot merged commit 031d8c2 into symfony:2.7 Jan 12, 2017

This was referenced Jan 12, 2017

Release v2.7.23 #21258

Merged

Release v2.8.16 #21259

Merged

Release v3.1.9 #21260

Merged

Release v3.2.2 #21261

Merged

sstok mentioned this pull request Jan 13, 2017

DateTimeToLocalizedStringTransformer does not use timezone when using date only rollerworks/search#135

Closed

HeahDude mentioned this pull request Jan 25, 2017

DateTimeToLocalizedStringTransformer ignoring time during reverse transform between timezones #20806

Closed

[Form] DateTimeToLocalizedStringTransformer does not use timezone when using date only #21218

[Form] DateTimeToLocalizedStringTransformer does not use timezone when using date only #21218

Conversation

magnetik commented Jan 9, 2017

HeahDude left a comment

Choose a reason for hiding this comment

HeahDude Jan 9, 2017

Choose a reason for hiding this comment

nicolas-grekas Jan 10, 2017

Choose a reason for hiding this comment

HeahDude Jan 10, 2017

Choose a reason for hiding this comment

nicolas-grekas Jan 10, 2017

Choose a reason for hiding this comment

HeahDude Jan 10, 2017

Choose a reason for hiding this comment

magnetik commented Jan 10, 2017 • edited Loading

nicolas-grekas commented Jan 12, 2017

magnetik commented Jan 12, 2017

nicolas-grekas commented Jan 12, 2017 • edited Loading

magnetik commented Jan 12, 2017

HeahDude commented Jan 12, 2017

magnetik commented Jan 12, 2017

HeahDude commented Jan 12, 2017

magnetik Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

magnetik Jan 12, 2017

Choose a reason for hiding this comment

tkleinhakisa commented Jan 12, 2017

nicolas-grekas commented Jan 12, 2017

tkleinhakisa commented Jan 12, 2017

nicolas-grekas commented Jan 12, 2017

tkleinhakisa commented Jan 12, 2017

nicolas-grekas commented Jan 12, 2017

nicolas-grekas Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

tkleinhakisa Jan 12, 2017

Choose a reason for hiding this comment

tkleinhakisa commented Jan 12, 2017

nicolas-grekas commented Jan 12, 2017

stof commented Jan 12, 2017

tkleinhakisa commented Jan 12, 2017 • edited Loading

stof commented Jan 12, 2017

tkleinhakisa commented Jan 12, 2017

nicolas-grekas commented Jan 12, 2017 • edited Loading

stof commented Jan 12, 2017

fabpot commented Jan 12, 2017

tkleinhakisa commented Jan 12, 2017

magnetik commented Jan 10, 2017 •

edited

Loading

nicolas-grekas commented Jan 12, 2017 •

edited

Loading

magnetik Jan 12, 2017 •

edited

Loading

nicolas-grekas Jan 12, 2017 •

edited

Loading

nicolas-grekas Jan 12, 2017 •

edited

Loading

tkleinhakisa commented Jan 12, 2017 •

edited

Loading

nicolas-grekas commented Jan 12, 2017 •

edited

Loading