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

Support the DTSTART of an event also being an EXDATE #246

Merged
merged 2 commits into from
Dec 3, 2019

Conversation

s0600204
Copy link
Collaborator

Potential fix for #240.

@u01jmg3
Copy link
Owner

u01jmg3 commented Nov 19, 2019

Thanks for this - don't seem to get the tests to pass:

1) RecurrencesTest::testStartDateIsExdateUsingUntil
Exception: DateTime::__construct(): Failed to parse time string (20191023T095000EXDATE;TZID=Europe/London:20191009T095000EXDATE;TZID=Europe/London:20190925T095000EXDATE;TZID=Europe/London:20190911T095000) at position 15 (E): The timezone could not be found in the database

C:\ics-parser\vendor\nesbot\carbon\src\Carbon\Carbon.php:555
C:\ics-parser\src\ICal\ICal.php:2169
C:\ics-parser\src\ICal\ICal.php:1278
C:\ics-parser\src\ICal\ICal.php:709
C:\ics-parser\src\ICal\ICal.php:541
C:\ics-parser\tests\RecurrencesTest.php:270
C:\ics-parser\tests\RecurrencesTest.php:208

2) RecurrencesTest::testStartDateIsExdateUsingCount
Exception: DateTime::__construct(): Failed to parse time string (20191023T095000EXDATE;TZID=Europe/London:20191009T095000EXDATE;TZID=Europe/London:20190925T095000EXDATE;TZID=Europe/London:20190911T095000) at position 15 (E): The timezone could not be found in the database

C:\ics-parser\vendor\nesbot\carbon\src\Carbon\Carbon.php:555
C:\ics-parser\src\ICal\ICal.php:2169
C:\ics-parser\src\ICal\ICal.php:1278
C:\ics-parser\src\ICal\ICal.php:709
C:\ics-parser\src\ICal\ICal.php:541
C:\ics-parser\tests\RecurrencesTest.php:270
C:\ics-parser\tests\RecurrencesTest.php:229

@s0600204
Copy link
Collaborator Author

Hmm. Seems to be a whitespace issue stemming from trying to include multiple ical lines in one argument to RecurrencesTest.php's version of formatIcalEvent().

#247 should make adding EXDATEs to tests in this file viable. Once that's been considered, I'll update this PR.

@u01jmg3
Copy link
Owner

u01jmg3 commented Nov 26, 2019

Testing now passes ✔️

However, there are a couple of differences between the example events that are produced before and after this PR.

Notably the Paris Timezone Test no longer appears. There are also slight differences in the output of the BYMONTHDAY Test (22-09-2016 13:00) event and the Parent Recurrence Event (13-08-2017 19:00). Is this expected?

As this messes up the processing of events with `RECURRENCE-ID` (which is reliant
on the events they replace remaining at the same (numerical) index in the array).
@s0600204
Copy link
Collaborator Author

s0600204 commented Dec 3, 2019

Should be resolved, now.

The Paris Timezone Test was not being included due to it having an invalid RRULE (touched on in #236) and me forgetting to add a line to retain the event in this case.

The other two differences were interlinked. It appears that the code processing events with a RECURRENCE-ID is reliant on the entries of $this->cal['EVENTS'] array remaining at the same (numerical and otherwise arbitrary) index after recurrence-determination as before. If not, then the wrong event gets replaced.

Anyhow, $this->cal['EVENTS'] is now modified instead of replaced, which should solve both problems.

@u01jmg3
Copy link
Owner

u01jmg3 commented Dec 3, 2019

The Paris Timezone Test was not being included due to it having an invalid RRULE (touched on in #236) and me forgetting to add a line to retain the event in this case.

Will change to RRULE:FREQ=WEEKLY;BYDAY=WE

It appears that the code processing events with a RECURRENCE-ID is reliant on the entries of $this->cal['EVENTS'] array remaining at the same (numerical and otherwise arbitrary) index after recurrence-determination as before.

🤢

@u01jmg3 u01jmg3 merged commit 2941e9a into u01jmg3:master Dec 3, 2019
@s0600204 s0600204 deleted the issue-240 branch December 3, 2019 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants