simshaun commented 8 years ago

Currently, the Rule class handles properties such as DTSTART, DTEND, and RDATE. Rule was originally intended to reflect the RFCs RRULE property, but it ended up encompassing a bit more than that. Properties like DTSTART, DTEND, and RDATE don't really belong there.

My plan is to alter Recurr to parse iCalendar objects for VEVENT definitions and generate occurrences for them based on the event properties like DTSTART, DTEND, RRULE, RDATE, etc.

It would no longer be possible to generate occurrences from the contrived RRULE strings that Recurr v1 supports. However, if desired, I could create an iCal factory class that accepts the v1-style strings (those that contain DTSTART, DTEND, RDATE, etc.).

Tasks

[ ] Build an iCalendar object parser specifically to extract VEVENTs.
[ ] Build a class that generates an occurrence collection from a VEVENT definition.
[ ] Optionally, build an iCal factory class that accepts the v1-style rule strings.
Goal

The goal is to end up with code like the following (not set in stone):

$iCal = new Recurr\ICal($iCalString);
$generator = new Recurr\OccurrenceGenerator(/* $generatorConfig */);
foreach ($ical->getEventComponents() as $event) {
    $occurrences = $generator->generate($event);
}

brianmuse commented 8 years ago

Couple thoughts on this from using this library for a bit. For lack of better place this seems like a good spot, as this issue relates to a large refactor.

First, I'm all about returning a generator during expansion instead of a fully populated array. Because expansion can be expensive, this allows us to exit iteration whenever we want. Secondly it allows us to lazily iterate over multiple generators to compose a list of instances together from multiple rules.

Secondly, the constraint stuff I'd just axe. It's a little cumbersome to use and I think it's more preferable for this to be managed by the caller, especially if they're able to iterate of these as generators, they can just break when needed. At the very most, you just need a $seed (start date) and an upper bound. However doing constraints in the way the library currently exists they just serve to create result sets that wouldn't actually be accurate for the iCal rules.

That takes me to my final suggestion... Currently the lib is all about the Rule class, which also defined RDATE and EXDATE as well as DTSTART and DTEND. This is somewhat of a mis-representation of the model defined in the RFC.

You're on the right track with you suggestion above, though it's a slippery slope where this library will slowly be expected to be a full RFC5545 compliant wrapper.

How we've implemented this in our project is (some influence from iCal4j): Property abstract class, extended by RRule etc, which can define attributes according to the spec. PropertyCollection a collection of generic ICal properties

// Below is _super_ rough and probably doesnt full work, but the idea is to 
// compose a collection of rules and dates together into a result, instead of expand of off a single
// rule only.
RecurrenceExpander::expand(
  DateTimeImmutable $seed, // the seed date to expand from, often DTSTART, but allow any seed here, in case we're using cache at our call site and want to start expanding from the middle.
  PropertyCollection $recurrenceProperties,
  ExpansionConfig $config // config value-object holding things like `$timezone`.
) {
    // get all RRULE, EXRULE (for legacy RFC 2445 support), RDATE, EXDATE from recurrence properties

    // An array to hold all the generators for dates that we'll include.
    $r_generators = [];

    // An array to hold all the generators for dates that we'll exclude.
    $ex_generators = [];

    // Add a generator for the RDATE collection
    $r_generators[] = $this->generateFromDates($rdates, $timezone);

    // Add a generator for each RRULE
    foreach ($rrules as $rrule) {
        $r_generators[] = $this->generateFromRule(
            $rule
            $config->getTimezone()
        );
    }

    // Add a generator for the EXDATE collection
    $ex_generators[] = $this->generateFromDates($exdates, $timezone);

    // Add a generator for each EXRULE
    foreach ($exrules as $exrule) {
        $ex_generators[] = $this->generateFromRule(
            $rule
            $config->getTimezone()
        );
    }

  do {
        // Holds the next date provided by each `$r_generators`
        $r_dates = [];

        /** @type Generator $generator */
        foreach ($r_generators as $key => $generator) {
            if (!$generator->valid()) {
                // This generator has reached it's end... Unset if from our array and continue.
                unset($r_generators[$key]);

                continue;
            }

            $r_dates[] = $generator->current();
        }

        // Get the lowest of all the `$r_dates` This is the "next" instance date for the series, so long as
        // it's not also found in with the EXDATES or the EXRULEs.
        $min_r_date = DateTimeFunctions\min(...$r_dates);

        $date_is_excluded = false;

        foreach ($ex_generators as $key => $generator) {
            if (!$generator->valid()) {
                // This generator has reached it's end... Unset if from our array and continue.
                unset($ex_generators[$key]);

                continue;
            }

            // Iterate through all exdates until we reach the `$min_r_date`
            while (-1 === DateTimeFunctions\compare($generator->current(), $min_r_date)) {
                $generator->next();
            }

            // If the current EXDATE matches the current `$min_r_date`, then the date should be excluded from the
            // series.
            if (0 === DateTimeFunctions\compare($generator->current(), $min_r_date)) {
                $date_is_excluded = true;
            }
        }

        if (!$date_is_excluded) {
            $iterations++;
            yield $min_r_date;
        }

        foreach ($r_generators as $generator) {
            // Advance all the `$r_generators` that yielded the previous result.
            if (0 === DateTimeFunctions\compare($generator->current(), $min_r_date)) {
                $generator->next();
            }
        }
        // Keep looping so long as the `$r_generators` are yielding dates and we're under the iteration limit.
    } while (!empty($r_generators) && $iterations < $iteration_limit);
};

With the above you can use all your existing code in for the Rule generator...just yield instead of build that datetime array which is returned at the end.

Hope this makes sense, but yeah it would be nice to be able to compose the various parts of iCal recurrence together in various ways to build more flexible models and use the library in more ways.

simshaun commented 8 years ago

Thank you for your input.

First, I'm all about returning a generator during expansion instead of a fully populated array.

I agree.

Secondly, the constraint stuff I'd just axe. It's a little cumbersome to use and I think it's more preferable for this to be managed by the caller

I'll think about this. To enforce a "minimum date" constraint, I already have to start from the very beginning and generate a bunch of unwanted occurrences until the first desired occurrence is found. In this scenario, it would be just as easy for the caller to implement that logic, so the library wouldn't need to offer it.

However, if I can figure out an efficient way for the library to skip generating unwanted occurrences, so that it starts generating only from the first desired occurrence, then I'll at least have a minimum date option in the generator. I'm not very optimistic that I'll be able to figure this out though. At first glance, it seems there are just too many scenarios to account for, so it's far, far easier to just generate unwanted occurrences and toss them out.

You're on the right track with you suggestion above, though it's a slippery slope where this library will slowly be expected to be a full RFC5545 compliant wrapper.

I'm not concerned. If somebody requests functionality that's out of scope of this library, I'll most likely refuse.

simshaun / recurr

Require an iCalendar object(string) to generate occurrences. [OPEN FOR DISCUSSION, MAJOR BC BREAK] #94

Tasks

Goal