All time calculations should be done in UTC, rather than just tacitly taking PHP's default timezone.

CommentFileSizeAuthor
#4 use-datetime-2912347-3.patch5.93 KBerik.erskine

Comments

joachim created an issue. See original summary.

erik.erskine’s picture

erik.erskine commented

I'm not sure this will work - doing any kind of add/subtract on a date is dependent on timezone.

One example is with intervals that span a change in daylight savings time. For example, "2017-10-28 12:00 Europe/London" + "1 day" should result in "2017-10-29 12:00 Europe/London". In this instance the interval of 1 day is actually 23 hours because of the change in DST. But we only know this by having the timezone available along with the start date.

I think this can be avoided by making the plugin timezone agnostic, by changing RecurringPeriodInterface::calculateDate to operate on \DateTime objects rather than timestamps. We can then always using the add(\DateInterval $interval) function.

joachim’s picture

joachim commented

> For example, "2017-10-28 12:00 Europe/London" + "1 day" should result in "2017-10-29 12:00 Europe/London"

I've been racking my brains trying to think of a case where timezone would affect the result, as I had a feeling it would, but couldn't think of one! :) Thanks!

> by changing RecurringPeriodInterface::calculateDate to operate on \DateTime objects rather than timestamps. We can then always using the add(\DateInterval $interval) function.

That's handing over responsibility to the caller, which has the same decision to make...

In the case of something like a license, users expect the license to work in their own timezone, set by their personal settings... which this module can't know about, so yes, I guess changing to DateTime objects works, and the calling module has to decide what to do.

This responsibility will need documenting.

erik.erskine’s picture

erik.erskine commented
Title: do all time calculations in UTC for consistency » Use \DateTimeImmutable rather than timestamp for all time calculations
StatusFileSize
new use-datetime-2912347-3.patch5.93 KB

Patch included. This uses \DateTimeImmutable rather than \DateTime to avoid plugins modifying the passed-in values.

joachim’s picture

joachim commented
Status: Active » Fixed

Committed with some documentation and code formatting tweaks.

Thanks!

  • joachim committed fb2ea91 on 8.x-1.x authored by erik.erskine 
    Issue #2912347 by erik.erskine: Changed to use \DateTimeImmutable rather...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.