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

Create periods with timezones in a couple places that are missing it. #13445

Merged
merged 4 commits into from Oct 3, 2018

Conversation

Projects
None yet
2 participants
@diosmosis
Member

diosmosis commented Sep 18, 2018

No description provided.

@diosmosis

This comment has been minimized.

Member

diosmosis commented Sep 18, 2018

@tsteur I'm wondering if my understanding of how matomo deals w/ timezones is correct. It's used sometimes when creating periods/dates, but not always.

EDIT: dealt with.

diosmosis added some commits Sep 18, 2018

@tsteur

This comment has been minimized.

Member

tsteur commented Oct 2, 2018

@diosmosis which issue fixes this here?

if (strpos($endDate, '-') === false) {

This comment has been minimized.

@tsteur

tsteur Oct 2, 2018

Member

why do we need to apply the timezone in one case but not the other?

I would have actually expected we wouldn't need to apply a timezone here at all... especially since it doesn't seem to be used for real time / raw data. Or is it applied in case there is today/yesterday? Actually I think I just answered the question :) a comment be good that the timezone might be only needed for today/yesterday basically.

For some reason I would not have expected the timezone eg in $last30Relative = new Range($period, $lastN, $timezone); Might be a bug.

This comment has been minimized.

@diosmosis

diosmosis Oct 2, 2018

Member

lastN is based on today, so we'd need the timezone to figure out what 'today' is for the site?

This comment has been minimized.

@tsteur

tsteur Oct 2, 2018

Member

I see... it is used when last parameter is not set 👍

@diosmosis

This comment has been minimized.

Member

diosmosis commented Oct 2, 2018

@tsteur fixes this issue: #13369 (comment)

@tsteur

This comment has been minimized.

Member

tsteur commented Oct 2, 2018

LGTM if tests pass

@diosmosis

This comment has been minimized.

Member

diosmosis commented Oct 2, 2018

thanks for adding the comment, 👍

@diosmosis diosmosis merged commit ae872e5 into 3.x-dev Oct 3, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details

@diosmosis diosmosis deleted the evolution-graph-timezone branch Oct 3, 2018

InfinityVoid added a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018

Create periods with timezones in a couple places that are missing it. (
…matomo-org#13445)

* Create periods with timezones in a couple places that are missing it.

* tweak

* Apply timezone only if endDate is not a normal date.

* add comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment