Skip to content
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

Make use of until parameter in nextMonthly function #532

Merged

Conversation

jaircuevajunior
Copy link
Contributor

Hello there,

I've felt the need of this PR becauase of a buggy event:
FREQ=MONTHLY;
INTERVAL=1;
BYMONTHDAY=2;
BYDAY=2WE;
WKST=WE;
UNTIL=20211214T000000Z

The "Until" was not being taken into account, so it was falling into the 10000 year problem.

@staabm
Copy link
Member

staabm commented Feb 10, 2021

Thanks for the PR.

Could you add unit test which fails without your fix?

@jaircuevajunior
Copy link
Contributor Author

The problem is that this is a performance issue...
If you get this rule, you'll realize it's an impossible rule, because of the BYMONTHDAY=2;.
When the object receives an impossible rule, it will loop through all the possibilities until it reaches this breakpoint:

            // To prevent running this forever (better: until we hit the max date of DateTimeImmutable) we simply
            // stop at 9999-12-31. Looks like the year 10000 problem is not solved in php ....
            if ($this->currentDate->getTimestamp() > 253402300799) {
                $this->currentDate = null;

                return;
            }

But it could stop the loop as soon as it arrives the until parameter as proposed in my PR:

            // For some reason the "until" parameter was not being used here,
            // that's why the workaround of the 10000 year bug was needed at all
            // let's stop it before the "until" parameter date
            if ($this->until && $this->currentDate->getTimestamp() >= $this->until->getTimestamp()) {
                return;
            }

In both cases the result is gonna be the same, but the process will be much faster with my purposed fix.

NOTE: Obviously, this is only working when the until parameter is set, otherwise it'll run untill the 10000 year as normally expected.

Now i have runned the tests, found an error and fixed it, but I don't know the right way of creating a test for this case since it's a performance issue... I don't want to create a test using the time as a parameter to fail the test... any idea?

@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #532 (1bbae65) into master (3168acd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #532   +/-   ##
=========================================
  Coverage     98.69%   98.69%           
- Complexity     1757     1759    +2     
=========================================
  Files            66       66           
  Lines          4277     4279    +2     
=========================================
+ Hits           4221     4223    +2     
  Misses           56       56           
Impacted Files Coverage Δ Complexity Δ
lib/Recur/RRuleIterator.php 99.41% <100.00%> (+<0.01%) 155.00 <0.00> (+2.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3168acd...1bbae65. Read the comment docs.

@phil-davis phil-davis self-requested a review February 12, 2021 00:00
Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

LGTM

@phil-davis phil-davis merged commit 38bc2d2 into sabre-io:master Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants