Last occurence missing

[BrunoSpy]

Hi, First : thank you for this wonderful library !

When I create a new recurrence from a string pattern like the following one, the last occurence is missing. Example : FREQ=DAILY;INTERVAL=1;UNTIL=20160629

$recurrence = new Recurrence($startdate, $pattern);
//$startdate 24/06/2016 10:00 UTC for the example below
$rset = $recurrence->getRSet();
foreach($rset as $occurence) {
   print_r($occurrence);
}

That will give you :

[Thu Jun 23 11:17:43.218553 2016] [:error] [pid 6821] [client 127.0.0.1:50212] DateTime Object\n(\n    [date] => 2016-06-24 10:00:00.000000\n    [timezone_type] => 3\n    [timezone] => Etc/GMT\n)\n, referer: http://127.0.0.1/epeires2b/
[Thu Jun 23 11:17:43.219020 2016] [:error] [pid 6821] [client 127.0.0.1:50212] DateTime Object\n(\n    [date] => 2016-06-25 10:00:00.000000\n    [timezone_type] => 3\n    [timezone] => Etc/GMT\n)\n, referer: http://127.0.0.1/epeires2b/
[Thu Jun 23 11:17:43.219498 2016] [:error] [pid 6821] [client 127.0.0.1:50212] DateTime Object\n(\n    [date] => 2016-06-26 10:00:00.000000\n    [timezone_type] => 3\n    [timezone] => Etc/GMT\n)\n, referer: http://127.0.0.1/epeires2b/
[Thu Jun 23 11:17:43.220017 2016] [:error] [pid 6821] [client 127.0.0.1:50212] DateTime Object\n(\n    [date] => 2016-06-27 10:00:00.000000\n    [timezone_type] => 3\n    [timezone] => Etc/GMT\n)\n, referer: http://127.0.0.1/epeires2b/
[Thu Jun 23 11:17:43.220390 2016] [:error] [pid 6821] [client 127.0.0.1:50212] DateTime Object\n(\n    [date] => 2016-06-28 10:00:00.000000\n    [timezone_type] => 3\n    [timezone] => Etc/GMT\n)\n, referer: http://127.0.0.1/epeires2b/

[rlanvin]

Hi, First: you're welcome. :)

Now I'm sorry but your code doesn't look like it's part of this library. There is no Recurrence class. Do you have some custom code on top of the library that could cause this issue maybe?

[BrunoSpy]

Oups, i forgot the most important part... Yes the Recurrence part is part of my code : https://github.com/BrunoSpy/epeires2/blob/test/module/Application/src/Application/Entity/Recurrence.php But it does basically nothing else than wrapping your lib. Without the recurrence class, it would look like

$rule = 'DTSTART;TZID=Etc/GMT:' . $this->getStartdate()->format('Ymd\THis') . '
                 RRULE:FREQ=DAILY;INTERVAL=1;UNTIL=20160629';
$rrule = new RRule($rule);
$rset = new RSet();
$rset->addRule($rrule);
foreach....

[rlanvin]

Alright I see. The problem is that you don't define a time component for the UNTIL part. So by default, it's be 00:00:00. Therefore, your rule is "until 2016-06-29 00:00:00", and indeed the last occurrence "2016-06-29 10:00:00" is beyond this limit, so is not returned.

Be careful of another gotcha: you are explicitly using GMT timezone for DTSTART, but you are not explicitly using a timezone for UNTIL, therefore it'll take your default (PHP) timezone.

So you should write UNTIL=20160629T100000+0000.

EDIT: Actually this is an invalid syntax, sorry about that. The correct syntax is: UNTIL=20160629T100000Z

Let me know if this work!

Personnally I would suggest working with DateTime object and array directly, as it's more precise. If you have any suggestions as to how the library should handle this differently, feel free to share.

[BrunoSpy]

Thx for your help. If this is the expected behaviour in the RFC, I think we can close this issue and I will fix my code.

[rlanvin]

If this is the expected behaviour in the RFC

Well... I re-read the RFC just to be sure, and I have to say it's not strictly the expected behaviour. Here is the relevant part of the RFC:

The value of the UNTIL rule part MUST have the same value type as the "DTSTART" property. Furthermore, if the "DTSTART" property is specified as a date with local time, then the UNTIL rule part MUST also be specified as a date with local time. If the "DTSTART" property is specified as a date with UTC time or a date with local time and time zone reference, then the UNTIL rule part MUST be specified as a date with UTC time. In the case of the "STANDARD" and "DAYLIGHT" sub-components the UNTIL rule part MUST always be specified as a date with UTC time. If specified as a DATE-TIME value, then it MUST be specified in a UTC time format.

According to this, two importants things:

1. it's invalid to have rules with UNTIL part that has no time component when DTSTART has a time component. So to be strict the parser should reject them.
2. The UNTIL part must always be in UTC. As far as I understand it, there is no way to specify a timezone. In fact, the format I suggested you use (with the timezone) is explicitly forbidden if you want to follow the RFC strictly:

 The form of date and time with UTC offset MUST NOT be used.  For
 example, the following is not valid for a DATE-TIME value:

  19980119T230000-0800       ;Invalid time format

So, maybe the RFC string parser should be more strict?

[BrunoSpy]

3.3.10 [...] recur-rule-part = ( "FREQ" "=" freq ) ( "UNTIL" "=" enddate ) [...] enddate = date / date-time

The value of the UNTIL rule part MUST have the same value type as the "DTSTART" property.

So : UNTIL can be just a date and IF it's a date-time, it must be in a UTC format.

BUT as my DTSTART is a date-time, UNTIL must be a date-time also.

Conclusion : I will get rid of the time in my DTSTART to comply to the RFC but if I do that, will your parser be compliant ?

[BrunoSpy]

After some tests :

1.

DTSTART;TZID=Etc/GMT:20160624T100000 RRULE:FREQ=DAILY;INTERVAL=1;UNTIL=20160628T100000

Does not return 20160628

2.

DTSTART;TZID=Etc/GMT:20160624 RRULE:FREQ=DAILY;INTERVAL=1;UNTIL=20160628

Does not return 20160628

3.

DTSTART;TZID=Etc/GMT:20160624T100000 RRULE:FREQ=DAILY;INTERVAL=1;UNTIL=20160628T100000Z

Returns 20160628

[BrunoSpy]

So it seems your parser is right but could be stricter... Maybe just a warning in order to not introduce a backward compatibility break ?

[rlanvin]

Thank you for your tests!

Ok so as we discussed before, test 1 is technically invalid from a RFC point of view, as UNTIL must be in UTC (test 1 is missing the "Z" indicator at the end). The parser instead interprets the date with the default PHP timezone, which is not the expected behaviour of the RFC, I agree. I guess the expected behaviour would be to throw an exception - or indeed maybe just a warning. The other option is to call this a "feature" of the lib and document it. :-)

I believe test 2 should indeed return 20160628 according to the RFC, and you definitely identified a bug there. This is due to the parser converting the date into a DateTime at the default PHP's timezone. For for example, in my case the "20160628" becomes "2016-06-28 00:00:00 Europe/Helsinki" (which is my default timezone), which translates to "2016-06-27 22:00:00 GMT". I definitely need to look into this.

Test 3 is valid from a RFC point of view, and the lib works as expected - no problem there.

[rlanvin]

After pulling my hairs many hours on this problem, I realized that your test 2 is also not valid according to the RFC. You cannot use TZID with a date only... So I think the cause of all the problems is a parser that is not strict enough.

@BrunoSpy I pushed a commit in master to make the parser throw InvalidArgumentException for invalid DTSTART/UNTIL formats. Could you test and give me your feedback?

[BrunoSpy]

I'll give a try next week but I think you chose the right solution.

[BrunoSpy]

Just looked at your commits and tested it : that's fine for me !

[rlanvin]

Thanks!