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

Fix get_beginning_of_week #380

Merged
merged 2 commits into from
Jun 14, 2018
Merged

Fix get_beginning_of_week #380

merged 2 commits into from
Jun 14, 2018

Conversation

FewKinG
Copy link

@FewKinG FewKinG commented Oct 18, 2016

The method get_beginning_of_week has some flaws which I addressed in this commit.

  1. The comments make no sense (mixing up end of week and beginning of week)
  2. When the passed $date is not a start of the week and the passed number $week is greater than 1, the method will behave incorrectly, because the difference between $day_of_week and $start_of_week is then subtracted multiple times. In the current calendar implementation this seems to have no negative effect, because the passed $date will always be a start of a week. But I thought it would be proper to fix it anyway.
  3. When, by means of the given offset of $week, a DST switch introducing additional time occurs during the period between $date and the final date, the method will also behave incorrectly (the same date(s) may occur multiple times in the calendar causing misalignment of weekdays and possible confusion)

fix get_beginning_of_week returning wrong results when given $date is not a start of the week
fix get_beginning_of_week returning wrong results on DST switching
@FewKinG
Copy link
Author

FewKinG commented Oct 18, 2016

Concerning the DST problem, I replaced the calculation of the week offset, which was done by adding a fixed amount of seconds, with the use of strtotime like this:
$date = strtotime ( '+' . ( $week - 1 ) . ' week', $date ) ;

I'm not all too familiar with PHP but I hope this will be fine. At least this seems to respect DST changes.

@FewKinG
Copy link
Author

FewKinG commented Oct 21, 2016

I added another commit, also fixing the same error in the corresponding method get_ending_of_week.

In this case the error resulted in the database query selecting a too short week range for weeks after the DST change whereby all sundays after the change appeared empty in the calendar.

@cojennin
Copy link
Contributor

cojennin commented Oct 21, 2016

When, by means of the given offset of $week, a DST switch introducing additional time occurs during the period between $date and the final date, the method will also behave incorrectly (the same date(s) may occur multiple times in the calendar causing misalignment of weekdays and possible confusion)

jon-stewart-mind-blown

Crazy find! Given how gnarly this method is (and how critical it is to making sure the calendar renders correctly) can you add a test-edit-flow-calendar.php file with a few tests that make sure it executes correctly in normal situations as well as funky ones (i.e., DST change in between start and end of week). Also, would hopefully test a few international date situations as well (if you know of any interesting one's to test for)!

Will review after that test is in and then work on merging. Thanks!

@rinatkhaziev
Copy link
Contributor

This looks good to me, however, I'm hesitant to include it in 0.8.3 without tests. I'm tentatively marking this for 0.8.3. @sboisvert care you chime in?

@FewKinG will you be open to adding tests? I realize that it's been way too long since the PR, and priorities most likely have shifted for you :)

@rinatkhaziev rinatkhaziev added this to the 0.8.3 milestone Apr 22, 2018
@rinatkhaziev rinatkhaziev requested a review from sboisvert April 22, 2018 19:29
@rinatkhaziev rinatkhaziev merged commit 31c2903 into Automattic:master Jun 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants