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

Issue with rule when DTSTART is set and "Z" Time Zone Identifier is apart of the datetime string. #106

Closed
a-am opened this issue Nov 29, 2016 · 4 comments
Assignees

Comments

@a-am
Copy link
Contributor

a-am commented Nov 29, 2016

My RRule is: FREQ=WEEKLY;DTSTART=20161130T013000Z;DTEND=20161130T030000Z;BYDAY=TU;UNTIL=20170124T140000Z;WKST=MO

If I am in Mountain Time the start date is actually 11/29/2016 6:30 PM. So I would expect the first recurrence item start to be:

[*:start] => DateTime#2
        (
            [date] => '2016-11-30 01:30:00.000000'
            [timezone_type] => 3
            [timezone] => 'UTC'
        )

However this is what is returned:

[*:start] => DateTime#2
        (
            [date] => '2016-12-06 01:30:00.000000'
            [timezone_type] => 3
            [timezone] => 'UTC'
        )

The first issue is the occurance didn't include the start date as the first occurrence. Second I would expect that occurrence to be:

[*:start] => DateTime#2
        (
            [date] => '2016-12-07 01:30:00.000000'
            [timezone_type] => 3
            [timezone] => 'UTC'
        )

In my rule declaration I have tried setting the Timezone attribute to both Mountain and UTC with the same results. If I declare a start date it gets overridden by the DTSTART (I don't know if this is intentional). Is this a lack of logic to handle the DTSTART attribute with a Time Zone Identifier? According to the calendar spec having a Time Zone Identifier is accepted. It appears that even though the start date is in UTC it converts it again during recurrence generation.

@simshaun simshaun added the bug label Dec 1, 2016
@simshaun simshaun self-assigned this Dec 1, 2016
@simshaun
Copy link
Owner

simshaun commented Dec 1, 2016

The lib currently does not support parsing TZID from DTSTART.

Your problem stems from a bug in the rule loader where the instantiated DTSTART, DTEND, and UNTIL DateTimes aren't adjusted to your desired timezone when they contained the UTC designator (or any timezone identifier recognized by \DateTime's parser).

I'm debating the best way to handle this problem. A couple options floating around in my head:

  1. Remove support for DTSTART and DTEND in the RRULE string since they aren't actually part of a real RRULE string.

    • Rely on the $startDate and $endDate constructor arguments/setters instead.
    • Remove the $timezone constructor argument/setter.
    • Infer timezone from the $startDate.

    This is a big BC break, but I think it would help reduce confusion.

  2. Keep support for DTSTART and DTEND in the RRULE string and just fix the bug. Always adjust DTSTART, DTEND, and UNTIL to a specific timezone in the following priority:

    1. Constructor $timezone argument
    2. Timezone of $startDate, if $startDate provided
    3. Default PHP timezone

    I think that is the behavior you expected. Correct me if I'm wrong. Anyway, because this "bugfix" would change the generated recurrence set, I'd consider it a BC break and release it as a major version bump.

Given a PHP timezone of America/Denver, and using your example:

  1. 20161130T013000Z (2016-11-30 1:30 AM, UTC) is adjusted to 2016-11-29 6:30 PM, MT.
  2. Generated occurrences are:
    • 2016-11-29 6:30 PM

    • ...

    • 2017-01-17 6:30 PM

      Note that 2017-01-24 6:30 PM is not included. The original UNTIL 2017-01-24 2 PM, UTC is adjusted to 2017-01-24 7 AM, MT. The next occurrence, 2017-01-24 6:30 PM, MT, falls after the adjusted UNTIL, so it's not included in the set.


I'd like to remove timezone conversion from the library entirely, leaving it up to the user, but I think that's going to be a "2.0" thing. DATE-TIME values without a TZID or UTC designator are supposed to be floating dates (e.g. A person is unavailable from 11am - 3pm no matter what timezone he/she is in.) So, instead of doing any timezone conversion on DTSTART, a recurrence would include a \DateTime in the 1) explicitly declared TZID value in DTSTART, or 2) UTC with a boolean indicator for "floating time".

@a-am
Copy link
Contributor Author

a-am commented Dec 1, 2016

@simshaun thank you for the response. I suppose my expectation was that the RRule would be the ultimate determining factor in start date time and timezone according to the DTSTART. I ultimately was confused on how best to setup the rule constructor.

In your first option "Remove support for DTSTART and DTEND in the RRULE string since they aren't actually part of a real RRULE string" I am confused because to my understanding DTSTART and DTEND are apart of the rule iCalendar spec. (https://www.ietf.org/rfc/rfc2445.txt [page 30]) I may have misunderstood you.

Thinking through this I think the best option would be to rely on the RRule to specify the start date and timezone. This way the user can ultimately determine if they need it floating or precise and the RRule becomes the reference point, which I would think would be fitting if we would stick to the standard.

For the mean time I think option two would be the best bet to fix the bug.

@simshaun
Copy link
Owner

simshaun commented Dec 1, 2016

They're part of an iCal object, but not part an actual RRULE string. See the example at the bottom of page 53, https://tools.ietf.org/html/rfc2445#page-54.

I plan on handling this differently with #94. My goal is to accept an actual iCal object, extract the VEVENTS, and generate occurrences from there. DTSTART and DTEND in each VEVENT would be the source of truth, so there will be no $startDate or $endDate arguments like those that currently exist. For users that don't have or want to pass an iCal object, I'll provide a builder class that's similar to the current Rule class. It should be pretty straight-forward.

Anyway, I've got the bugfix for this ready and will release it tonight when I get home.

@a-am
Copy link
Contributor Author

a-am commented Dec 1, 2016

That sounds like a great way forward. Thank you for the quick bugfix!

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

No branches or pull requests

2 participants