Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Serializer] DateTimeNormalizer: allow to provide timezone#22444
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $timezone = $this->getTimezone($context); | ||
| if (null !== $timezone) { | ||
| $object = (new \DateTime('@'.$object->getTimestamp()))->setTimezone($timezone); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
new \DateTime('@'.$object->getTimestamp(), $timezone) instead?
ogizanagiApr 16, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The thing is precisely not to use the second argument here actually, because as specified on the manual entry:
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).
—http://php.net/manual/en/datetime.construct.php#refsect1-datetime.construct-parameters
on the contrary of\DateTime::setTimezone() which will properly set the timezone and "update" the date accordingly.
See the following:
>>>new \DateTime('@1480546800')=> DateTime {#166 +"date":"2016-11-3023:00:00.000000", +"timezone_type":1, +"timezone":"+00:00", }>>>new \DateTime('@1480546800',new \DateTimeZone('Japan'))=> DateTime {#173 +"date":"2016-11-3023:00:00.000000", +"timezone_type":1, +"timezone":"+00:00", }>>> (new \DateTime('@1480546800'))->setTimezone(new \DateTimeZone('Japan'))=> DateTime {#174 +"date":"2016-12-0108:00:00.000000", +"timezone_type":3, +"timezone":"Japan", }>>>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I suggest to useDateTimeImmutable instead ofDateTime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
| if (null !== $dateTimeFormat) { | ||
| $object = \DateTime::class === $class ? \DateTime::createFromFormat($dateTimeFormat, $data) : \DateTimeImmutable::createFromFormat($dateTimeFormat, $data); | ||
| if (null === $timezone) { | ||
| // https://bugs.php.net/bug.php?id=68669 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Asthis issue is related to an unsupported version PHP, I think it's not usefull to handle this problem here.
ogizanagiApr 16, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We can remove this workaround in Symfony 4.0, but for now Symfony 3still supports PHP 5.5.
See failing tests on Travis:https://travis-ci.org/symfony/symfony/jobs/222333390#L2437-L2471
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In this case, maybe is it better to check PHP version like inthis example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Updated. Thank you.
| /** | ||
| * @var string | ||
| */ | ||
| private $format; | ||
| /** | ||
| * @param string $format | ||
| * @var \DateTimeZone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This docblock is useless (same for the existing docblock for$format btw).
ogizanagiApr 18, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh. I misread. I thought you were talking about the privateDateTimeNormalizer::getTimezone() method.
While I agree, I added it to be consistent with the existing docblock. If we want to remove it, shouldn't we make a dedicated PR on the lowest branch rather than here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Indeed.
| $timezone = $this->getTimezone($context); | ||
| if (null !== $timezone) { | ||
| $object = (new \DateTime('@'.$object->getTimestamp()))->setTimezone($timezone); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I suggest to useDateTimeImmutable instead ofDateTime.
| */ | ||
| private function getTimezone(array $context) | ||
| { | ||
| $dateTimeZone = array_key_exists(self::TIMEZONE_KEY, $context) ? $context[self::TIMEZONE_KEY] : $this->timezone; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can useisset instead ofarray_key_exists here (faster as it's not a function call).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The idea behind this was to allow settingnull as timezone, in case one was provided in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok got it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
Ready to go? :) |
Thank you@ogizanagi. |
…zone (ogizanagi)This PR was merged into the 3.4 branch.Discussion----------[Serializer] DateTimeNormalizer: allow to provide timezone| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | N/A| License | MIT| Doc PR | N/AMy own use-case was for denormalization of a csv file provided by a third-party. The datetime format inside does not contain any timezone information, and won't change, but it's established to be UTC (or at least consistent).So by providing the new `datetime_timezone` option, the returned instance of `\DateTime(Interface)` will properly be set with the expected timezone. (In case the format already supports the time offset, the provided timezone is ignored in favor of the one parsed by the `\DateTime` object)Regarding normalization, the expected behavior of this feature is to consistently return the same time offset.Commits-------c10a780 [Serializer] DateTimeNormalizer: allow to provide timezone
It seems this breaks sf 3.4 tests (seehttps://travis-ci.org/symfony/symfony/jobs/243836779). Are you sure the issue you talked about is fixed in php 5.6? |
Hmm, indeed, it seems I misinterpretedphp/php-src#1167 (comment). Consideringhttps://3v4l.org/NmFK5 and the list of tags containingphp/php-src@365f428, it seems it's only available since PHP 7... Thanks for the report. See#23217. |
This PR was merged into the 3.4 branch.Discussion----------[Serializer] Fix workaround min php version| Q | A| ------------- | ---| Branch? | 3.4 <!-- see comment below -->| Bug fix? | yes| New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets |#22444 (comment) <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/ACommits-------74db2e6 [Serializer] Fix workaround min php version
…(ogizanagi)This PR was merged into the 4.0-dev branch.Discussion----------[Serializer] Remove DateTimeNormalizer PHP < 7 bc layer| Q | A| ------------- | ---| Branch? | master <!-- see comment below -->| Bug fix? | no| New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes (failure unrelated)| Fixed tickets |#22444 <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/ACommits-------c73dae3 [Serializer] Remove DateTimeNormalizer PHP < 7 bc layer
Uh oh!
There was an error while loading.Please reload this page.
My own use-case was for denormalization of a csv file provided by a third-party. The datetime format inside does not contain any timezone information, and won't change, but it's established to be UTC (or at least consistent).
So by providing the new
datetime_timezoneoption, the returned instance of\DateTime(Interface)will properly be set with the expected timezone. (In case the format already supports the time offset, the provided timezone is ignored in favor of the one parsed by the\DateTimeobject)Regarding normalization, the expected behavior of this feature is to consistently return the same time offset.