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

Audit Date class and re-structure API #13829

Open
4 tasks
diosmosis opened this issue Dec 11, 2018 · 8 comments
Open
4 tasks

Audit Date class and re-structure API #13829

diosmosis opened this issue Dec 11, 2018 · 8 comments
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself.

Comments

@diosmosis
Copy link
Member

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 in getTimestamp() it is treated as a time in another timezone.
  • getTimestamp() says it returns a UTC time, but it treats the stored time as non-UTC and converts it to UTC. getTimestampUTC() however just returns the timestamp. so 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:

  • timestamps should always be in UTC. timezones are just a view on that timestamp. so Date should not store a timezone and should not do any conversion in methods that return timestamps.
  • methods that take string representations or return string representations should accept timezones, eg: getDatetime($timezone = 'UTC'), toString($timezone = 'UTC'), Date::factory($value, $timezone = 'UTC')
  • since timestamps should always be UTC, Date::factory(...) should not accept timestamps (or should throw a if a timezone is specified w/ the timestamp). the semantics of Date::factory() should be, give me the posix timestamp of this date in this timezone. (ie, give me the UTC timestamp of 2012-03-04 03:05:23 UTC+5.
  • ...UTC() methods should be removed

Refs #13799
Refs #13786

@diosmosis diosmosis added the c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. label Dec 11, 2018
@diosmosis diosmosis added this to the 4.0.0 milestone Dec 11, 2018
diosmosis added a commit that referenced this issue Dec 14, 2018
…ther, to get proper result. (#13799)

* Update TimezonesTest to be more useful.

* Update TimezonesTest which will not pass until #13829

* Non-intrusively fix evolution graph timezone end date issue using new Date method.

* Make sure Date::factoryInTimezone only works w/ special word times.
@diosmosis
Copy link
Member Author

Should also probably make sure the mysql session uses UTC instead of any other configured timezone.

@mattab
Copy link
Member

mattab commented Jan 30, 2020

How much work would this be do you reckon, probably several days?
do you know if tackling this debt solve any existing known bug?

@diosmosis
Copy link
Member Author

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.

do you know if tackling this debt solve any existing known bug?

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.

@tsteur
Copy link
Member

tsteur commented Jan 31, 2020

@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

@diosmosis
Copy link
Member Author

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.

@mattab
Copy link
Member

mattab commented Feb 20, 2020

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.

@tsteur tsteur modified the milestones: 4.0.0, 5.0.0 Apr 13, 2020
@tsteur
Copy link
Member

tsteur commented Apr 13, 2020

Moving this into 5.0 unfortunately as we're running late on Matomo 4 Milestone.

@diosmosis
Copy link
Member Author

  • Another issue spotted by @tsteur: Date::factory() calls __toString() on the input if it is a Date instance, which removes the time of day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself.
Projects
None yet
Development

No branches or pull requests

3 participants