search

tplaner / When

PHP Date Recursion library
https://github.com/tplaner/When
MIT License
513 stars 96 forks source link

Fixes issues #55 & #58 #59

Closed Blindmikey closed 8 years ago

Blindmikey commented 8 years ago

Fixes issue #55 Refactored existing BYSETPOS limiting logic found in Monthly execution in generateOccurrences(); expanded to cover Yearly as well as Weekly. Added appropriate tests as well.

Fixes issue #58 Updated getOccurrencesBetween() method to utilize generateOccurrences() method, and thereby inherit all the existing benefits such as BYSETPOS filtering that exist within generateOccurrences(). Memory footprint has not increased. There is a slight decrease in speed; of the 183 tests there was an increase of time spent from 945ms to 1.04s; in practice this should be negligible.

akf commented 8 years ago

Hi! I haven't had time to look through this as closely as I'd like to, but " Updated getOccurrencesBetween() method to utilize generateOccurrences()" got my attention. You've addressed my main concern, which is that generateOccurrences sets $until if it wasn't already specified, and getOccurrencesBetween is meant to support definitions without an end date.

I do have a couple of concerns about this. One lends itself to being attached to a line in your changes, so I'll add that separately. The other is that getOccurrencesBetween used to avoid modifying $this. There is a comment on line 449, just prior to the function definition, to this effect: 

// Get occurrences between two DateTimes, exclusive. Does not modify $this.

A minimal fix, of course, would be to change the comment! The reason that I made a point of leaving $this alone is that if we change parts of the occurrence definition, calls to $this->getOccurrencesBetween (or even $this->generateOccurrences) could get different results depending on what previous calls had been made.

I don't know how critical that is; note that I'm not a maintainer, just a contributor, and I contributed getOccurrencesBetween to solve my own problem. In doing so, I actually introduced this problem in a small way -- when generateOccurrences was the only way to get results out, your occurrences might have been truncated but at least they'd be consistent.

Blindmikey commented 8 years ago

I think I may understand what you mean by

The reason that I made a point of leaving $this alone is that if we change parts of the occurrence definition, calls to $this->getOccurrencesBetween (or even $this->generateOccurrences) could get different results depending on what previous calls had been made.

If a user would use generateOccurrences() after using getOccurrencesBetween() ( or even getOccurrencesBetween() a second time... ) I see that our effect on $this->limit $this->until $this->occurrences and $this->startDate could definitely alter the output.

If we should expect users to use this class as such, then it would be a good idea to somehow sustain those original properties.

akf commented 8 years ago

Yes, this is what I was getting at:

If a user would use generateOccurrences() after using getOccurrencesBetween() ( or even getOccurrencesBetween() a second time... ) I see that our effect on $this->limit $this->until $this->occurrences and $this->startDate could definitely alter the output.

After stepping away for a bit, it occurs to me that the cleanest and easiest way to sustain the original properties would be to clone the initial When object and modify the clone before calling generateOccurences on it (the clone), thus eliminating the need to modify the original object at all. Yesterday I was thinking in terms of saving the original properties and restoring them, which seemed headache-inducing.

I have no comments on BYSETPOS, by the way, because that is really giving me a headache! Thanks for working on that.

Blindmikey commented 8 years ago

@akf I've taken your advice to clone the current object to prevent altering the original object - this has worked quite well. I've also fixed the issue of a premature failure when valid occurrences were both inside and outside the range limit, by moving the check and only clearing occurrences from $thisClone->occurrences that were before the $startDate; where before $thisClone->occurrences was being erased completely.