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 #61642: modify("+5 weekdays") returns Sunday #122

Closed
wants to merge 1 commit into from
Closed

Conversation

0
Copy link
Contributor

@0 0 commented Jul 8, 2012

@smalyshev
Copy link
Contributor

After applying this patch, today on July 14 I get:

./cliphp -r '$d = new datetime(); var_dump($d->modify("+5 weekdays"));'
object(DateTime)#1 (3) {
["date"]=>
string(19) "2012-07-23 00:00:00"
["timezone_type"]=>
int(3)
["timezone"]=>
string(19) "America/Los_Angeles"
}

and this:

./cliphp -r '$d = new datetime(); var_dump($d->modify("+6 weekdays"));'
object(DateTime)#1 (3) {
["date"]=>
string(19) "2012-07-23 00:00:00"
["timezone_type"]=>
int(3)
["timezone"]=>
string(19) "America/Los_Angeles"
}

Looks like something is wrong - +5 weekdays and +6 weekdays should not return the same result.

@smalyshev
Copy link
Contributor

I'd recommend also adding more numbers - maybe from -10 to +10 - to the test, just to be sure.

Adding a non-zero multiple of 5 weekdays to any Friday, Saturday, or
Sunday would result in a Sunday instead of the correct date. This patch
provides an implementation of do_adjust_special_weekday() which does
not suffer from this issue.
@0
Copy link
Contributor Author

0 commented Jul 29, 2012

I had based my implementation on the bug report, and made it work exactly as the "Expected" section for +5 weekdays. I also used the existing +0/+1 semantics for weekends, which caused e.g. Saturday +0 weekdays and Saturday +1 weekday to both result in the following Monday. I've given it some more thought and I realized you're right that adding 5 and 6 weekdays to a Saturday shouldn't result in the same day; the jump from Thursday (+4) directly to Monday (+5) doesn't make any sense either. Similar considerations apply to negative offsets.

I've rewritten the implementation to make more sense (though it no longer exactly matches the bug report), and increased the range for the test. Let me know if any part of it still needs improvement.

@php-pulls
Copy link

Comment on behalf of stas at php.net:

merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants