Problem/Motivation
DateTimePlus doesn't apply the correct time zone when converting a string with an offset.
Per the documentation for \DateTime::__construct():
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).
See also https://3v4l.org/S9r0q
Proposed resolution
This a documentation bug; update the docblocks to align with \DateTime::__construct()
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#15 | 2889814-15.patch | 2.02 KB | mpdonadio |
Comments
Comment #2
mpdonadioHere is a demo.
There was 1 failure:
1) Drupal\Tests\Component\Datetime\DateTimePlusTest::testDateTimezone with data set #4 ('2007-01-31T21:00:00-05:00', 'America/New_York', 'America/New_York', 'DateTimePlus uses the specifi...vided.')
DateTimePlus uses the specified timezone if provided.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'America/New_York'
+'-05:00'
Comment #3
mpdonadioBetter test-only version.
Comment #4
mpdonadioComment #7
mpdonadioI'll update the test-only patch to cover the timestamp case once I verify that it is also broken.
Comment #8
mpdonadioThis
- Updates the docs to match what the code does with timezones for the constructor, and to align it with what the test coverage is.
- Fixes the bug described in the IS
- Adds some test coverage
The static create methods also needs to be fixed...
Comment #10
jhedstromInteresting. This passes locally, but not on testbot... I wonder if we need to explicitly set the server timezone for the tests or something? Or perhaps its a php version issue (I have 7 locally)?
Comment #11
mpdonadioKTB should have a default timezone from the phpunit bootstrap, which I have a related issue to sanity check: #2889803: Add test coverage for default time zone in tests.
11 hours def suggests a 'Australia/Sydney' vs 'UTC' prob, as I am pretty sure that date is in DST then and therefore UTC+11.
Haven't tried that locally yet or dug into it much.
Comment #12
mpdonadioThis fails for me locally in all PHP versions.
The problem is the values all have offsets in them. Old behavior was to ignore the system one; patch changes this.
So, the question is whether we
1) move forward with the patch, and adjust this test
2) consider this a documentation bug, and just update docblocks
Kinda leaning towards (2) right now. It aligns better with how \DateTime works, and I also fear that this may cause BC problems for anyone who is relying on the behavior.
?
Comment #13
mpdonadioComment #14
jhedstromI think updating the documentation to how things currently behave and keeping things inline with
\DateTime
makes sense.Comment #15
mpdonadioDocblock fix. Text is lifted nearly verbatim from the note on http://php.net/manual/en/datetime.construct.php
I already rolled and attached the patch, but wondering if the @see should be moved from @param mixed $timezone to the top level of the docblock?
Comment #16
jhedstromAre the
@
symbols intentional here?Comment #17
mpdonadio#16, yes. If you want to parse a unix timestamp directly from the $time string, the format is with a leading @, eg '@1500000000'.
Comment #18
jhedstromThis looks good to me.
Comment #20
mpdonadioRandom b/c OutsideIn.
Comment #22
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!