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

Trying to improve the accuracy of the isToday method #20136

Merged
merged 1 commit into from Jan 11, 2023

Conversation

snake14
Copy link
Contributor

@snake14 snake14 commented Dec 23, 2022

Description:

Thomas noticed that the Date->isToday() method was producing inconsistent results when dealing with multiple time zones. This is an effort to improve that accuracy.

Review

@snake14 snake14 added the Needs Review PRs that need a code review label Dec 23, 2022
@github-actions
Copy link
Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Dec 30, 2022
Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran tests successfully and breakpoint tested a timezone conversion, works well 👍

@bx80 bx80 merged commit f82d099 into 4.x-dev Jan 11, 2023
@bx80 bx80 deleted the trying-to-fix-date-istoday-method branch January 11, 2023 22:38
@tsteur
Copy link
Member

tsteur commented Jan 11, 2023

@snake14 @bx80 I believe this change actually caused a regression the other day in funnels plugin? Maybe we need to revert it?

@snake14
Copy link
Contributor Author

snake14 commented Jan 11, 2023

@snake14 @bx80 I believe this change actually caused a regression the other day in funnels plugin? Maybe we need to revert it?

Yeah. This would probably have a similar issue with sites that use UTF-5 type time zones. I can see if I can come up with a workaround.

@justinvelluppillai justinvelluppillai added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jan 12, 2023
@snake14
Copy link
Contributor Author

snake14 commented Jan 12, 2023

@tsteur @bx80 I just created a new PR with a proposed fix so that this PR won't need to be reverted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Stale The label used by the Close Stale Issues action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants