Problem/Motivation

The template files (.tpl.php) that generate the ical feed have DOS-style (CRLF) line endings. This is a good thing. Patches that affect these files can cause serious headaches if you use git apply. This is a bad thing.

Specifically, if you follow this advice and do

$ git config --global core.autocrlf input

on a Mac or Linux machine, then

$ git clone --branch 7.x-3.x http://git.drupal.org/project/calendar.git
$ cd calendar
$ wget http://drupal.org/files/issues/calendar-ical-1284170-3.patch
$ git apply -v calendar-ical-1284170-3.patch

will fail. See #1284170-8: All-day events missing or wrong in ical feed.

Proposed resolution

After studying the git man pages and testing, I found this solution. Tell git to ignore line endings on the template files by adding the file calendar_ical/.gitattributes with the single line

*.tpl.php -text

This will take precedence over the git config settings for core.autocrlf. The patch in #2 adds this one-line file.

Remaining tasks

I have only tested this on Mac and Linux machines. I have not tried it on a Windows machine.

User interface changes

N/A

API changes

N/A

CommentFileSizeAuthor
#22 calendar-ical-1302052-22.patch4.22 KBbenjifisher
#22 date-ical-1302052-22.patch5.86 KBbenjifisher
#20 calendar-ical-git-1302052-20.patch2.31 KBbenjifisher
#19 calendar-ical-git-1302052-19.patch1.48 KBbenjifisher
#8 calendar-ical-git-1302052-8.patch4.37 KBbenjifisher
#2 calendar-ical-git.1302052.2.patch197 bytesbenjifisher
calendar-ical-git.patch200 bytesbenjifisher
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

helmo’s picture

helmo CreditAttribution: helmo commented

Nice find, .gitattributes, but git apply was still complaining about white space (both with .gitattributes and without)

Your "safest thing" from http://drupal.org/node/1284170#comment-5081062 still seems like the best option, then also people working outside of git are helped.
It might me nice to define a mini function e.g. print_rn() to print with "\r\n" appended to the string.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
FileSize
calendar-ical-git.1302052.2.patch197 bytes

helmo, thanks for checking. Sorry, I should have tested more carefully before posting.

I did not have any trouble when using patch instead of git apply. I think this is a git solution to a git problem. Are there other tools we need to worry about?

The attached patch uses the following line in .gitattributes:

*.tpl.php -text

This tells git that the template files are "binary" (not text) so it should not even try to be clever with line endings. I do not know why it is not good enough to tell git to use CRLF line endings. :-(

Here is a successful test on my Ubuntu server. Note the version number of git.

$ git --version
git version 1.7.7
$ git config --global --get core.autocrlf
input
$ git clone --branch 7.x-3.x http://git.drupal.org/project/calendar.git
Cloning into calendar...
remote: Counting objects: 6298, done.
remote: Compressing objects: 100% (2470/2470), done.
remote: Total 6298 (delta 4561), reused 5326 (delta 3785)
Receiving objects: 100% (6298/6298), 2.18 MiB | 1.37 MiB/s, done.
Resolving deltas: 100% (4561/4561), done.
$ cd calendar
$ wget http://drupal.org/files/calendar-ical-git.1302052.2.patch
[snip]
2011-10-07 11:12:31 (10.4 MB/s) - `calendar-ical-git.1302052.2.patch' saved [197/197]

$ wget http://drupal.org/files/calendar-ical-1284170-17.patch
[snip]
2011-10-07 11:13:03 (62.4 KB/s) - `calendar-ical-1284170-17.patch' saved [5787/5787]

$ git apply -v calendar-ical-git.1302052.2.patch 
Checking patch calendar_ical/.gitattributes...
Applied patch calendar_ical/.gitattributes cleanly.
$ git status
# On branch 7.x-3.x
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#	calendar-ical-1284170-17.patch
#	calendar-ical-git.1302052.2.patch
#	calendar_ical/.gitattributes
nothing added to commit but untracked files present (use "git add" to track)
$ git apply -v calendar-ical-1284170-17.patch 
Checking patch calendar_ical/calendar-row-ical-node.tpl.php...
Checking patch calendar_ical/calendar-style-ical.tpl.php...
Checking patch calendar_ical/calendar_plugin_row_ical_node.inc...
Checking patch calendar_ical/calendar_plugin_style_ical.inc...
Checking patch calendar_ical/theme.inc...
Applied patch calendar_ical/calendar-row-ical-node.tpl.php cleanly.
Applied patch calendar_ical/calendar-style-ical.tpl.php cleanly.
Applied patch calendar_ical/calendar_plugin_row_ical_node.inc cleanly.
Applied patch calendar_ical/calendar_plugin_style_ical.inc cleanly.
Applied patch calendar_ical/theme.inc cleanly.
$

I got similar results on my Mac using git 1.7.3.5, but this is what happens if I use the version of git that comes with Ubuntu 10.04. Note the version number and the response to git status. (This time I tell wget to be quiet so that I do not have to edit the session.)

$ git --version
git version 1.7.0.4
$ git config --global --get core.autocrlfinput
$ git clone --branch 7.x-3.x http://git.drupal.org/project/calendar.git
Initialized empty Git repository in /tmp/calendar/.git/
remote: Counting objects: 6298, done.
remote: Compressing objects: 100% (2470/2470), done.
remote: Total 6298 (delta 4557), reused 5329 (delta 3785)
Receiving objects: 100% (6298/6298), 2.18 MiB | 1.41 MiB/s, done.
Resolving deltas: 100% (4557/4557), done.
$ cd calendar
$ wget --quiet http://drupal.org/files/calendar-ical-git.1302052.2.patch
$ wget --quiet http://drupal.org/files/calendar-ical-1284170-17.patch
$ git apply -v calendar-ical-git.1302052.2.patch Checking patch calendar_ical/.gitattributes...
Applied patch calendar_ical/.gitattributes cleanly.
$ git status# On branch 7.x-3.x
# Changed but not updated:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   calendar_ical/calendar-row-ical-node.tpl.php
#	modified:   calendar_ical/calendar-style-ical.tpl.php
#
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#	calendar-ical-1284170-17.patch
#	calendar-ical-git.1302052.2.patch
#	calendar_ical/.gitattributes
no changes added to commit (use "git add" and/or "git commit -a")
$ git apply -v calendar-ical-1284170-17.patch Checking patch calendar_ical/calendar-row-ical-node.tpl.php...
error: while searching for:
 * 
 *   $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.

error: patch failed: calendar_ical/calendar-row-ical-node.tpl.php:3
error: calendar_ical/calendar-row-ical-node.tpl.php: patch does not apply
Checking patch calendar_ical/calendar-style-ical.tpl.php...
error: while searching for:
?>
BEGIN:VCALENDAR
VERSION:2.0
METHOD:<?php print $method; ?>
<?php if (!empty($calname)): ?>
X-WR-CALNAME;VALUE=TEXT:<?php print $calname . "\r\n"; ?>
<?php endif; ?>
PRODID:-//Drupal iCal API//EN
<?php print $rows ?>

error: patch failed: calendar_ical/calendar-style-ical.tpl.php:9
error: calendar_ical/calendar-style-ical.tpl.php: patch does not apply
Checking patch calendar_ical/calendar_plugin_row_ical_node.inc...
Checking patch calendar_ical/calendar_plugin_style_ical.inc...
Checking patch calendar_ical/theme.inc...
$

I will update the issue summary.

helmo’s picture

helmo CreditAttribution: helmo commented

I just tried it with git version 1.7.4.1, and also failed. See below.
The in #1284170: All-day events missing or wrong in ical feed suggested patch -p1 < /path/to/calendar-ical-1284170-17.patch does apply without objections.

This dependency on a git version seems like a risk to add to this project.
What's your objection against your "safest thing" from http://drupal.org/node/1284170#comment-5081062 ?
That would also help when git is not being used.

$ git apply -v calendar-ical-1284170-17.patch 
calendar-ical-1284170-17.patch:9: trailing whitespace.
 *   $event['all_day'] - TRUE for all-day event.
calendar-ical-1284170-17.patch:17: trailing whitespace.
<?php if ($event['all_day']): ?>
calendar-ical-1284170-17.patch:18: trailing whitespace.
DTSTART;VALUE=DATE:<?php print($event['start'] . "\r\n") ?>
calendar-ical-1284170-17.patch:19: trailing whitespace.
<?php else: ?>
calendar-ical-1284170-17.patch:21: trailing whitespace.
<?php endif; ?>
Checking patch calendar_ical/calendar-row-ical-node.tpl.php...
Checking patch calendar_ical/calendar-style-ical.tpl.php...
Checking patch calendar_ical/calendar_plugin_row_ical_node.inc...
Checking patch calendar_ical/calendar_plugin_style_ical.inc...
Checking patch calendar_ical/theme.inc...
Applied patch calendar_ical/calendar-row-ical-node.tpl.php cleanly.
Applied patch calendar_ical/calendar-style-ical.tpl.php cleanly.
Applied patch calendar_ical/calendar_plugin_row_ical_node.inc cleanly.
Applied patch calendar_ical/calendar_plugin_style_ical.inc cleanly.
Applied patch calendar_ical/theme.inc cleanly.
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented

@helmo,

Your listing does not show that you applied the patch from this issue before trying
$ git apply -v calendar-ical-1284170-17.patch
If you did not apply this patch, or add .gitattributes yourself, then I would expect it to fail.

The "safest thing" makes the file hard to read, thus harder to maintain. It seems to me that this is a git problem, so there should be a git solution. But maybe it is time to give up.

helmo’s picture

helmo CreditAttribution: helmo commented

Sorry for leaving that off, but I do actually have the .gitattributes file in place.

I had a go at converting one of the tpl files, I'd argue that it is even better readable this way.

<?php
/**
 * $title
 *   The name of the calendar.
 *
 *   NOTE: Lines should end in \r\n, 
 */
if (empty($method)) {
  $method = 'PUBLISH';
}

print "BEGIN:VCALENDAR\r\n"; 
print "VERSION:2.0\r\n"; 
print "METHOD:$method\r\n"; 

if (!empty($title)) {
  print "X-WR-CALNAME;VALUE=TEXT:$title\r\n";
}
print "PRODID:-//Drupal iCal API//EN\r\n";
print "$rows\r\n";
print "END:VCALENDAR\r\n";

?>

as opposed to

<?php
/**
 * $title
 *   The name of the calendar.
 */
if (empty($method)) {
  $method = 'PUBLISH';
}
?>
BEGIN:VCALENDAR
VERSION:2.0
METHOD:<?php print $method . "\r\n"; ?>
<?php if (!empty($title)): ?>
X-WR-CALNAME;VALUE=TEXT:<?php print $title . "\r\n"; ?>
<?php endif; ?>
PRODID:-//Drupal iCal API//EN
<?php print $rows ?>
END:VCALENDAR

?>
KarenS’s picture

KarenS CreditAttribution: KarenS commented
Status: Needs review » Needs work

We have gone back and forth on the line endings in this file for years now. Every fix fixes one set of problems and creates others (i.e. the file that is created won't work in certain systems if you try to import it).

I'm pretty much afraid to mess with it any more, at least not without extensive testing that it not only fixes this problem but still creates a file that works successfully across different systems when imported into Google calendar, Outlook, etc.

It's also not clear what patch is 'ready for review'.

helmo’s picture

helmo CreditAttribution: helmo commented

@KarenS: I can understand your hesitation to change this.
It was the patch in #2 that needed review.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
Status: Needs work » Needs review
FileSize
calendar-ical-git-1302052-8.patch4.37 KB

@KarenS, @helmo: I agree that we have to be careful with these files. In fact, that is what this issue is about! I also understand that the output files have to have DOS line endings, as I wrote in the issue summary.

As I said in #4, maybe it is time to give up on finding a git solution and take @helmo's approach. I think @helmo is right: the result is quite readable. The attached patch uses @helmo's strategy for both of the template files. There are a few differences from the suggestion in #5:

I re-introduced the bugs because they should be addressed in a separate issue and because I want to be able to compare the .ics files I get before and after applying the patch. My results show that the only thing that changed is the time stamp:

macpro:calendar benji$ git reset --hard
HEAD is now at 6cfed73 Fix merge conflict
macpro:calendar benji$ wget http://localhost:8888/dgd7/drupal/calendar/ical/clone/calendar.ics -O before.ics -q
macpro:calendar benji$ git apply -v calendar-ical-git-1302052-8.patch 
Checking patch calendar_ical/calendar-row-ical-node.tpl.php...
Checking patch calendar_ical/calendar-style-ical.tpl.php...
Applied patch calendar_ical/calendar-row-ical-node.tpl.php cleanly.
Applied patch calendar_ical/calendar-style-ical.tpl.php cleanly.
macpro:calendar benji$ wget http://localhost:8888/dgd7/drupal/calendar/ical/clone/calendar.ics -O after.ics -q
macpro:calendar benji$ diff before.ics after.ics 
7c7
< DTSTAMP:20111115T191037Z
---
> DTSTAMP:20111115T191107Z
16c16
< DTSTAMP:20111115T191037Z
---
> DTSTAMP:20111115T191107Z
25c25
< DTSTAMP:20111115T191037Z
---
> DTSTAMP:20111115T191107Z

I did this experiment on a Mac using apache. I expect to get the same results on other systems. I can test with Ubuntu/apache and Windows/apache (XAMPP) if you like, but not on Windows/IIS.

helmo’s picture

helmo CreditAttribution: helmo commented 
--- a/calendar_ical/calendar-row-ical-node.tpl.php
+++ b/calendar_ical/calendar-row-ical-node.tpl.php

@@ -1,18 +1,21 @@
+print "$rows";

The double quotes are not needed here, but that a minor issue. It's good though that you didn't include a \r\n for this line as calendar-row-ical-node.tpl.php already adds these.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented

@helmo:

Right. Including \r\n would introduce a harmless blank line in the .ics file, but that would have made my diff less clean in #8 above.

KarenS’s picture

KarenS CreditAttribution: KarenS commented

See related issue at #1018782: Unix Line endings. Every time I change line endings someone else reports that as a problem.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented

@KarenS,

Thanks for pointing out the related issue. I completely agree with @penguin25's comments there. I will add a comment to that issue linking to this one.

The only differences between @penguin25's proposed patch and the one here are stylistic. Following @helmo's suggestion in comment #5, my patch in #8 uses a single opening PHP tag at the start of the file, then explicit print statements on almost every line. If you prefer, I can rewrite it to be a theme function rather than a template file, since that is pretty much the same thing. The patch proposed by @penguin25 accomplishes the same end by adding a bunch of <?php print "\r\n"; ?> statements at ends of lines. I am also happy to rewrite the patch here in @pengiun25's style.

Either way, the goal is to have

  1. UNIX line endings in our files, both to conform to Drupal coding standards and to avoid practical problems with applying patches;
  2. DOS line endings in the ical feeds output by the server.
helmo’s picture

helmo CreditAttribution: helmo commented
Status: Needs review » Needs work

One thing we could also borrow from the patch in #1018782: Unix Line endings is the extra comment referencing the RFC.

+++ date-6.x-2.x-dev-2011_11_27-with_ical_fix/theme/date-vevent.tpl.php	2011-12-21 10:45:51.000000000 +0000
@@ -14,9 +14,13 @@
+ * If you are editing this file, remember that all output lines generated by it
+ * must end with DOS-style \r\n line endings, and not Unix-style \n, in order to
+ * be compliant with the iCal spec (see http://tools.ietf.org/html/rfc5545#section-3.1)

Looks better then what's in the patch from #8 

+++ b/calendar_ical/calendar-style-ical.tpl.php
@@ -1,18 +1,21 @@
+ *   NOTE: Output lines should end in "\r\n".
KarenS’s picture

KarenS CreditAttribution: KarenS commented

OK, the style in this patch is much cleaner, so I think I prefer this one. I agree we want the better documentation. I don't see a line break after METHOD:$method, is that an oversight or did you have a reason for that? I assume it was an oversight. We should also add a comment above $rows about why that one line does not have line breaks (because they got added in the row template).

KarenS’s picture

KarenS CreditAttribution: KarenS commented
Version: 7.x-3.x-dev » 6.x-2.x-dev
Status: Needs work » Patch (to be ported)

I couldn't get the patch to apply anyway, for the reasons noted above, so I did it manually and added in the additional documentation. There are similar files in Date API, so I made the same changes there.

http://drupalcode.org/project/calendar.git/commit/fd9815b
http://drupalcode.org/project/date.git/commit/bd2cacf

All of these changes should be back-ported to the D6 version.

helmo’s picture

helmo CreditAttribution: helmo commented

Great,

The METHOD line must indeed have been an oversight.

Happy new Year

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented

Happy new year indeed! I am glad we all agree on this issue. For the last few months, I have been saying that I am skeptical that it is really the 21'st century, since CRLF issues should have been fixed by now. I think 2012 will be a little better.

I will have a look at the changes that were committed, and I will do the back-port unless someone beats me to it.

The METHOD line was not an oversight. As I wrote in #8 above,

I re-introduced the bugs because they should be addressed in a separate issue and because I want to be able to compare the .ics files I get before and after applying the patch.

I have not checked: maybe the METHOD line was fixed since I made the patch.

KarenS’s picture

KarenS CreditAttribution: KarenS commented

I want to take care of all issues related to line endings here. If some line endings were missing, fix them here :) I don't want to mix in any other issues though.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
FileSize
calendar-ical-git-1302052-19.patch1.48 KB

I got the latest 7.x-3.x from git.

I tried validating a calendar feed at http://icalvalid.cloudapp.net. I got an unrelated error, for which I have opened a new issue: #1390766: Commas should be escaped.. There are no problems with line endings.

Comparing with the version from the patch in #8 above, I found some typos have crept into the comments. The attached patch corrects these. I re-validated, and got the same unrelated error.

More to come: I still have to look at the corresponding changes to the Date module, and I still owe you a port to 6.x.

Note the attached patch is for the 7.x version.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
FileSize
calendar-ical-git-1302052-20.patch2.31 KB

I looked at the patch for the Date module, 7.x-2.x branch.

I did not test this! If you can tell me where these template files get output, I will validate them. AFAICT, they are not used in generating the calendar feed (ics file).

I found one line that seems to be wrong in date-valarm.tpl.php. The attached patch replaces "\n" with "\r\n".

I took the liberty of making some other changes. I removed the CVS // $Id$ lines. I made sure all files end in newlines. (This is a Drupal coding standard. It does not seem to affect the ical feeds. My development environment runs Drupal Code Sniffer automatically, so all of these are flagged as errors.) I changed one comment to match the following line of code (theme('date_vevent') instead of theme('date_event')).

Note that this patch is for the Date module.

KarenS’s picture

KarenS CreditAttribution: KarenS commented

Thanks! The files in Date API are the ones I wanted to use but I couldn't find any way to get the Calendar Views plugin to use a theme from another module. Ideally, they would exist only in the Date API (or maybe Date Views) since they are generic files that go with the Date API ical functionality, not something specific to Calendar. I finally gave up trying to find a way to use the same files in more than one place and ended up with duplicates.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
Status: Patch (to be ported) » Needs review
FileSize
date-ical-1302052-22.patch5.86 KB
calendar-ical-1302052-22.patch4.22 KB

Here are the ports to the D6 branches: one patch for the Calendar module, one for Date.

After these are committed, I will help clean up the issues queue. IIRC, there are some duplicate issues out there.

Note to self: if a file has mysteriously been updated, check whether it is being tracked by git. ;)

arlinsandbulte’s picture

arlinsandbulte CreditAttribution: arlinsandbulte commented
Status: Needs review » Fixed

OK, I committed both patches above from #22:
Calendar: http://drupalcode.org/project/calendar.git/commit/42eea9c
Date: http://drupalcode.org/project/date.git/commit/1681542

I also created a related date issue here: #1391374: CRLF line endings cause problems with "git apply", just for posterity's sake.

Thanks benjifisher!

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented

I marked #1109324: ical feed not parsing due to inconsistent line endings (D6 branch) as a duplicate of this issue.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Use -text instead of eol=crlf in .gitattributes