Problem/Motivation

When using the calendar_ical module to generate a feed, events that are more than one day and are all-day events (formatted as DATE instead of DATETIME) are off by one day. This is because the ical specification (RFC 2445, clarified in RFC 5545) specifies that the DTEND value is non-inclusive. The two calendar programs I use (Apple iCal and Google Calendar) both follow the ical specification, and when I import a Drupal-generated ical into either program, my events show up ending one day earlier than they should.

Another problem is that dates (both DTSTART and DTEND) for all-day events are calculated incorrectly if the local timezone has a positive offset relative to UTC. For example, Europe/Berlin.

Proposed resolution

The attached patch modifies template_preprocess_date_vcalendar() in theme/theme.inc.

A work-around is to put a modified calendar/calendar_ical/calendar-view-ical.tpl.php into your theme folder. See #1038218-17: All Day events not RFC 2445/3339/5545 compliant. Or you could copy template_preprocess_date_vcalendar() to template.php in your own theme folder, rename the function THEME_preprocess_date_vcalendar() (replacing THEME with the name of your theme) and make the changes from this patch.

Remaining tasks

Please review this patch.

User interface changes

N/A

API changes

This patch modifies template_preprocess_date_vcalendar().

Related Issues

If you commit this patch, please give joint credit to crifi and benjifisher (see #1038218: All Day events not RFC 2445/3339/5545 compliant). We discussed the issue and developed patches there before deciding that this change should be made here in the preprocess function instead of in the template file.

The patch in #5 also addresses #517186: iCal doesn't take account of timezones in all day events

When going in the other direction (importing an ical file into Drupal) the corresponding change should be made. See #606658: Multiple day VEVENT not working from iCal.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

crifi’s picture

Status: Active » Reviewed & tested by the community

Since this is a modification of my patch (http://drupal.org/node/1038218#comment-4994326) at another location, I have reviewed it and it's working.

benjifisher’s picture

I have opened an issue for the D7 version: #1284170: All-day events missing or wrong in ical feed. For D7, the patch is in the Calendar module, not Date.

benjifisher’s picture

This patch changes template_preprocess_date_vcalendar() in modules/date/theme/theme.inc, which is called in the last line of template_preprocess_calendar_view_ical() in modules/calendar/calendar_ical/theme.inc. So we could fix this problem in the Calendar module instead of in the Date module. Does the code from the Date module get used anywhere else?

crifi’s picture

It isn't a calendar module specific problem since it's definitely a bug with this function. A third party module could also use this function for own ical feeds. In my opinion timezone calculation isn't the task of the calendar ical module and should be fully transfered from the calendar module to the date module.

benjifisher’s picture

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

The attached patch correctly calculates the DTSTART and DTEND values for all-day events in more cases. In timezones with a positive offset from UTC, such as Europe/Berlin, a local time such as "2011-09-05 00:00:00" is converted into something like "2011-09-04 22:00:00" in UTC. We want to use 09-05 and not 09-04. See #517186: iCal doesn't take account of timezones in all day events.

The attached patch is a lot larger than the previous one because I replaced

  foreach ($events_in as $uid => $event) {
    $row = array_shift($rows);
    // Omit any items with empty dates.
    if (!empty($event['start'])) {
      // many lines to end of foreach block
    }
  }

with

  foreach ($events_in as $uid => $event) {
    $row = array_shift($rows);
    // Skip any items with empty dates.
    if (empty($event['start'])) {
      continue;
    }
    // rest of foreach block
  }

I will also update the issue summary.

benjifisher’s picture

crifi has confirmed that the patch in #5 fixes #517186: iCal doesn't take account of timezones in all day events and marked that issue as a duplicate of this one.

crifi’s picture

Thanks Benji, I will review the code by the end of this week.

penyaskito’s picture

Subscribing

crifi’s picture

Status: Needs review » Reviewed & tested by the community

Very good work. I have reviewed the code and patch works here in my testing environment.

benjifisher’s picture

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

Status: Needs work » Needs review
FileSize
3.65 KB

This is an update of the patch in #5. I changed only one character: It seems that an earlier patch correctly replaced a ";" with a ":". The patch in #5 already made the same change.

I have not tested this yet. Any testing of this patch should be done along with crifi's patch in #1038218: All Day events not RFC 2445/3339/5545 compliant.

It has been more than three months since I made the patch in #5, and I do not remember everything I was doing, but it was marked RTBC at the time.

benjifisher’s picture

I tested and validated the patch in #11. If it were not my own work, I would mark it RTBC.

arlinsandbulte’s picture

Assigned: benjifisher » Unassigned
Status: Needs review » Reviewed & tested by the community

I'm going to ask KarenS to take a look at this & the other related patch... #1038218: All Day events not RFC 2445/3339/5545 compliant

arlinsandbulte’s picture

Assigned: Unassigned » arlinsandbulte

putting on my list to review & commit...

KarenS’s picture

Status: Reviewed & tested by the community » Needs work

The first few lines of the patch make changes to the code that have nothing to do with this problem, they just handle the test for empty dates in a different way, which makes it harder to review the patch. It's best to make patches that only touch the bug.

I see:

-      $date_format = ($row->calendar_all_day == TRUE) ? DATE_FORMAT_ICAL_DATE : DATE_FORMAT_ICAL;

and I don't see $date_format added back anywhere, but the variable is still used. That doesn't seem right.

Please see if you can reduce this patch to just the changes needed to eliminate the bug.

arlinsandbulte’s picture

Assigned: arlinsandbulte » Unassigned

Yep, I agree. Needs work.

+++ b/theme/theme.inc
@@ -178,42 +178,57 @@ function template_preprocess_date_vcalendar(&$vars) {
+    if (empty($event['start'])) {
+      continue;
+    }

IMO, I prefer not using a continue statements like this. Seems like spaghetti code to me.
Also, I think this is what KarenS meant by her comment in #15. This changes the indentation of everything below which makes it REALLY hard to review as every line changes, even if it is only an indentation change.

First make a patch the does the MINIMUM to fix the bug.
Later we can talk about code style patches.

arlinsandbulte’s picture

Status: Needs work » Needs review
FileSize
2.07 KB

Here is a re-factored patch that does not change the test for empty dates.

Also, while KarenS was correct that the $date_format declaration was removed, she was incorrect that it was still used elsewhere in the function. After this patch, it is NOT used anywhere else in the function.

KarenS’s picture

OK, this it's much easier to see what you're doing here. We have a date object to use, we don't need strtotime() or date() functions. In fact we avoid using them because you can't control the timezone in those the way you can with date objects.

Going back to the way this was already fixed in D7, and using the date object we already have, I came up with this patch instead:

       $date_format = ($row->calendar_all_day == TRUE) ? DATE_FORMAT_ICAL_DATE : DATE_FORMAT_ICAL;
       $events[$uid]['start'] = date_format($event['start'], $date_format);
+
+      // According to RFC 2445 (clarified in RFC 5545) the DTEND value is
+      // non-inclusive.  When it is a DATE rather than a DATETIME, this means
+      // that we should add one day to its value.
+      if ($row->calendar_all_day) {
+        if (empty($event['end'])) {
+          $event['end'] = $event['start'];
+        }
+        date_modify($event['end'], "+1 day");
+      }
+
       if ($event['start'] && $event['end']) {
         $events[$uid]['end'] = date_format($event['end'], $date_format);
       }
       else {
         $events[$uid]['end'] = $events[$uid]['start'];
       }

KarenS’s picture

And if the start date getting passed in is wrong, that is a calendar issue. This function should assume the right values have already be passed in.

KarenS’s picture

Status: Needs review » Fixed

Committed this. There may still be a calendar issue for the way the start date is created if you get the wrong value by formatting it. There may already be an issue for that, but if not please open a calendar issue for the problem with the timezone conversion in the start date.

http://drupalcode.org/project/date.git/commit/5004a85

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

I added a reference to #517186: iCal doesn't take account of timezones in all day events and mentioned that the latest patch fixes that problem, too.