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

ZonedDateTime::of not working correctly with seconds part offset #35

Closed
solodkiy opened this issue May 30, 2021 · 4 comments
Closed

ZonedDateTime::of not working correctly with seconds part offset #35

solodkiy opened this issue May 30, 2021 · 4 comments

Comments

@solodkiy
Copy link
Contributor

solodkiy commented May 30, 2021

Looks like \DateTimeZone with used in ZonedDateTime::of logic not working correctly with seconds in offset. It silently convert any offset with seconds to UTC timezone.

1) Brick\DateTime\Tests\ZonedDateTimeTest::testOf with data set #17 ('2016-05-17T12:34:56.123456789', '+00:15:01', '+00:15:01', 0, 1463487597, 123456789)
Failed asserting that 1463488496 is identical to 1463487597.

diff --git a/tests/ZonedDateTimeTest.php b/tests/ZonedDateTimeTest.php
index 151b3b4..2bb4edb 100644
--- a/tests/ZonedDateTimeTest.php
+++ b/tests/ZonedDateTimeTest.php
@@ -75,6 +75,7 @@ class ZonedDateTimeTest extends AbstractTestCase
             ['2014-03-15T12:34:56.123456789', '-00:15', '-00:15', 0, 1394887796, 123456789],
             ['2015-04-16T12:34:56.123456789', '+00:00', '+00:00', 0, 1429187696, 123456789],
             ['2016-05-17T12:34:56.123456789', '+00:15', '+00:15', 0, 1463487596, 123456789],
+            ['2016-05-17T12:34:56.123456789', '+00:15:01', '+00:15:01', 0, 1463487597, 123456789],
             ['2017-06-18T12:34:56.123456789', '+00:30', '+00:30', 0, 1497787496, 123456789],
             ['2018-07-19T12:34:56.123456789', '+00:45', '+00:45', 0, 1532000996, 123456789],
             ['2019-08-20T12:34:56.123456789', '+01:00', '+01:00', 0, 1566300896, 123456789],

1) Brick\DateTime\Tests\TimeZoneTest::testFromDateTimeZone with data set #2 ('+01:00:01')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'+01:00:01'
+'Z'

/home/doc/projects/_forks/date-time/tests/TimeZoneTest.php:81

FAILURES!
Tests: 16, Assertions: 23, Failures: 1.
[doc@doc-pc ~/projects/_forks/date-time (master)]# git diff tests/Time
TimeZoneOffsetTest.php  TimeZoneRegionTest.php  TimeZoneTest.php        
[doc@doc-pc ~/projects/_forks/date-time (master)]# git diff tests/TimeZoneTest.php
diff --git a/tests/TimeZoneTest.php b/tests/TimeZoneTest.php
index 4e4709f..60b0174 100644
--- a/tests/TimeZoneTest.php
+++ b/tests/TimeZoneTest.php
@@ -86,6 +86,7 @@ class TimeZoneTest extends AbstractTestCase
         return [
             ['Z'],
             ['+01:00'],
+            ['+01:00:01'],
             ['Europe/London'],
             ['America/Los_Angeles']
         ];

@solodkiy
Copy link
Contributor Author

Looks like we should or start throw exception on any seconds part start from TimeZoneOffset object, or rewrite ZonedDateTime::of logic to not use \DateTimeZone.
First solution looks more reasonable for me.

@BenMorel
Copy link
Member

BenMorel commented May 30, 2021

Good finding! I don't think there is a use case for a time-zone offset with seconds, so I agree that we should throw an exception.

Should we regexp it first, before giving it to DateTimeZone?

Just read the code again, and if I'm not mistaken, we should deny creating a TimeZoneOffset with seconds altogether:

  • in TimeZoneOffset factory methods
  • in IsoParsers::timeZoneOffset()

BTW, I think this should also be brought up on the internals mailing list, as it's a rather big annoyance.

@BenMorel
Copy link
Member

BenMorel commented Aug 13, 2021

This will be fixed in PHP 8.1: https://bugs.php.net/bug.php?id=81097

Do you think we should throw an exception in earlier PHP versions, when seconds is not zero?

@BenMorel
Copy link
Member

Fixed in 01c7e25. We now prevent seconds altogether in TimeZoneOffset, so that we're always compatible with DateTimeZone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants