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
Comment | File | Size | Author |
---|---|---|---|
#22 | calendar-ical-1302052-22.patch | 4.22 KB | benjifisher |
#22 | date-ical-1302052-22.patch | 5.86 KB | benjifisher |
#20 | calendar-ical-git-1302052-20.patch | 2.31 KB | benjifisher |
#19 | calendar-ical-git-1302052-19.patch | 1.48 KB | benjifisher |
#8 | calendar-ical-git-1302052-8.patch | 4.37 KB | benjifisher |
Comments
Comment #1
helmo CreditAttribution: helmo commentedNice 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.
Comment #2
benjifisherhelmo, thanks for checking. Sorry, I should have tested more carefully before posting.
I did not have any trouble when using
patch
instead ofgit 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:
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.
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.)I will update the issue summary.
Comment #3
helmo CreditAttribution: helmo commentedI 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.
Comment #4
benjifisher@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.
Comment #5
helmo CreditAttribution: helmo commentedSorry 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.
as opposed to
Comment #6
KarenS CreditAttribution: KarenS commentedWe 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'.
Comment #7
helmo CreditAttribution: helmo commented@KarenS: I can understand your hesitation to change this.
It was the patch in #2 that needed review.
Comment #8
benjifisher@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:
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.
Comment #9
helmo CreditAttribution: helmo commentedThe 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.
Comment #10
benjifisher@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.
Comment #11
KarenS CreditAttribution: KarenS commentedSee related issue at #1018782: Unix Line endings. Every time I change line endings someone else reports that as a problem.
Comment #12
benjifisher@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
Comment #13
helmo CreditAttribution: helmo commentedOne thing we could also borrow from the patch in #1018782: Unix Line endings is the extra comment referencing the RFC.
Looks better then what's in the patch from #8
Comment #14
KarenS CreditAttribution: KarenS commentedOK, 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).
Comment #15
KarenS CreditAttribution: KarenS commentedI 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.
Comment #16
helmo CreditAttribution: helmo commentedGreat,
The METHOD line must indeed have been an oversight.
Happy new Year
Comment #17
benjifisherHappy 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 have not checked: maybe the METHOD line was fixed since I made the patch.
Comment #18
KarenS CreditAttribution: KarenS commentedI 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.
Comment #19
benjifisherI 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.
Comment #20
benjifisherI 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 oftheme('date_event')
).Note that this patch is for the Date module.
Comment #21
KarenS CreditAttribution: KarenS commentedThanks! 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.
Comment #22
benjifisherHere 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. ;)
Comment #23
arlinsandbulte CreditAttribution: arlinsandbulte commentedOK, 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!
Comment #24
benjifisherI marked #1109324: ical feed not parsing due to inconsistent line endings (D6 branch) as a duplicate of this issue.
Comment #25.0
(not verified) CreditAttribution: commentedUse -text instead of eol=crlf in .gitattributes