All-day events missing or wrong in ical feed

The attached patch includes my original changes and fixes the off-by-one problem with DTEND. I use the PHP DateTime::modify() function, which was introduced in PHP 5.2, so this should work in Calendar 7.x-3.x.

In my testing, the patches work: I generate an ical feed that validates on http://icalvalid.cloudapp.net/ and imports into Apple iCal with the expected results.

Here are some questions to consider.

1. I separately decide whether DTSTART and DTEND should be treated as all-day (DATE, not DATETIME) values. This is different from the behavior in the 6.x branch, where both are given the same format. Which behavior is right?
2. I pass boolean variables to the template file, simplifying the tests there compared to #1038218: All Day events not RFC 2445/3339/5545 compliant. If you like this method, then we could do the same in the 6.x branch. This would also require changes to the 6.x branch of the Date module, #1282538: ical feed generates wrong DTEND value for multi-day, all-day events.
3. It may be easy to simplify the code, if you are more familiar with the dateObject API than I am. Currently, the sequence of steps is
   

       $start = new dateObject($item['value'], 'UTC');
       $start->setTimezone(timezone_open($item['timezone']));
       $start_allday = ($start->format('His') == '000000');
       $start->setTimezone(timezone_open('UTC'));
   

   Is there a safe way to combine the first two lines? There are a few cases, with different versions of the first line, so changing this would make the patch more complex.

4. In the 6.x branch, the preprocess function is in the Date (or Date API) module, not the Calendar module. The function template_preprocess_date_vcalendar() is still in the D7 version of the Date module, but is not used here. Is that function ever used? It looks like duplicated code to me. Either it should be removed from the Date API module, or it should get a similar patch. If it is not removed, perhaps the render() function in calendar_plugin_row_ical_node.inc should call it instead of duplicating its functionality.
5. In the 6.x branch, the decision is made before the preprocess function whether to treat an event as all-day. Are there plans to do the same in the 7.x branch?

I will also update the issue summary.

Answering some of my own questions ...

1. Both Google Calendar and Apple iCal have a single flag for each calendar entry. That is, either both the start time and end time are treated as DATE values or both are treated as DATETIME values. Since the D6 version behaves similarly, I think this is the right way to do it. My next patch will follow this approach. I will have to test what happens when the event node has a start time and no end time.

5. In the 6.x branch, the function calendar_build_nodes() in modules/calendar/calendar.module marks some nodes (or pseudo-nodes) as all-day events. I think this is intended to be used when displaying events in the calendar format. I cannot find any similar code in the 7.x branch, so I assume it was intentionally removed. I think it makes sense to have the ical feed "decide for itself" whether to treat an event as all-day, rather than using the decision made for the calendar display.

This version calls an event all-day if both the start and end times are 00:00:00. See Comment #2.

If the node has a start time but no end time, then the existing code does, in effect,
$end = $start; Since my patch adds $end->modify("+1 day"); for all-day events, I changed that to $end = clone $start;

I think this patch is ready to go.

Nick_vh, thanks for taking the time to check this out. Let me try to settle the format (line ending) issues first.

Background

I will add some of this to the issue summary once we get this sorted out.

RFC 2445 specifies that an ical file has to have DOS-style line endings. As a result, the template files affected by this patch (calendar/calendar_ical/calendar-row-ical-node.tpl.php and calendar/calendar_ical/calendar-style-ical.tpl.php) have to have DOS-style line endings, and you will see a lot of lines like print $foo . "\r\n"; in the templates. Naturally, we have to be careful about line endings when we make and apply patches. My version of the patch has some DOS-style line endings, and I think that is correct.

What I think went wrong

I have tried downloading a fresh copy of the Calendar module using git, downloading the tarball, and downloading the zip file. I even tried using Chrome, wget, and Safari to download the archive files. (Safari expands the archives automatically.) I have used both Chrome and wget to fetch the patch files. I have tried using patch --verbose and git apply -v. With all these variations, I get the same result: my patch from #3 works, and yours from #5 does not work. (The errors I get are similar to the ones you posted in #4.)

I think that you somehow ended up with template files that have Unix-style line endings. Can you check this? Either way, what method did you use to download the module and the patch?

Nick_vh:

I am still curious about whether your template files have UNIX or DOS style line endings.

I tried Googling for git dos line endings and found this page: http://help.github.com/line-endings/ . If I try
$ git config --global --get core.autocrlf
then I get an empty response. If I follow the advice (for Mac/Linux users) from github,

$ git config --global core.autocrlf input
$ git config --global --get core.autocrlf
input

If I then try the sequence you posted in #8, I get the same errors you do.

(Editing my comment)
At first, I thought that the core.autocrlf setting affected how git clone works. After further testing, I realize that it changes how git apply works.

I expect that the patch in #3 will apply cleanly if you use patch instead of git.

I am now sure that the culprit is the core.autocrlf. The question is whether it should be set my way or your way. According to this comment in Drupal's Git documentation, your way is recommended. So I tried it that way, setting autocrlf to input (and trying with and without core.safecrlf set to true). The patch from #5 applied cleanly, but when I downloaded calendar.ics from my test site, it failed validation at http://icalvalid.cloudapp.net/ because of CRLF issues. (The validator classifies this as a warning, not an error.)

Conclusion: you should leave core.autocrlf unset (the default).

First of all, changing the documentation is outside the scope of this issue. I think that #1046962: Provide Git core.*crlf settings recommendations based on OS sniffing would be a good place to continue this discussion. (I already added a short comment there.)

Second of all, it is only a comment in the Git instructions that recommends setting core.autocrlf. So I would not call it changing the documentation.

A quick look at the man page for git suggests that we could use something like
$ git -c core.autocrlf=??? apply -v foo.patch
I am not sure what the syntax is to simulate unsetting the config variable. It might be better to use a per-project configuration file, .git/config. Perhaps the safest thing is to rewrite the template files with more print "\r\n"; statements, so that the file itself has UNIX-style line endings.

Again, all of this is beyond the scope of this issue. If I have time tomorrow, I will have a look at the "Git instructions" project. I think that is a better place for this discussion.

helmo:

I agree. That is what I meant by "Perhaps the safest thing" in #13.

After studying the git man pages, I think I have found a much better solution. Add the file calendar_ical/.gitattributes with the single line

*.tpl.php eol=crlf

This tells git not to use the config variable core.autocrlf for these files.

I like this solution enough that I have opened a separate issue for it: #1302052: CRLF line endings cause problems with "git apply".

@helmo, I would appreciate your comments on this approach.

I will edit the issue summary to mention the CRLF issue.

This version of the patch is the same as the one in #3 except that it removes (or avoids adding) trailing white space on some lines.

I quoted the correct lines from a sample .ics file.
I updated the "Remaining tasks" section now that there is an improved patch in Comment #1.

After further testing, I have decided that the one line in #16 should be

*.tpl.php -text

I added a summary of Comments 4-16, dealing with CRLF issues.

I will try to get this done today or tomorrow.

The attached patch is an update of #17. I have checked that the output validates on http://icalvalid.cloudapp.net/, and I have imported it into Google Calendar to make sure that all-day events are treated correctly.

Note the comment I added in calendar_ical/calendar_plugin_row_ical_node.inc:

// All-day events should be treated differently. Treat an event as all-day
// if, in the site's timezone, its start and end times are '00:00:00'

If there is a better way to decide which events are all-day, I do not know the code well enough to find it.

While updating the patch from #17, I noticed that it also fixes #1390766: Commas should be escaped.. I will mark that issue as a duplicate.

The code in the D6 branch is very different. I suggest that, instead of adding the patch to this issue as a back-port, we update the patches in the issues mentioned in the issue summary above.

I do not have time now to look at the differences between #21 and what you committed.

Please have a look at the D6 versions of this patch (Date and Calendar modules): #1038218: All Day events not RFC 2445/3339/5545 compliant and #1282538: ical feed generates wrong DTEND value for multi-day, all-day events. I do not (yet) use the D7 versions of Data and Calendar. The main reason I contributed the patches on this and related threads was in the hope of getting the D6 patches added as back-ports.

@KarenS, the D6 versions already have their own patches. The one for Calendar is RTBC. The patch for Date was RTBC, but I had to rewrite it after @arlinsandbulte committed my patch for #1302052: CRLF line endings cause problems with "git apply". I tested it pretty thoroughly.

I apologize if it is not appropriate to call the existing D6 patches ports of this one.

I spent a few hours today testing the changes you committed in #23. The ical feeds validate 100% at http://icalvalid.cloudapp.net/. All-day events show up correctly when I import them into a calendar application (Apple's iCal). As far as this issue is concerned, I think the recent commit works well.

There are still issues with time zones. My site's time zone is America/New_York (GMT -0500) and I configured my Date type to use the site's time zone. I entered an event as Feb 21 from 20:00 to 22:30. It shows up correctly on the node page and on the calendar page, but the ical feed makes it 20:00 to 22:30 GMT, so it shows up in my calendar as 15:00 to 17:30.

When I entered an event with a start time of 00:00 and left the end time empty, but did not mark it as all-day, it was still saved as an all-day event. This is not what I expected, but I think this is a documentation issue. The ical feed is consistent with the node page, so it is not a problem with this issue.

Perhaps these comments belong to a different issue. I am not sure what the correct procedure is when testing comes after committing.

I compared my patch from #21 with what you committed, and the main differences seem to be how you handle time zones and the use of date_is_all_day().

Minor point: it seems odd to use !empty() on a boolean on line 34 of calendar-row-ical-node.tpl.php, especially since you test the same boolean directly on line 28:

if ($event['all_day']):    // line 28
  print "DTSTART;VALUE=DATE:" . $event['start'] . "\r\n";
else:
  print "DTSTART:" . $event['start'] . "Z\r\n";
endif;
if (!empty($event['end'])):
  if (!empty($event['all_day'])):    // line 34

I listed the D6 versions of the patch in the issue summary, under "Related Issues": #1038218: All Day events not RFC 2445/3339/5545 compliant and #1282538: ical feed generates wrong DTEND value for multi-day, all-day events.

update advice on avoiding git problems