i think I've found the source of the problem affecting (among other things) CiviEvent importation: when the DTSTART/END format includes VALUE=DATE rather than TZ=.*, such as when timestamps are given in zulu time (ie. 'Z' at end), an "if" matches which assumes a numerical input format that may be invalid, and the GMT timezone setting is inadvertently skipped.

i've changed the logic so that 1) it looks right to me, and 2) it works on my system. YMMV.

below is a diff with the most up to date -dev version. basically, i removed unnecessary check from if(), moved a closing bracket up to a sensible location, and moved the zulu-time setting to right before the timezone section.

(also includes change to _date_ical_tz(), see previous issue.)

hth,
.b

176c176
<                 if (ereg ('^([0-9]{4})([0-9]{2})([0-9]{2})$', $data)) { // removed incorrect half of test:  preg_match("/^DTSTART;VALUE=DATE/i", $field) ||
---
>                 if ((preg_match("/^DTSTART;VALUE=DATE/i", $field)) || (ereg ('^([0-9]{4})([0-9]{2})([0-9]{2})$', $data))) {
187a188,190
>                   elseif ($zulu_time) {
>                     $tz_dtstart = 'GMT';
>                   }
192d194
<                 }
195,197d196
<                   if ($zulu_time) {
<                     $tz_dtstart = 'GMT';
<                   }
221c220
<
---
>                 }
229c228
<             if (ereg ('^([0-9]{4})([0-9]{2})([0-9]{2})$', $data)) { // removed preg_match("/^DTEND;VALUE=DATE/i", $field) ||
---
>             if ((preg_match("/^DTEND;VALUE=DATE/i", $field)) || (ereg ('^([0-9]{4})([0-9]{2})([0-9]{2})$', $data))) {
239a239,241
>                   elseif ($zulu_time) {
>                     $tz_dtend = 'GMT';
>                   }
244d245
<                 }
246,248d246
<                   if ($zulu_time) {
<                     $tz_dtend = 'GMT';
<                   }
272c270
<
---
>                 }
316c314
<     if ($tz == $zone['timezone'])
---
>     if ($tz == $zone['timezone']);

Comments

brush@groups.drupal.org’s picture

brush@groups.drupal.org commented

aish, it's late. this is, of course, in date_ical.inc

.b

karens’s picture

karens commented

Might be the right fix. Can you make this into a unified patch? It's hard to follow what you posted since you have inserted comments, etc.

brush@groups.drupal.org’s picture

brush@groups.drupal.org commented

sure! i've removed the comments, and decided to put the patch inline rather than attached.

really, near as i can tell the only question is whether there's any actual need for special behavior for "DTSTART;VALUE=DATE", and if so what. doesn't seem so to me.

haven't corrected indents in this patch.

.b

--- date_ical.inc.old   2007-06-22 03:10:24.000000000 -0700
+++ date_ical.inc       2007-07-03 04:17:17.000000000 -0700
@@ -173,7 +173,7 @@
                  $data = str_replace('T', '', $data);
                  $data = str_replace('Z', '', $data);
                  $field = str_replace(';VALUE=DATE-TIME', '', $field);
-                 if ((preg_match("/^DTSTART;VALUE=DATE/i", $field)) || (ereg ('^([0-9]{4})([0-9]{2})([0-9]{2})$', $data))) {
+                 if (ereg ('^([0-9]{4})([0-9]{2})([0-9]{2})$', $data)) {
                    ereg ('([0-9]{4})([0-9]{2})([0-9]{2})', $data, $dtstart_check);
                    $allday_start = $data;
                    $start_date = $allday_start;
@@ -185,15 +185,16 @@
                      $tz_dtstart = $tz_tmp[1];
                      unset($tz_tmp);
                    }
-                   elseif ($zulu_time) {
-                     $tz_dtstart = 'GMT';
-                   }
                    preg_match ('/([0-9]{4})([0-9]{2})([0-9]{2})([0-9]{0,2})([0-9]{0,2})/', $data, $regs);
                    $start_date = $regs[1] . $regs[2] . $regs[3];
                    $start_time = $regs[4] . $regs[5];
                    $start_unixtime = gmmktime($regs[4], $regs[5], 0, $regs[2], $regs[3], $regs[1]);
+                 }
               $dlst = date('I', $start_unixtime);
                    $server_offset_tmp = _date_ical_chooseOffset($start_unixtime, 'Same as Server');
+                   if ($zulu_time) {
+                     $tz_dtstart = 'GMT';
+                   }
                    if (isset($tz_dtstart)) {
                      if ($tz = _date_ical_tz($tz_dtstart)) {
                        $offset_tmp = date('I', $start_unixtime) ? _date_ical_calcString($tz->offset_dst)  : _date_ical_calcString($tz->offset);
@@ -217,7 +218,7 @@
                    $start_tz = $tz_dtstart;
                    $start_raw = $data;
                    unset($server_offset_tmp, $offset_tmp, $tz_dtstart);
-                 }
+
                  break;
           case 'DTEND':
                  $zulu_time = false;
@@ -225,7 +226,7 @@
                  $data = str_replace('T', '', $data);
                  $data = str_replace('Z', '', $data);
                  $field = str_replace(';VALUE=DATE-TIME', '', $field);
-            if ((preg_match("/^DTEND;VALUE=DATE/i", $field)) || (ereg ('^([0-9]{4})([0-9]{2})([0-9]{2})$', $data))) {
+            if (ereg ('^([0-9]{4})([0-9]{2})([0-9]{2})$', $data)) {
               $allday_end = $data;
               $end_date = $allday_end;
               $end_unixtime = strtotime($data);
@@ -236,14 +237,15 @@
                      $tz_dtend = $tz_tmp[1];
                      unset($tz_tmp);
                    }
-                   elseif ($zulu_time) {
-                     $tz_dtend = 'GMT';
-                   }
                    preg_match ('/([0-9]{4})([0-9]{2})([0-9]{2})([0-9]{0,2})([0-9]{0,2})/', $data, $regs);
                    $end_date = $regs[1] . $regs[2] . $regs[3];
                    $end_time = $regs[4] . $regs[5];
                    $end_unixtime = gmmktime($regs[4], $regs[5], 0, $regs[2], $regs[3], $regs[1]);
+                 }
                  $server_offset_tmp = _date_ical_chooseOffset($end_unixtime, 'Same as Server');
+                   if ($zulu_time) {
+                     $tz_dtend = 'GMT';
+                   }
                    if (isset($tz_dtend)) {
                      if ($tz = _date_ical_tz($tz_dtend)) {
                        $offset_tmp = date('I', $end_unixtime) ? _date_ical_calcString($tz->offset_dst)  : _date_ical_calcString($tz->offset);
@@ -267,7 +269,7 @@
                    $end_tz = $tz_dtend;
                    $end_raw = $data;
                    unset($server_offset_tmp, $offset_tmp, $tz_dtend);
-                 }
+
                  break;
           case 'DURATION':
                  if (!stristr($field, '=DURATION')) {
@@ -311,7 +313,7 @@
   include_once(drupal_get_path('module', 'date_api') .'/date.inc');
   include_once(DATE_TIMEZONES);
   foreach (date_get_timezones() as $delta => $zone) {
-    if ($tz == $zone['timezone']);
+    if ($tz == $zone['timezone'])
     return (object) $zone;
   }
 }
karens’s picture

karens commented

This code is from the event module and has been there for a long time, but was never functional, and therefore never tested. As I look at this and dig into the ical spec, I think you're right that this needs a rewrite to account for both DTSTART;VALUE=DATE and DTSTART;TZID=. And it also needs to be smarter about testing for and handling the 'Z' component. And I think the code could be made easier to track by combining the DTSTART and DTEND functions, which are doing exactly the same thing, just storing the results in different places.

I'm going to work on a rewrite of this code and try to clean it up.

ore’s picture

ore commented

Does this effect the non ability to load feeds from civevent1.8 even though it loads them ok from civevent1.7. the only difference i can see in the feeds is that the 1.7 has no line breaks and the 1.8 version does.

version 1.8 http://sandbox.civicrm.org/drupal/civicrm/event/ical?reset=1&page=1
version 1.7 http://demo.civicrm.org/drupal/civicrm/event/ical?reset=1&page=1

weird

ore

ore’s picture

ore commented

Just ignore me, I'm a typical dumb ass who has spent the last two hours wondering why it hasnt been working and then all of a sudden it worked! perhaps a mysterious caching thing ;-)

twowheeler’s picture

twowheeler commented
Priority: Critical » Normal

I am coming quite late to this party, but wanted to add one note. The patch allows the drupal calendar view to display the Start and End times correctly for an event. However, the red-ish block which indicates the current date on the monthly view still moves to the next date block four hours before midnight, which would be consistent with ignoring my timezone of -0400. I take it from this that some other part of the code controlling the red block on the current date still needs a patch.

alan426’s picture

alan426 commented

Is this patch compatible with the newest 1.8 version? I tried to apply it but it threw an error. Is Karen still working on cleaning up the TZIDs?

karens’s picture

karens commented
Status: Active » Fixed

All the iCal handling is reworked in the 5.2 version, which is nearly ready. In the new version, the iCal code just parses the ical to identify the right date and timezone without actually doing a timezone conversion. It was a mistake to convert it there because sometimes you need to convert the ical date to the site timezone, sometimes to the user timezone, and sometimes to GMT (when you're trying to import nodes from an iCal feed).

Issue #7 is unrelated to this thread and is a Calendar module issue which is already reported, and which also is being addressed with the 5.2 version of the date module.

There's no patch to apply to 5.1.8, this will all change in 5.2.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)

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