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?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

crifi’s picture

Title: All Day events cause ical parsing errors » All Day events not RFC 2445/3339 compliant

I 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.

crifi’s picture

Here are some more informations according Outlook 2010. Generated ICS file (http://mosbacher-berg.de/calendar/ical/2011-02), first 21 lines only:

VERSION:2.0
METHOD:PUBLISH
X-WR-CALNAME: Kalender | Gymnasium am Mosbacher Berg
PRODID:-//Drupal iCal API//EN
BEGIN:VEVENT
UID:calendar.1004.field_when.0.0
SUMMARY:Mittelverteilungskonferenz
DTSTAMP:20110221T232619Z
DTSTART:20110221Z
DTEND:20110221Z
URL;VALUE=URI:http://mosbacher-berg.de/node/1004
END:VEVENT
BEGIN:VEVENT
UID:calendar.488.field_when.0.1
SUMMARY:Info 11a GO
DTSTAMP:20110221T232619Z
DTSTART:20110222T180000Z
DTEND:20110222T180000Z
URL;VALUE=URI:http://mosbacher-berg.de/node/488
END:VEVENT
[..]

Error messages of Outlook 2010:

Fehler (0x0004002B) beim Ausführen der Aufgabe "Kalender " Kalender | Gymnasium am Mosbacher Berg" wird geöffnet": "Die DTSTART-Eigenschaft im Bereich von Zeile 10 kann von Outlook nicht analysiert werden. Die Eigenschaft wird ignoriert."
Fehler (0x0004002B) beim Ausführen der Aufgabe "Kalender " Kalender | Gymnasium am Mosbacher Berg" wird geöffnet": "Die DTEND-Eigenschaft im Bereich von Zeile 11 kann von Outlook nicht analysiert werden. Die Eigenschaft wird ignoriert."

Translation:

Error (0x0004002B) during opening of task "calendar" " Kalender | Gymnasium am Mosbacher Berg": "The DSTART attribute near line 10 can not analyzed by Outlook. Attribute ignored."
[..]
crifi’s picture

For all day events we can use this notation instead of DTSTART:20110308Z

DTSTART;VALUE=DATE:20110308
hirbys’s picture

I 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."

hirbys’s picture

I 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:


<?php
/**
 * $calname
 *   The name of the calendar.
 * $site_timezone
 *   The name of the site timezone.
 * $events
 *   An array with the following information about each event:
 *
 *   $event['uid'] - a unique id for the event (usually the url).
 *   $event['summary'] - the name of the event.
 *   $event['start'] - the formatted start date of the event.
 *   $event['end'] - the formatted end date of the event.
 *   $event['rrule'] - the RRULE of the event, if any.
 *   $event['timezone'] - the formatted timezone name of the event, if any.
 *   $event['url'] - the url for the event.
 *   $event['location'] - the name of the event location.
 *   $event['description'] - a description of the event.
 *
 *   Note that there are empty spaces after RRULE, URL, LOCATION, etc
 *   that are needed to make sure we get the required line break.
 *
 */

?>
BEGIN:VCALENDAR
VERSION:2.0
METHOD:PUBLISH
X-WR-CALNAME: <?php print $calname . "\r\n"; ?>
PRODID:-//Drupal iCal API FCUCC Rev 20110521//EN
<?php foreach($events as $event): ?>
BEGIN:VEVENT
UID:<?php print($event['uid'] . "\r\n") ?>
SUMMARY:<?php print($event['summary'] . "\r\n") ?>
DTSTAMP:<?php print($current_date . "Z\r\n") ?>
<?php if ( ($event['start'] == $event['end']) && (!strpos( $event['start'],'T' ))): ?>
DTSTART;VALUE=DATE:<?php print( $event['start'] . "\r\n") ?>
DTEND;VALUE=DATE:<?php print( $event['start'] . "\r\n") ?>
<?php else: ?>
DTSTART:<?php print($event['start'] . "Z\r\n") ?>
<?php if (!empty($event['end'])): ?>
DTEND:<?php print($event['end'] . "Z\r\n") ?>
<?php endif; ?>
<?php endif; ?>
<?php if (!empty($event['rrule'])) : ?>
<?php print($event['rrule'] . "\r\n") ?>
<?php endif; ?>
<?php if (!empty($event['url'])): ?>
URL;VALUE=URI:<?php print($event['url'] . "\r\n") ?>
<?php endif; ?>
<?php if (!empty($event['location'])): ?>
LOCATION:<?php print($event['location'] . "\r\n") ?>
<?php endif; ?>
<?php if (!empty($event['description'])) : ?>
DESCRIPTION:<?php print($event['description'] . "\r\n") ?>
<?php endif; ?>
END:VEVENT
<?php endforeach; ?>
END:VCALENDAR
crifi’s picture

Thanks 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):

