Problem/Motivation
The ical feed http://mysite.org/calendar/ical
does not conform to the ical standard, RFC 2445. Specifically, if the DTSTART and DTEND values are formatted as DATE and not DATETIME, then the syntax needs to be changed. The OP recommends using http://icalvalid.cloudapp.net and http://severinghaus.org/projects/icv/ to validate ical feeds.
Proposed resolution
The patch in #9 modifies calendar/calendar_ical/calendar-view-ical.tpl.php
to format DTSTART and DTEND values correctly.
Warning when using git apply
If you have problems applying the patch using git apply
, then see #1302052: CRLF line endings cause problems with "git apply" or use
$ patch -p1 < /path/to/patchfile.patch
Remaining tasks
Please commit the patch from Comment #25. Credit should go to crifi and hirbys. (See Comments #8 and #9.)
User interface changes
N/A
API changes
N/A
Related Issues
Comments 14–23 deal with an off-by-one problem with the DTEND value. This has been moved to the Date issue queue: #1282538: ical feed generates wrong DTEND value for multi-day, all-day events.
For D7, both issues can be solved within the Calendar module. See #1284170: All-day events missing or wrong in ical feed.
Original report by gmak
I've just spent the afternoon changing all my 'All Day' events (eg. those that have a start and end time of 00:00) to have set time periods. The reason was I was suddenly finding that the ical export to Google Calendar was broken. I'm waiting to see if this fixes things, but I did find that the all-day events in my calendar were causing parsing errors (according to http://icalvalid.cloudapp.net and http://severinghaus.org/projects/icv/)
The CloudApp checker was presenting errors such as: The 'date-time' value '20110212Z' is not formatted properly.
Having changed all the all-day events, both checkers give 100% scores.
Is there some trick to All Day events for ical export or did I find a bug?
Comment | File | Size | Author |
---|---|---|---|
#39 | 1038218-calendar-ical-date-39.patch | 2.32 KB | neclimdul |
#16 | calendar-ical-1038218-9.patch | 945 bytes | crifi |
#9 | calendar-ical-1038218-9.patch | 945 bytes | crifi |
#8 | calendar-ical-1038218-8.patch | 931 bytes | hirbys |
Comments
Comment #1
crifi CreditAttribution: crifi commentedI can confirm this bug. I think this is still not RFC compliant and produces errors while importing to several programs (Outlook 2010 is here affected). Like advised in #760316: calendar_ical isn't RFC compliant this bug is a new one and the old one shouldn't be reopened.
Comment #2
crifi CreditAttribution: crifi commentedHere are some more informations according Outlook 2010. Generated ICS file (http://mosbacher-berg.de/calendar/ical/2011-02), first 21 lines only:
Error messages of Outlook 2010:
Translation:
Comment #3
crifi CreditAttribution: crifi commentedFor all day events we can use this notation instead of DTSTART:20110308Z
Comment #4
hirbys CreditAttribution: hirbys commentedI am running into this problem as well. When all-day events appear in the feed, the feed does not validate, and Tightrope Media's Carousel will not import it. Example feed at http://www.firstcongoappleton.org/calendar-date/ical/ From the validator at http://icalvalid.cloudapp.net/, the reported error is: "The 'date-time' value '20110522Z' is not formatted properly."
Comment #5
hirbys CreditAttribution: hirbys commentedI solved this problem for myself by overriding the default Views ICAL template file in my site's theme. Following crifi's suggestion, before printing DTSTART and DTEND, I test for an all-day event (start date = end date and no 'T' marker in the date-time value), and if I find one, I use the alternate date value syntax suggested. This approach validates at http://icalvalid.cloudapp.net/, and iCal imports the feed.
Here's my code:
Comment #6
crifi CreditAttribution: crifi commentedThanks for the submitted patch suggestion. The patch is still not complete, because it doesn't handle events properly, which are over a few days like this (but without a time set):
Comment #7
hirbys CreditAttribution: hirbys commentedRe: comment #6:
Ah, yes - my proposed code will not generate a valid feed in the case of all-day events spanning days. I'll work to address that next.
Comment #8
hirbys CreditAttribution: hirbys commentedI've now dealt with the remaining problem case, in which all-day events (no time) span days, and my feed validates. I've attached a patch which, which, when applied, results in a valid feed at http://www.firstcongoappleton.org/calendar-date/ical/2011-05. Note: first-ever patch submission. Not at all sure about the naming convention. Please let me know what I could have done better.
Comment #9
crifi CreditAttribution: crifi commentedVery good job, hirbys! This patch is simple and effective. I've created the following patch with git, so that the path standards are complied and the patch is ready to commit.
Comment #10
crifi CreditAttribution: crifi commentedMarked #1036802: Whole day timestamps invalid in ical export as duplicate of this.
Comment #11
crifi CreditAttribution: crifi commentedThere is a related issue to all-day events based on a timezone problem #517186: iCal doesn't take account of timezones in all day events
Comment #12
stella CreditAttribution: stella commentedThis fixes the problem for me too. I was using the iCal app on Mac OS X and it was showing all the upcoming events with today's date. This was fixed with the above patch, though it didn't apply cleanly and I had to manually copy and paste in the changes.
Comment #13
benjifisher@stella: The patch applied for me:
Note the working directory and the
-p1
option. I always run patch with the--dry-run
option when I am not sure if it will apply cleanly. Once it does, I run patch without that option.I also use iCal on the Mac. Happy me: with this patch, multi-day events now display properly, instead of covering the whole display. Unfortunately, they do not display on the final day, but I think this is a bug in iCal. When I look at calendar.ics in a text editor, it looks right.
Comment #14
benjifisherSorry to reply to myself, but I am beginning to think that Apple's iCal is not an error. Google Calendar treats multi-day events the same way.
I tried a Google search for "icalendar dtend value" and decided that the answer is murky. See, for example, these two pages: http://www.kanzaki.com/docs/ical/vevent.html and http://www.bedework.org/trac/bedework/wiki/Bedework/DevDocs/DtstartEndNotes.
I do not want to edit all my event dates (again) and change the date handling for the whole site, so I plan to modify my copy of calendar-view-ical.tpl.php. If people think this is the right solution, I will re-roll the patch.
Explicit example: If I have an event that runs from October 21 to October 23 (three days) then the patch in #9 generates
and both Apple iCal and Google Calendar display this on October 21 and 22 but not on October 23. On my site, I will change it to get
and I am curious what others think.
Comment #15
crifi CreditAttribution: crifi commentedYeah, you're right. There is still a bug in calculating the right end date of a multi day event, see RFC 5545 (http://tools.ietf.org/html/rfc5545#section-3.6.1):
I also closed an duplicate here: #483132: iCal export multi-day is one day short in Microsoft Outlook
Comment #16
crifi CreditAttribution: crifi commentedThis is a new patch including the problem mentioned in #13 and #14. Please review it.
Comment #17
crifi CreditAttribution: crifi commentedSorry! Wrong patch file. This is the right one. If this patch is correct we should also patch the active dev branch of the D7 version! We should also think about commenting the file.
Comment #18
benjifisher@crifi, thanks for finding the right RFC! I have updated the issue title.
As promised, I have made a new patch. I see you beat me to it. I increase the day by one with a little type juggling:
I think that is simpler than your method.
To be cautious, I put this inside
instead of the original
For consistency, I did this for DTSTART as well as DTEND.
The version of the tpl.php file that I get from git is different from the 6.2.x-dev version I have been using. My patch fixes a syntax error earlier in the file, so it might not apply cleanly to the 6.2.x-dev release.
This tpl.php file has more logic in it than most. It might be preferable to do more processing in the function that prepares these variables, but maybe what we are doing here is good enough for the 6.x branch. Something to think about before working on the 7.x branch, though.
It should also work to put the tpl.php file in your theme directory instead of modifying the one that comes with the calendar module. I have not tested.
Comment #19
crifi CreditAttribution: crifi commentedWhat about setting the date to 20110531 and increment this date? ;-)
In my opinion we don't need to introduce preg_match with proofing for numbers, since data is given by the calendar modules theme.inc and strpos() is faster than preg_match(). I also don't see a possibility to have a DATE-TIME type without a T delimiter. What do you think?
Comment #20
benjifisher@crifi: I concede: the idea of adding 1 was (at best) half baked. Given that, there is no reason to check that the date strings match /^\d+$/.
OK, your patch in #17 is certainly better than mine in #18. I will start testing it.
I still do not like the idea of using
date()
andstrtotime()
in a template file. I think these date strings come from modules/date/theme/theme.inc:and the formats are defined in modules/date/date_api.module:
Do you think it is worth doing the "add one day" calculation in
template_preprocess_date_vcalendar()
instead of in the template file?FWIW, the D7 version of modules/date/date_api/theme/date-vevent.tpl.php contains these lines:
Comment #21
crifi CreditAttribution: crifi commentedThanks for pointing out this. Yeah, I agree with you, this isn't a good idea. The correct point to fix this is the function template_preprocess_date_vcalendar() in [date-module]/theme/theme.inc. There is also a comment, that this code is for building ical feeds:
So, we need also a date module issue...
Comment #22
crifi CreditAttribution: crifi commentedArgh! There are already issues in the date module queue:
- #606658: Multiple day VEVENT not working from iCal
- #370092: All-day event support for ical
We should now first proof our issue against this similar issues. There are also some out of module fixes against the calendar module. :-(
Comment #23
benjifisherBoth Date and Calendar modules are maintained by KarenS. I think we need buy-in from her before we can get anything committed.
For now, we should test plopping the improved template file into our theme directories. Not tonight ...
Comment #24
benjifisherI added an issue summary.
I have moved this all the way from "needs work" to RTBC, but I have a good reason. The patch in #9 was already marked as RTBC before I raised additional issues in Comment #14. We agree that the right place to deal with these issues is in the Date module, so I think the patch from Comment #9 is ready to go. I opened a new issue in the Date queue: #1282538: ical feed generates wrong DTEND value for multi-day, all-day events. I include a patch there, based on crifi's patch in #16. I forgot to mark it as "needs review," but please check it out and mark it RTBC if you think it works.
I do have one quibble with the patch in #9: whitespace.
But I do not care enough to roll a new patch, sorry!
I spent a lot of time looking at related issues in the Date and Calendar queues. I now have a lot more sympathy for KarenS! The two issues that crifi mentions in #22 are very stale, with latest patches in those issues from January 8, 2010 and November 11, 2009. I am pretty sure that those patches will not apply to any recent release of the Date and Calendar modules, so I do not feel too bad about not cooperating with them. Most of the issues I looked at related to timezone handling.
Comment #24.0
crifi CreditAttribution: crifi commentedI added an issue summary.
Comment #25
crifi CreditAttribution: crifi commentedOkay. So here is a patch which is fulfilling the coding standards.
Comment #26
crifi CreditAttribution: crifi commentedWe need a patch ported to the highest dev version now.
Comment #27
benjifisherI have opened an issue for the D7 version: #1284170: All-day events missing or wrong in ical feed.
Comment #28
benjifisherAccording to the Status definitions,
So I am reverting the status to RTBC.
I think I will also add the related issue from Comment #27 to the issue summary.
Comment #28.0
benjifisherRecommend patch from Comment #25 instead of the one from #9. The only difference is white space.
Comment #29
crifi CreditAttribution: crifi commentedOkay, so we patch the D7 version in an own bug report. And this is the D6 only fix.
Comment #29.0
crifi CreditAttribution: crifi commentedI added a related issue to the issue summary.
Comment #30
benjifisherThis needs to be updated after #1302052-23: CRLF line endings cause problems with "git apply".
Comment #31
crifi CreditAttribution: crifi commentedPatch for the new code cleanup version in repo. Please review it!
Comment #32
benjifisherI looked at the code, and it is a pretty simple update of the patch in #25, which was already RTBC.
I tested, and it works as advertised.
I validated with http://icalvalid.cloudapp.net/ and it looks good.
Please commit this patch!
Comment #33
benjifisherI spoke too soon. There was a stray
"print"
in the code. Please test the attached patch, instead.While I was at it, I changed one more
if (){...}
to the alternative syntax. This is consistent with the rest of the file and with Drupal coding standards for .tpl.php files.Comment #34
crifi CreditAttribution: crifi commentedBut this was not introduced by my patch. ;-) Thanks for the code review. In my test it works pretty well.
Comment #35
crifi CreditAttribution: crifi commentedBut this was not introduced by my patch. ;-) Thanks for the code review. In my test it works pretty well.
Comment #36
benjifisherRight. My fault, my fix.
Comment #37
Jonathan Stein CreditAttribution: Jonathan Stein commentedI wonder why this bug fix has not been released yet? The status says "reviewed & tested by the community". What more needs to be done?
Comment #38
benjifisherIt just needs some attention from the maintainers. The issue summary has a link to the corresponding issue on the D7 issues queue. That patch has already been committed and marked as "ready to port."
Comment #38.0
benjifisherAdd a warning about using git apply.
Comment #39
neclimdulUpdated patch with fixes for timezones if they are not UTC.
Comment #40
jvieille CreditAttribution: jvieille commentedNothing was committed apparently.
Patch 39 is OK, but the dates are shifted 1 day early.
I had to modify as follows:
Comment #41
neclimdulits been at this point literally years since I looked at this but as I remember that generally had to do with time zone issues and the implementation was a best effort. Timezones are always tricky.
Comment #42
izmeez CreditAttribution: izmeez commentedThe comment #2200263-7: All day events not displaying on first day of month in month view may help with timezone.
Comment #43
izmeez CreditAttribution: izmeez commentedThere is overlap in the patch in this queue and the one in #975812: webcal repeating/all day events are not showing in ical feed
Comment #44
neclimdulComment #45
Neslee Canil PintoThe D6 branch is no longer supported so we're closing this issue. If you think this issue still exists in D7 you can open a new one.