[Serializer] DateTimeNormalizer: allow to provide timezone #22444

ogizanagi · Apr 15, 2017

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

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

maidmaid · Apr 16, 2017

src/Symfony/Component/Serializer/Normalizer/DateTimeNormalizer.php

+        $timezone = $this->getTimezone($context);
+
+        if (null !== $timezone) {
+            $object = (new \DateTime('@'.$object->getTimestamp()))->setTimezone($timezone);


new \DateTime('@'.$object->getTimestamp(), $timezone) instead?

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-30 23:00:00.000000", +"timezone_type": 1, +"timezone": "+00:00", } >>> new \DateTime('@1480546800', new \DateTimeZone('Japan')) => DateTime {#173 +"date": "2016-11-30 23:00:00.000000", +"timezone_type": 1, +"timezone": "+00:00", } >>> (new \DateTime('@1480546800'))->setTimezone(new \DateTimeZone('Japan')) => DateTime {#174 +"date": "2016-12-01 08:00:00.000000", +"timezone_type": 3, +"timezone": "Japan", } >>>

I suggest to use DateTimeImmutable instead of DateTime.

maidmaid · Apr 16, 2017

src/Symfony/Component/Serializer/Normalizer/DateTimeNormalizer.php


        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


As this issue is related to an unsupported version PHP, I think it's not usefull to handle this problem here.

We can remove this workaround in Symfony 4.0, but for now Symfony 3 still supports PHP 5.5.
See failing tests on Travis: https://travis-ci.org/symfony/symfony/jobs/222333390#L2437-L2471

In this case, maybe is it better to check PHP version like in this example?

Updated. Thank you.

dunglas · Apr 18, 2017

src/Symfony/Component/Serializer/Normalizer/DateTimeNormalizer.php


    /**
     * @var string
     */
    private $format;

    /**
-     * @param string $format
+     * @var \DateTimeZone


This docblock is useless (same for the existing docblock for $format btw).

Oh. I misread. I thought you were talking about the private DateTimeNormalizer::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?

dunglas · Apr 18, 2017

src/Symfony/Component/Serializer/Normalizer/DateTimeNormalizer.php

+        $timezone = $this->getTimezone($context);
+
+        if (null !== $timezone) {
+            $object = (new \DateTime('@'.$object->getTimestamp()))->setTimezone($timezone);


I suggest to use DateTimeImmutable instead of DateTime.

dunglas · Apr 18, 2017

src/Symfony/Component/Serializer/Normalizer/DateTimeNormalizer.php

+     */
+    private function getTimezone(array $context)
+    {
+        $dateTimeZone = array_key_exists(self::TIMEZONE_KEY, $context) ? $context[self::TIMEZONE_KEY] : $this->timezone;


You can use isset instead of array_key_exists here (faster as it's not a function call).

The idea behind this was to allow setting null as timezone, in case one was provided in the constructor.

dunglas

👍

ogizanagi · Jun 14, 2017

Ready to go? :)

fabpot · Jun 14, 2017

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

GuilhemN · Jun 17, 2017

It seems this breaks sf 3.4 tests (see https://travis-ci.org/symfony/symfony/jobs/243836779). Are you sure the issue you talked about is fixed in php 5.6?

ogizanagi · Jun 17, 2017

Hmm, indeed, it seems I misinterpreted php/php-src#1167 (comment). Considering https://3v4l.org/NmFK5 and the list of tags containing php/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  | Bug fix? | yes | New feature? | no  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | #22444 (comment)  | License | MIT | Doc PR | N/A Commits ------- 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  | Bug fix? | no | New feature? | no  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes (failure unrelated) | Fixed tickets | #22444  | License | MIT | Doc PR | N/A Commits ------- c73dae3 [Serializer] Remove DateTimeNormalizer PHP < 7 bc layer

maidmaid reviewed Apr 16, 2017

View reviewed changes

dunglas requested changes Apr 18, 2017

View reviewed changes

dunglas approved these changes Apr 19, 2017

View reviewed changes

nicolas-grekas added this to the 3.4 milestone Apr 20, 2017

xabbuh approved these changes May 12, 2017

View reviewed changes

ogizanagi changed the base branch from master to 3.4 May 17, 2017 18:31

[Serializer] DateTimeNormalizer: allow to provide timezone

c10a780

fabpot approved these changes Jun 14, 2017

View reviewed changes

fabpot merged commit c10a780 into symfony:3.4 Jun 14, 2017

ogizanagi deleted the feature/serializer/date_timezone branch June 14, 2017 20:04

ogizanagi mentioned this pull request Jun 17, 2017

[Serializer] Fix workaround min php version #23217

Merged

ogizanagi mentioned this pull request Jul 3, 2017

[Serializer] Remove DateTimeNormalizer PHP < 7 bc layer #23364

Merged

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

ro0NL mentioned this pull request Apr 26, 2019

[Serializer] Provide option to always apply timezone of the DateTimeNormalizer context. #31256

Closed

Search code, repositories, users, issues, pull requests...

Uh oh!

[Serializer] DateTimeNormalizer: allow to provide timezone #22444

[Serializer] DateTimeNormalizer: allow to provide timezone #22444

Uh oh!

Conversation

ogizanagi commented Apr 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi Apr 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi Apr 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas left a comment

Choose a reason for hiding this comment

Uh oh!

ogizanagi commented Jun 14, 2017

Uh oh!

fabpot commented Jun 14, 2017

Uh oh!

GuilhemN commented Jun 17, 2017

Uh oh!

ogizanagi commented Jun 17, 2017

Uh oh!

Uh oh!

ogizanagi commented Apr 15, 2017 •

edited

Loading

ogizanagi Apr 16, 2017 •

edited

Loading

ogizanagi Apr 16, 2017 •

edited

Loading

ogizanagi Apr 18, 2017 •

edited

Loading