BEGIN:VEVENT
SUMMARY:iCal testentry
DTSTAMP:20110523T113719Z
DTSTART:20110525Z
DTEND:20110531Z
[...]
END:VEVENT
hirbys’s picture

Re: 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.

hirbys’s picture

Status: Active » Needs review
FileSize
931 bytes

I'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.

crifi’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
945 bytes

Very 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.

crifi’s picture

crifi’s picture

There 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

stella’s picture

This 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.

benjifisher’s picture

@stella: The patch applied for me:

modules/calendar$ patch --dry-run -p1 < ~/src/calendar-ical-1038218-9.patch 
patching file calendar_ical/calendar-view-ical.tpl.php
Hunk #1 succeeded at 34 (offset 1 line).

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.

benjifisher’s picture

Status: Reviewed & tested by the community » Needs review

Sorry 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

DTSTART;VALUE=DATE:20111021
DTEND;VALUE=DATE:20111023

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

DTSTART;VALUE=DATE:20111021
DTEND;VALUE=DATE:20111024

and I am curious what others think.

crifi’s picture

Status: Needs review » Needs work

Yeah, 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):

The following is an example of the "VEVENT" calendar component used to represent a multi-day event scheduled from June 28th, 2007 to July 8th, 2007 inclusively. Note that the "DTEND" property is set to July 9th, 2007, since the "DTEND" property specifies the non-inclusive end of the event:

BEGIN:VEVENT
UID:20070423T123432Z-541111@example.com
DTSTAMP:20070423T123432Z
DTSTART;VALUE=DATE:20070628
DTEND;VALUE=DATE:20070709
SUMMARY:Festival International de Jazz de Montreal
TRANSP:TRANSPARENT
END:VEVENT

I also closed an duplicate here: #483132: iCal export multi-day is one day short in Microsoft Outlook

crifi’s picture

Status: Needs work » Needs review
FileSize
945 bytes

This is a new patch including the problem mentioned in #13 and #14. Please review it.

crifi’s picture

FileSize
976 bytes

Sorry! 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.

benjifisher’s picture

Title: All Day events not RFC 2445/3339 compliant » All Day events not RFC 2445/3339/5545 compliant
FileSize
1.22 KB

@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:

 print( ($event['end'] + 1) . "\r\n") 

I think that is simpler than your method.

To be cautious, I put this inside

 if (preg_match('/^\d+$/', $event['end'])): 

instead of the original

 if (!strpos( $event['end'],'T')): 

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.

crifi’s picture

<?php
 print( ($event['end'] + 1) . "\r\n") 
?>

What about setting the date to 20110531 and increment this date? ;-)

<?php
 if (preg_match('/^\d+$/', $event['end'])): 
?>

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?

benjifisher’s picture

@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() and strtotime() in a template file. I think these date strings come from modules/date/theme/theme.inc:

