Problem/Motivation
In an ical feed, all-day events (DATE format, not DATETIME) need to be treated differently. Instead of
DTSTART:20110919T233000Z
DTEND:20110920T010000Z
they should be
DTSTART;VALUE=DATE:20110923
DTEND;VALUE=DATE:20110926
Furthermore, according to the ical spec (RFC 2445, clarified in RFC5545) the DTEND value is non-inclusive. For DATE values, this means we should increase it by 1. This will avoid problems when importing Drupal-generated ical feeds into other calendar programs. (So far, tested with Google Calendar and Apple iCal.)
Proposed resolution
The patch in #17 modifies the template file and associated preprocess functions to give correctly formatted ical feeds.
Warning when using git apply
If you apply the patch using git apply
then it may fail because of line-ending issues, depending on your git configuration. This is discussed in #4–#18 below, and the solution given in #18 is repeated in #1302052: CRLF line endings cause problems with "git apply". You can also use patch
instead of git apply
. You may still have problems with some versions of git: I find that 1.7.0.4 does not work, but 1.7.3.5 does. An alternative to adding the .gitattributes file is to use
$ git -c core.autocrlf=false apply -v /tmp/calendar-ical-1284170-17.patch
but the -c
option is not supported in git 1.7.0.4. I have had no problems with
$ patch -p1 < /path/to/calendar-ical-1284170-17.patch
Remaining tasks
See Comment #1. Further testing is needed. Check further changes with http://icalvalid.cloudapp.net/ .
User interface changes
N/A
API changes
N/A
Related issues
This is a port to D7 of the issues raised and patches produced for the D6 branch: #1038218: All Day events not RFC 2445/3339/5545 compliant and #1282538: ical feed generates wrong DTEND value for multi-day, all-day events. Credit goes to hirbys, crifi, and benjifisher.
Minor fixes
The attached patch also fixes some minor bugs:
- The preprocess function now passes
$title
to the template file, but the template still uses the old variable,$calname
. - The METHOD line in the template file needs to end with
"\r\n"
. - The node title should be escaped with
date_ical_escape_text()
.
Comment | File | Size | Author |
---|---|---|---|
#22 | calendar-ical-1284170-22.patch | 8.44 KB | CandC540 |
#21 | calendar-ical-1284170-21.patch | 6.9 KB | benjifisher |
#17 | calendar-ical-1284170-17.patch | 5.65 KB | benjifisher |
#6 | calendar (3).ics_.jpg | 42.28 KB | Nick_vh |
#5 | calendar-ical.patch | 5.25 KB | Nick_vh |
Comments
Comment #1
benjifisherThe 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.
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.
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 therender()
function in calendar_plugin_row_ical_node.inc should call it instead of duplicating its functionality.I will also update the issue summary.
Comment #2
benjifisherAnswering 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()
inmodules/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.Comment #3
benjifisherThis 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.
Comment #4
Nick_vhApplying this patch gives me the following errors :
Could you recheck if the format is right? I'll see if I can improve it
(following up on your request in the drupal boston user group)
Comment #5
Nick_vhReformatted your patch, applying properly now - I'll review the functionality in a second :-)
Comment #6
Nick_vhInitial testing did not turn out as expected. I was expecting a format as you described above
But instead I am getting the following
Comment #7
benjifisherNick_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
andcalendar/calendar_ical/calendar-style-ical.tpl.php
) have to have DOS-style line endings, and you will see a lot of lines likeprint $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
andgit 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?
Comment #8
Nick_vhbenjifisher,
I tried the workflow again by only using command line
However this did not succeed (as I mentioned before). I would assume this is probably a setting of git that has some strict checking on line-endings? Let's wait and see if someone else can try and apply your patch. I'd be very happy to see this working
Comment #9
benjifisherNick_vh:
I am still curious about whether your template files have UNIX or DOS style line endings.
Comment #10
benjifisherI 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,
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 howgit clone
works. After further testing, I realize that it changes howgit apply
works.I expect that the patch in #3 will apply cleanly if you use patch instead of git.
Comment #11
benjifisherI 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).Comment #12
Nick_vhSo we should change all documentation? Would there be some override argument during the patching (leaving the recommended autocrf on)? (no time to look right now)
Comment #13
benjifisherFirst 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.
Comment #14
helmo CreditAttribution: helmo commentedCouldn't the two tpl.php files be adapted to explicitly print the "\r\n" for every line as is already done on a number of lines?
The two tpl.php template files in calendar_ical are the exception as they need to generate output where the lines end in \r\n.
Having two line ending standards in the same project is going to cause more headaches. I see no sane way to advise git settings for that,
Correct me if I'm wrong here...
Comment #15
benjifisherhelmo:
I agree. That is what I meant by "Perhaps the safest thing" in #13.
Comment #16
benjifisherAfter studying the git man pages, I think I have found a much better solution. Add the file
calendar_ical/.gitattributes
with the single lineThis 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.
Comment #17
benjifisherThis 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.
Comment #17.0
benjifisherI 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.
Comment #18
benjifisherAfter further testing, I have decided that the one line in #16 should be
Comment #18.0
benjifisherI added a summary of Comments 4-16, dealing with CRLF issues.
Comment #19
KarenS CreditAttribution: KarenS commentedThis patch needs to be updated now that #1302052: CRLF line endings cause problems with "git apply" has gone in.
Comment #20
benjifisherI will try to get this done today or tomorrow.
Comment #21
benjifisherThe 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:
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.
Comment #22
CandC540 CreditAttribution: CandC540 commentedTested and it seems to work, but still did not solve http://drupal.org/node/1419122
Not sure if this is kosher or not, but I added to your patch and made an assumption that if the start and end dates are equal, treat the event as "All Day". It works on my end, but I'm sure that someone else can make it a little less crude.
Here's the patch based on #21. Hopefully I'm doing this right, because I've never used git before.
Comment #23
KarenS CreditAttribution: KarenS commentedThis is great, thanks!
I committed most of what is in #21, with a few changes. Let's see how that works.
http://drupalcode.org/project/calendar.git/commit/61d2115
Comment #24
benjifisherI 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.
Comment #25
KarenS CreditAttribution: KarenS commentedChange the version to make this to be ported. The code in D6 and D7 is totally different, D6 will need its own patch. You will probably can't learn much from what I did because it wouldn't be applicable to D6.
Comment #26
benjifisher@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:Comment #27
MickC CreditAttribution: MickC commentedHi guys - where is the D6 patch? This issue is filed under D6 but looks like the patches are D7.
My iCal exports are showing up with dates 1 day earlier
Can't find anything on it except this http://drupal.org/node/1419122 which is D7 but refers back here anyway.
Thanks.
Comment #28
benjifisherI 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.
Comment #28.0
benjifisherupdate advice on avoiding git problems
Comment #29
Neslee Canil PintoThe D6 branch is no longer supported so we're closing this issue. If you think this issue still exists in D7 you can open a new one.