-
Notifications
You must be signed in to change notification settings - Fork 8k
Fixed bug #77909: DatePeriod::__construct() with invalid recurrence count value #3987
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
Fixed bug #77909: DatePeriod::__construct() with invalid recurrence count value #3987
Conversation
|
ping @derickr |
derickr
left a comment
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.
Code and test looks fine, but I'd like to see a test for a negative number too.
derickr
left a comment
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.
Changes look good now. Can you please squash your commits, and title the commit:
Fixed bug #<bug number>: DatePeriod::__construct() with invalid recurrence count value
The PR should probably target PHP 7.2 as well.
If there is no bug report yet, we should make one.
If that's all too much, let me know and I'll sort that.
|
I can create the bug report and squash the commits. Targetting PHP7.2 that I don't know how to do it but if I do the first two things it will be easier for you to target 7.2 I believe |
…ount value Improve error message on invalid reccurence count Adding test when reccurence is -1
|
@derickr about what you asked here's a summary of what I've done 👍
|
|
Excellent, I'll go and merge this now then. |
|
All merged - thanks! |
|
@derickr Curious as to why this did not initially start as a E_DEPRECATED notice for PHP 7.2 and 7.3, then target throwing an Exception in 7.4+ Since the Seems like a rather significant change to the API for a patch release, that forces developers to change their code to produce the same results. https://3v4l.org/Zlqp0 From function main(int $recurrences): \DatePeriod
{
return new \DatePeriod(
new \DateTime('2012-07-01T00:00:00Z'),
new \DateInterval('P7D'),
$recurrences
);
}To function main(int $recurrences): \DatePeriod
{
$interval = new \DateInterval('P7D');
$start = new \DateTime('2012-07-01T00:00:00Z');
if ($recurrences < 1) {
$recurrences = clone $start;
$recurrences->add($interval);
}
return new \DatePeriod($start, $interval, $recurrences);
} |
This bug was encountered when adding DatePeriod::getRecurrences.
Currently when instantiating a
DatePeriodobject with the ISO Interval notation an exception is thrown if the recurrence count value is lesser than1.when using the alternate constructor no exception is thrown
This lead to
DatePeriod::getRecurrencesreturning a value that could be0or negative which should not be possible as the recurrence count should be equal or greater than1.The submitted bugfix will make the second example throw like the first example instead of returning an object in invalid state. For any code relying on the current behavior this means a BC break as the current behavior silently returns a object that may be iterable at most once if the recurrence count value is equal to
0.