function template_preprocess_date_vcalendar(&$vars) {
// skip some lines ...
      $date_format = ($row->calendar_all_day == TRUE) ? DATE_FORMAT_ICAL_DATE : DATE_FORMAT_ICAL;
// ...
      $events[$uid]['start'] = date_format($event['start'], $date_format);
      if ($event['start'] && $event['end']) {
        $events[$uid]['end'] = date_format($event['end'], $date_format);
      }
      else {
        $events[$uid]['end'] = $events[$uid]['start'];
      }

and the formats are defined in modules/date/date_api.module:

define('DATE_FORMAT_ICAL', "Ymd\THis");
define('DATE_FORMAT_ICAL_DATE', "Ymd");

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:

DTSTART;<?php print $event['timezone'] ?><?php print($event['start'] . "\r\n") ?>
<?php if (!empty($event['end'])): ?>
DTEND;<?php print $event['timezone'] ?><?php print($event['end'] . "\r\n") ?>
crifi’s picture

Status: Needs review » Needs work

I still do not like the idea of using date() and strtotime() in a template file.

Thanks 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:

/**
 *  Preprocessor to construct an ical vcalendar
 *
 * @param $events
 *   An array of events where each event is an array keyed on the uid:
 *    'start'
 *      Start date object,
 *    'end'
 *      End date object, optional, omit for all day event.
 * [..]
 */
function template_preprocess_date_vcalendar(&$vars) {

So, we need also a date module issue...

crifi’s picture

Argh! 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. :-(

benjifisher’s picture

Both 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 ...

benjifisher’s picture

Status: Needs work » Reviewed & tested by the community

I 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.

 if (!strpos( $event['start'],'T')):
// skip some lines ...
if (!strpos( $event['end'],'T')): 

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.

crifi’s picture

Issue summary: View changes

I added an issue summary.

crifi’s picture

FileSize
944 bytes

Okay. So here is a patch which is fulfilling the coding standards.

crifi’s picture

Version: 6.x-2.x-dev » 7.x-3.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

We need a patch ported to the highest dev version now.

benjifisher’s picture

I have opened an issue for the D7 version: #1284170: All-day events missing or wrong in ical feed.

benjifisher’s picture

Status: Patch (to be ported) » Reviewed & tested by the community

According to the Status definitions,

Patch (to be ported)
The patch has been successfully committed to a branch of the project, and still needs to be committed to another, but the current patch doesn't apply to the target branch and needs to be modified in order to do so.

So I am reverting the status to RTBC.

I think I will also add the related issue from Comment #27 to the issue summary.

benjifisher’s picture

Issue summary: View changes

Recommend patch from Comment #25 instead of the one from #9. The only difference is white space.

crifi’s picture

Version: 7.x-3.x-dev » 6.x-2.x-dev

Okay, so we patch the D7 version in an own bug report. And this is the D6 only fix.

crifi’s picture

Issue summary: View changes

I added a related issue to the issue summary.

benjifisher’s picture

Assigned: Unassigned » benjifisher
Status: Reviewed & tested by the community » Needs work
crifi’s picture

Status: Needs work » Needs review
FileSize
911 bytes

Patch for the new code cleanup version in repo. Please review it!

benjifisher’s picture

Status: Needs review » Reviewed & tested by the community

I 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!

benjifisher’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.27 KB

I 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.

crifi’s picture

But this was not introduced by my patch. ;-) Thanks for the code review. In my test it works pretty well.

crifi’s picture

Status: Needs review » Reviewed & tested by the community

But this was not introduced by my patch. ;-) Thanks for the code review. In my test it works pretty well.

benjifisher’s picture

Right. My fault, my fix.

Jonathan Stein’s picture

I wonder why this bug fix has not been released yet? The status says "reviewed & tested by the community". What more needs to be done?

benjifisher’s picture

It 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."

benjifisher’s picture

Issue summary: View changes

Add a warning about using git apply.

neclimdul’s picture

Assigned: benjifisher » neclimdul
Status: Reviewed & tested by the community » Needs review
FileSize
2.32 KB

Updated patch with fixes for timezones if they are not UTC.

jvieille’s picture

Nothing was committed apparently.
Patch 39 is OK, but the dates are shifted 1 day early.
I had to modify as follows:

...
    else {
      // Otherwise, this is a DATE value so specify that in the conditionals.
      $start_conditionals .= ';VALUE=DATE';
+    $event['start'] = $event['start']+1;
     }
    if ($event['end']) {
      // If there is a T inside the string this is a DATE-TIME.
      if (strpos($event['end'], 'T')) {
        // Adjust the string to specify UTC times.
        $event['end'] .= 'Z';
      }
      else {
        // Otherwise, this is a DATE value so specify that in the conditionals.
        $end_conditionals .= ';VALUE=DATE';
+      $event['end'] = $event['end']+1;        
      }
...
neclimdul’s picture

its 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.

izmeez’s picture

izmeez’s picture

neclimdul’s picture

Assigned: neclimdul » Unassigned
Neslee Canil Pinto’s picture

Status: Needs review » Closed (outdated)

The 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.