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
Audit Date class and re-structure API #13829
Comments
Should also probably make sure the mysql session uses UTC instead of any other configured timezone. |
How much work would this be do you reckon, probably several days? |
I don't know really, possibly longer, maybe less. It would just be refactoring Date and getting all tests to pass. Then manual tests to make sure nothing strange happens. This part could take a while.
There were some bugs that this caused that were worked around by some special methods. There may be many bugs we don't know about. @tsteur would have a better opinion on the value of this issue. |
@mattab @diosmosis I'm not too much of a help here... It will be eventually needed to be refactored I suppose since some things eg around timezones are not always clear how it works. At the same time it would take long to refactor and not sure it's worth the effort. Also it will likely introduce new regressions since the current implementation is sometimes unclear and therefore hard to ensure the behaviour will be the same afterwards (this should not be an excuse though not to refactor it). I really have no preference here |
Actually I think it is fixing a bug, see: 8f4538e It's been a while since I wrote this issue, but I think timezone handling is generally inaccurate in matomo. |
Proposal: let's spend 2-3 days max on this issue and see if it's enough to complete most important parts of the refactoring. |
Moving this into 5.0 unfortunately as we're running late on Matomo 4 Milestone. |
|
The logic in the
Date
class is strange and somewhat backwards. Multiple methods have documentation that contradict the actual methods, and sometimes the actual logic just doesn't make sense.For example:
Date::factory('today', $timezone)
takes a timezone, but does not return 'today in $timezone`, it returns 'the time in $timezone that is 'today' in UTC'. then it discards the timezone and treats the result as UTC.Date::$timestamp
is supposed to store the time in UTC, but ingetTimestamp()
it is treated as a time in another timezone.getTimestamp() != getTimestampUTC()
even though both say they return UTC time.The API should be audited, trimmed and made coherent, but only for Matomo 4 since so much depends on this code.
Things that should be done:
getDatetime($timezone = 'UTC')
,toString($timezone = 'UTC')
,Date::factory($value, $timezone = 'UTC')
Date::factory(...)
should not accept timestamps (or should throw a if a timezone is specified w/ the timestamp). the semantics ofDate::factory()
should be, give me the posix timestamp of this date in this timezone. (ie, give me the UTC timestamp of2012-03-04 03:05:23 UTC+5
.Refs #13799
Refs #13786
The text was updated successfully, but these errors were encountered: