Fix PHP Bug #78950: day-of-year (z) doesn't handle leap years#80
Fix PHP Bug #78950: day-of-year (z) doesn't handle leap years#80IMSoP wants to merge 1 commit intoderickr:masterfrom
Conversation
This patch changes behaviour in two ways: * First, it delays applying day-of-year until the end of parsing, so that it is normalised with respect to a year specified later. * Second, it makes mixing day-of-year with month or day-of-month an error, rather than silently changing behaviour in that case.
derickr
left a comment
derickr
left a comment
There was a problem hiding this comment.
Before I will consider this for merging, the following changes will have to be made:
- There needs to be an issue in this repository, with title "Day-of-year specifier (z) in parse from format does not handle leap years". (Don't refer to PHP tickets in the head line message, they can be in the description)
- The commit messages needs to be in the form "Fixed #XX: Day-of-year specifier (z) in parse from format does not handle leap years"
- A test needs to be added to
tests/c/parse_date_from_format.cpp, that covers the changes, for both the happy and sad paths.
| case TIMELIB_FORMAT_DAY_OF_YEAR: /* day of year - resets month (0 based) - also initializes everything else to !TIMELIB_UNSET */ | ||
| TIMELIB_CHECK_NUMBER; | ||
| if ((tmp = timelib_get_nr((char **) &ptr, 3)) == TIMELIB_UNSET) { | ||
| if ((day_of_year = timelib_get_nr((char **) &ptr, 3)) == TIMELIB_UNSET) { |
There was a problem hiding this comment.
Please separate out the assignment and the equivalence test into two lines.
| } | ||
|
|
||
| /* Handle day of year */ | ||
| if ( day_of_year != TIMELIB_UNSET ) { |
There was a problem hiding this comment.
No extra spaces inside ( and ), please.
| if (s->time->m != TIMELIB_UNSET || s->time->d != TIMELIB_UNSET) { | ||
| add_pbf_error(s, TIMELIB_ERR_MIX_DAY_OF_YEAR_WITH_M_D, "Mixing of day-of-year with month or day is not allowed", string, ptr); | ||
| } | ||
| else { |
There was a problem hiding this comment.
} else { goes on one line. I would also prefer that the part in the if contains the success case, and the part in the else the failure case.
|
https://bugs.php.net/bug.php?id=79580 has been closed in the meantime; is this PR still relevant? |
|
It is no longer relevant, the fix has been introduced in PHP 8.1 already, where we started throwing a |
3 participants
Original report: https://bugs.php.net/bug.php?id=79580
This patch changes behaviour in two ways:
so that it is normalised with respect to a year specified later.
an error, rather than silently changing behaviour in that case.
I'm not sure how to add tests for this here, but the existing tests all pass, and a branch of PHP with passing PHPT tests is here: php/php-src@PHP-7.3...IMSoP:bug-79580