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() issue when year is less than 1892 #44

Closed
dakur opened this issue Mar 1, 2022 · 5 comments · Fixed by #60
Closed

ZonedDateTime::of() issue when year is less than 1892 #44

dakur opened this issue Mar 1, 2022 · 5 comments · Fixed by #60

Comments

@dakur
Copy link

dakur commented Mar 1, 2022

Following code works well (no output):

ZonedDateTime::of(
	LocalDateTime::of(2022,1,1),
	TimeZoneRegion::parse('Europe/Prague')
);

while this one throws Brick\DateTime\DateTimeException: The time zone offset of 3464 seconds is not a multiple of 60:

ZonedDateTime::of(
	LocalDateTime::of(0,1,1),
	TimeZoneRegion::parse('Europe/Prague')
);

The only difference is the year passed into LocalDateTime::of(). If I try LocalDateTime::min() instead, it results in the exception as well.

I found out that the border where it stops working is 1891–1892 (which 130 years back from now). Is this some issue with DateTime and/or its getOffset() which is used inside of the ZonedDateTime::of()?

But it doesn't fail with UTC time zone actually..

@jiripudil
Copy link
Contributor

Hi, for dates before time zones were invented, the timezonedb tends to observe the local mean time of the place and simply provides the UTC offset of the town's meridian. Consider the database entry for Europe/Prague:

# Zone  NAME            STDOFF  RULES   FORMAT  [UNTIL]
Zone    Europe/Prague   0:57:44 -       LMT     1850
                        0:57:44 -       PMT     1891 Oct    # Prague Mean Time
                        1:00    C-Eur   CE%sT   1945 May  9
                        ...

You can see that until October 1891, the UTC offset was 0:57:44. However, sub-minute offsets have been prohibited in 01c7e25. Perhaps @BenMorel can chime in with the reasoning behind the change?

@BenMorel
Copy link
Member

BenMorel commented Jun 8, 2022

@jiripudil Please check #35 for the rationale behind this. Could you please investigate the issue and tell me what you think the best move would be here? I'd happy to revert the commit, but PHP seems to always refuse seconds in offset now: https://3v4l.org/FG6nZ

(see the PHP bug in the linked issue)

Does that mean that we can't rely on, or have to hack around, DateTimeZone now?

@jiripudil
Copy link
Contributor

Thanks for the context! It seems that this issue has been remediated in the fresh out of the oven PHP 8.1.7 by an update of the underlying timelib (php/php-src#8589).

Unfortunately, PHP's data structures do not support sub-minute offsets, so getName() truncates the seconds – this will be an issue in TimeZone::fromDateTimeZone, but I think it could be hacked around using getOffset() which appears to be computed correctly with seconds: https://3v4l.org/fFpgU

I don't know what your stance is on supported PHP versions, but even I would be hesitant to require a version that has just been tagged a few days ago. That being said, I see no reason why developers should be deprived of the feature if they're running a version of PHP that supports it. Perhaps we could only allow sub-minute offsets for PHP_VERSION_ID >= 80107? And add a hint about it to the exception message in previous versions?

@BenMorel
Copy link
Member

That sounds good, do you want to open a PR?

@jiripudil
Copy link
Contributor

Sure :)

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

Successfully merging a pull request may close this issue.

3 participants