Skip to content

Fixed bug #77243 (Weekdays are calculated incorrectly for negative years) #8740

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

Merged
merged 2 commits into from
Jun 17, 2022

Conversation

derickr
Copy link
Member

@derickr derickr commented Jun 9, 2022

Ideally this should be done by reimporting timelib, but I haven't made a release of that yet, so we'll do it with a "hot fix".

@derickr derickr requested a review from Girgias June 9, 2022 15:58
@derickr derickr self-assigned this Jun 9, 2022
@cmb69
Copy link
Member

cmb69 commented Jun 9, 2022

Do you have a reference on the weekdays of the dates in the tests? Respective tests with ext/calendar (since there is no year 0 there, I decreased the years by one):

var_dump(
    jddayofweek(gregoriantojd(1, 1, -1501), CAL_DOW_SHORT),
    jddayofweek(gregoriantojd(1, 1, -2147483649), CAL_DOW_SHORT),
    jddayofweek(gregoriantojd(1, 27, -292277022658), CAL_DOW_SHORT),
);

gives:

string(3) "Fri"
string(3) "Tue"
string(3) "Mon"

@derickr
Copy link
Member Author

derickr commented Jun 10, 2022

@cmb69, you were right about jddayofweek(gregoriantojd(1, 1, -1501), CAL_DOW_SHORT), having to return Friday. There was indeed another bug in my code with multiples of 100 (negative).

The other two can't be checked with ext/calendar, as its algorithm states:

 * VALID RANGE
 *
 *     4714 B.C. to at least 10000 A.D.

@derickr
Copy link
Member Author

derickr commented Jun 10, 2022

And FWIW, the results now match with what https://www.fourmilab.ch/documents/calendar/ does.

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

I think we're good to go. :)

@derickr derickr merged commit fe97a5a into php:PHP-8.0 Jun 17, 2022
@derickr derickr deleted the bug77342 branch June 17, 2022 08:50
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.

None yet

2 participants