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

Fix recurrence calculation #210

Merged
merged 4 commits into from
Mar 28, 2019

Conversation

hubermat
Copy link
Contributor

@hubermat hubermat commented Mar 15, 2019

This is the second pull request for issue #180. The first one added unit testing (PR #209), so that the fixes contained in this pull request can be automatically tested.

The pull request fixes four topics:

  • In iCalDateToUnixTimestamp it removes the $offset completely, as this is not only not needed, but simply wrong. With the $offset, the returned UNIX timestamp is incorrect. $forceTimeZone is already handled in iCalDateToDateTime, so there is no need to add an extra offset for it.

  • In processRecurrences it removes the $isAllDayEvent completely. It was previously used to set the timezone hard to UTC, but this has the unintended side effect of shifting whole day events by the timezone offset, if the initial timezone of the ICS data was not UTC. E.g. there is ICS data with a base timezone of Europe/Berlin and an event with start date 20180701, end date 20180702. It would then be treated as an all day event, but its resulting start datetime would be 20180701T000000 UTC which is wrong. Without the $isAllDayEvent, these all day events are now calculated correctly in the original timezone.

  • In processRecurrences it adds an if-clause to the MONTHLY case, as otherwise for non UTC timezones the event series would contain double start or end events (original start event + recurrence event, both for the same day). The reason is the way the monthly recurrences are calculated. The if-clause simply filters these cases out.

  • In processEventIcalDateTime it adds a recalculation of the UNIX timestamp in the updated $event, as typically this method is used to update the DateTime (array index 3), but the UNIX timestamp (array index 2) is not updated accordingly (leading to a mismatch between DateTime-String (with timezone) and UNIX timestamp.

All changes can be tested with the unit testing (introduced with pull request #209). Simply run composer test.

@u01jmg3 u01jmg3 merged commit a7c91e6 into u01jmg3:master Mar 28, 2019
@u01jmg3 u01jmg3 added this to the v2.x.x milestone Mar 28, 2019
u01jmg3 added a commit that referenced this pull request Mar 28, 2019
Update options to define all possible values
Only use double quotes when necessary
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants