-
Notifications
You must be signed in to change notification settings - Fork 8k
adding DatePeriod::getRecurrences method see bug #75113 #3888
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
adding DatePeriod::getRecurrences method see bug #75113 #3888
Conversation
ext/date/php_date.c
Outdated
|
|
||
| dpobj = Z_PHPPERIOD_P(ZEND_THIS); | ||
|
|
||
| if (!dpobj->recurrences) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derickr seems dpobj->recurrences by default equals 1 instead of NULL maybe we should substract 1 to the value and return if the substracted value equals 0 🤔
ext/date/php_date.c
Outdated
|
|
||
| dpobj = Z_PHPPERIOD_P(ZEND_THIS); | ||
|
|
||
| if (0 == dpobj->recurrences - 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's incorrect. By default, the start date is also included as a recurrence.
If you set the DatePeriod::EXCLUDE_START_DATE option, then this does not happen.
So instead of using - 1, you should be using - dpobj->include_start_date.
It's probably wise to add a test case for this too.
| string(12) "+0-0-1-0-0-0" | ||
| NULL | ||
| int(4) | ||
| NULL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure the last line has a new line too.
|
Committed and merged after rebasing this to PHP-7.2 |
|
@derickr Why putting a new method into |
|
|
||
| $periodWithRecurrencesWithoutStart = new DatePeriod($start, $interval, $recurrences, DatePeriod::EXCLUDE_START_DATE); | ||
|
|
||
| var_dump($periodWithRecurrences->getRecurrences()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo? I assume this should be $periodWithRecurrencesWithoutStart instead.
@derickr here's the PR to add
DatePeriod::getRecurrencesI hope I've follow your guidance correctly If something is missing please let me know.