Skip to content

Conversation

@nyamsprod
Copy link
Contributor

This bug was encountered when adding DatePeriod::getRecurrences.

Currently when instantiating a DatePeriod object with the ISO Interval notation an exception is thrown if the recurrence count value is lesser than 1.

new DatePeriod('R0/2012-07-01T00:00:00Z/P7D'); //throws an exception

when using the alternate constructor no exception is thrown

new DatePeriod(
    new DateTime('2012-07-01T00:00:00Z'),
    new DateInterval('P7D'),
    0
);

This lead to DatePeriod::getRecurrences returning a value that could be 0 or negative which should not be possible as the recurrence count should be equal or greater than 1.

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.

@nikic nikic requested a review from derickr March 25, 2019 20:07
@smalyshev smalyshev added the Bug label Mar 29, 2019
@nikic
Copy link
Member

nikic commented Apr 10, 2019

ping @derickr

Copy link
Member

@derickr derickr left a 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.

Copy link
Member

@derickr derickr left a 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.

@nyamsprod
Copy link
Contributor Author

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

@nyamsprod nyamsprod changed the title bugfix DatePeriod::__construct with invalid recurrence count value Fixed bug #77909: DatePeriod::__construct() with invalid recurrence count value Apr 16, 2019
…ount value

Improve error message on invalid reccurence count

Adding test when reccurence is -1
@nyamsprod
Copy link
Contributor Author

nyamsprod commented Apr 16, 2019

@derickr about what you asked here's a summary of what I've done 👍

  • bug report
  • squashing
  • targeting PHP7.2

@derickr
Copy link
Member

derickr commented Apr 17, 2019

Excellent, I'll go and merge this now then.

@derickr
Copy link
Member

derickr commented Apr 17, 2019

All merged - thanks!

@derickr derickr closed this Apr 17, 2019
@nyamsprod nyamsprod deleted the bugfix/dateperiod-recurrences-validation-on-constructor branch April 17, 2019 16:51
@fyrye
Copy link

fyrye commented Sep 23, 2019

@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 DatePeriod::_construct() $recurrences argument was accepting <= 0 without a notice or exception since PHP 5.3

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);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants