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

Wrong all day yearly recurrence handling #234

Closed
arwinvdv opened this issue Aug 6, 2019 · 4 comments
Closed

Wrong all day yearly recurrence handling #234

arwinvdv opened this issue Aug 6, 2019 · 4 comments
Labels

Comments

@arwinvdv
Copy link

arwinvdv commented Aug 6, 2019

Description of the Issue:

All day events without time, has a time after parsing:

Steps to Reproduce:

  1.  $ical = new ICalParser($link, [
                 'defaultSpan' => 2,
                 'defaultTimeZone' => 'CET',
                 'defaultWeekStart' => 'MO',
                 'disableCharacterReplacement' => false,
                 'skipRecurrence' => false,
                 'useTimeZoneWithRRules' => false,
             ]);
  2.  BEGIN:VEVENT
     CREATED:20120713T094854Z
     DTEND;VALUE=DATE:20120816
     DTSTAMP:20140814T070716Z
     DTSTART;VALUE=DATE:20120815
     LAST-MODIFIED:20140814T070710Z
     RRULE:FREQ=YEARLY
     SEQUENCE:3
     SUMMARY: X birthday
     UID:19933C3A-938E-4CB8-B6C5-C9057DE89363
     END:VEVENT
  3. Output will be:
object(Event)#1057 (28) {
  ["summary"]=>
  string(24) "X birthday"
  ["dtstart"]=>
  string(15) "20190815T220000"
  ["dtend"]=>
  string(15) "20190816T220000"
  ["duration"]=>......

The dtstart and dtend has now a time: 22:00u. Is this expected behavior?
I have already a solution, if you want I will make a pull request.

@u01jmg3 u01jmg3 self-assigned this Aug 6, 2019
@machfr
Copy link

machfr commented Aug 6, 2019

I can vouch for this happening

@u01jmg3
Copy link
Owner

u01jmg3 commented Aug 6, 2019

Thanks for logging this issue.

Please bear with me. @s0600204 has kindly submitted PR #232 which I need to find time to review. This may fix your issue so feel free to test out the code.

If not, I am happy to accept a PR.

@machfr
Copy link

machfr commented Aug 6, 2019

I tried to implement the correction from #232 (I think I did it correctly)
But It seems it is not working, the test file I am trying to use is (I can make this public as it is just test data): https://calendar.google.com/calendar/ical/rtaotivcoe06p16ji6ilem6i4o%40group.calendar.google.com/public/basic.ics

I want to add that as far as I can see it shows correct on the start date BUT when it has a yearly occurrence it will on the same date put in a date that is set to 22:00
(try parsing the link I posted "FØDSESLDAG 2") and everyone after that date will be set to 22:00

And It might be the same thing that does so "FØDSELSDAG" which was set to run UNTIL 20-08-2018 is being shown even though we are in 2019 so it should not.

@s0600204
Copy link
Collaborator

s0600204 commented Aug 6, 2019

Looking into this, the erroneous times appear to only be added if the timezone of the php environment (which can be changed via date_default_timezone_set()) is a php-recognisable timezone other than "UTC".

At a guess, this might be something to do with the fact we're parsing the iCal Date string (without any timezone context) with new DateTime() (line 1285), then (as this is a YEARLY rrule with no BYDAY or BYMONTH stanzas) the output is fed through gmdate() (line 1931), back in through strtotime() (line 1935), then finally out through date() (line 1938) to get the final result.

DateTime(), strtotime(), and date() use the php environment's timezone by default when no timezone context is otherwise passed to it (which is what happens here); whilst gmdate() strictly uses GMT (aka UTC).

DateTime::construct -> date more or less cancels out each other's timezone offsetting. DateTime::construct -> gmdate -> strtotime -> date inserts an off-by-x error where x just so happens to be the offset of the php environment's timezone away from UTC.

(And the reason why the day is still correct is because gmdate is only used on line 1931 for outputting years, months, hours, minutes, and seconds. The day is deliberately kept the same.)


The aforementioned PR (#232) does not fix the issue. However, the PR happens to be a precursory cleanup prior to a rewrite of the processRecurrences method that does. (If you wish to try that, you can find it over on the recur_master branch of my fork in PR #236.)

@u01jmg3 u01jmg3 added this to the v2.x.x milestone Aug 18, 2019
@u01jmg3 u01jmg3 closed this as completed Aug 18, 2019
@u01jmg3 u01jmg3 removed their assignment Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants