search

spatie / calendar-links

Generate add to calendar links for Google, iCal and other calendar systems
https://spatie.be/opensource
MIT License
914 stars 149 forks source link

Switch to sabre/vobject and add support VTIMEZONE for ICS link #75

Closed dogawaf closed 3 years ago

dogawaf commented 5 years ago

This PR introduces the usage of sabre/vobject for generating ics file. sabre/vobject is a pretty stable and robust library.

The escaping of strings and the date and timezone handling is delegated to the library.

This PR also adds a VTIMEZONE component when the event features dates that are not on UTC. VTIMEZONE is mandatory for any TZID parameter on dates of VEVENT (according rfc). This fixes an issue when importing an ics on Outlook, which cannot handle properly timezone if the ics file have no VTIMEZONE component.

Fixes #73

tusharvikky commented 4 years ago

Hello,

Any update on the merge?

Regards

spatie-bot commented 4 years ago

Dear contributor,

because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.

atymic commented 4 years ago

cc @freekmurze

Any chance of a review here? Thanks :)

spatie-bot commented 4 years ago

Dear contributor,

because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.

atymic commented 4 years ago

@freekmurze

atymic commented 4 years ago

cc @freekmurze

spatie-bot commented 3 years ago

Dear contributor,

because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.

philwolstenholme commented 3 years ago

I still think this would be a useful change, and it seems a shame to close something that has had a code review and is waiting on some changes

dogawaf commented 3 years ago

I created this PR and use it on a specific project, and did not have any issue with it since. But merging it as is would not be a very good idea. I agree with @lptn on pretty much all the remarks. I did not have the resource since to fix the remarks and add some tests (though sabre/vtimezone is already pretty well tested), but who knows... If somebody wan't to complete this PR and start again the review process, got for it. Or, you can of course apply the patch for your specific need (like I did). Cheers.

abferris commented 3 years ago

Hi there! I'm working on trying to wrap some of these up as a client has had issues. I'm not sure if all of this still applies, but I'm doing my best to work out exactly how all of it works. I'll be putting in comments in response to the above changes.

bboro commented 3 years ago

Hi @lptn @abferris

I've worked on some of the PR review suggestions, and also rerolled this against latest master (212a8c1). As I didn't have permission to push to @dogawaf's fork, I've created a PR here from my own fork to this PR: https://github.com/dogawaf/calendar-links/pull/3

I understand with all the changes coming in from master, it might be difficult to see what's changed. So here's a PR from the same branch against 212a8c1: https://github.com/bboro/calendar-links/pull/2/files

It would be great to move this forward and merged! Hope this helps.

mitcks commented 3 years ago

@bboro have you had any updates on this fix getting merged in?

bboro commented 3 years ago

@mitcks not yet