Problem/Motivation
When using the calendar_ical module to generate a feed, events that are more than one day and are all-day events (formatted as DATE instead of DATETIME) are off by one day. This is because the ical specification (RFC 2445, clarified in RFC 5545) specifies that the DTEND value is non-inclusive. The two calendar programs I use (Apple iCal and Google Calendar) both follow the ical specification, and when I import a Drupal-generated ical into either program, my events show up ending one day earlier than they should.
Another problem is that dates (both DTSTART and DTEND) for all-day events are calculated incorrectly if the local timezone has a positive offset relative to UTC. For example, Europe/Berlin.
Proposed resolution
The attached patch modifies template_preprocess_date_vcalendar()
in theme/theme.inc
.
A work-around is to put a modified calendar/calendar_ical/calendar-view-ical.tpl.php
into your theme folder. See #1038218-17: All Day events not RFC 2445/3339/5545 compliant. Or you could copy template_preprocess_date_vcalendar()
to template.php
in your own theme folder, rename the function THEME_preprocess_date_vcalendar()
(replacing THEME with the name of your theme) and make the changes from this patch.
Remaining tasks
Please review this patch.
User interface changes
N/A
API changes
This patch modifies template_preprocess_date_vcalendar()
.
Related Issues
If you commit this patch, please give joint credit to crifi and benjifisher (see #1038218: All Day events not RFC 2445/3339/5545 compliant). We discussed the issue and developed patches there before deciding that this change should be made here in the preprocess function instead of in the template file.
The patch in #5 also addresses #517186: iCal doesn't take account of timezones in all day events
When going in the other direction (importing an ical file into Drupal) the corresponding change should be made. See #606658: Multiple day VEVENT not working from iCal.
Comment | File | Size | Author |
---|---|---|---|
#17 | ical_wrong_DTEND-1282538-17.patch | 2.07 KB | arlinsandbulte |
#11 | date_ical_dtend_1282538_11.patch | 3.65 KB | benjifisher |
#5 | date_ical_dtend_1282538_5.patch | 3.65 KB | benjifisher |
date_ical_dtend.patch | 973 bytes | benjifisher | |
Comments
Comment #1
crifi CreditAttribution: crifi commentedSince this is a modification of my patch (http://drupal.org/node/1038218#comment-4994326) at another location, I have reviewed it and it's working.
Comment #2
benjifisherI have opened an issue for the D7 version: #1284170: All-day events missing or wrong in ical feed. For D7, the patch is in the Calendar module, not Date.
Comment #3
benjifisherThis patch changes
template_preprocess_date_vcalendar()
inmodules/date/theme/theme.inc
, which is called in the last line oftemplate_preprocess_calendar_view_ical()
inmodules/calendar/calendar_ical/theme.inc
. So we could fix this problem in the Calendar module instead of in the Date module. Does the code from the Date module get used anywhere else?Comment #4
crifi CreditAttribution: crifi commentedIt isn't a calendar module specific problem since it's definitely a bug with this function. A third party module could also use this function for own ical feeds. In my opinion timezone calculation isn't the task of the calendar ical module and should be fully transfered from the calendar module to the date module.
Comment #5
benjifisherThe attached patch correctly calculates the DTSTART and DTEND values for all-day events in more cases. In timezones with a positive offset from UTC, such as Europe/Berlin, a local time such as "2011-09-05 00:00:00" is converted into something like "2011-09-04 22:00:00" in UTC. We want to use 09-05 and not 09-04. See #517186: iCal doesn't take account of timezones in all day events.
The attached patch is a lot larger than the previous one because I replaced
with
I will also update the issue summary.
Comment #6
benjifishercrifi has confirmed that the patch in #5 fixes #517186: iCal doesn't take account of timezones in all day events and marked that issue as a duplicate of this one.
Comment #7
crifi CreditAttribution: crifi commentedThanks Benji, I will review the code by the end of this week.
Comment #8
penyaskitoSubscribing
Comment #9
crifi CreditAttribution: crifi commentedVery good work. I have reviewed the code and patch works here in my testing environment.
Comment #10
benjifisherThis needs to be updated after #1302052-23: CRLF line endings cause problems with "git apply".
Comment #11
benjifisherThis is an update of the patch in #5. I changed only one character: It seems that an earlier patch correctly replaced a ";" with a ":". The patch in #5 already made the same change.
I have not tested this yet. Any testing of this patch should be done along with crifi's patch in #1038218: All Day events not RFC 2445/3339/5545 compliant.
It has been more than three months since I made the patch in #5, and I do not remember everything I was doing, but it was marked RTBC at the time.
Comment #12
benjifisherI tested and validated the patch in #11. If it were not my own work, I would mark it RTBC.
Comment #13
arlinsandbulte CreditAttribution: arlinsandbulte commentedI'm going to ask KarenS to take a look at this & the other related patch... #1038218: All Day events not RFC 2445/3339/5545 compliant
Comment #14
arlinsandbulte CreditAttribution: arlinsandbulte commentedputting on my list to review & commit...
Comment #15
KarenS CreditAttribution: KarenS commentedThe first few lines of the patch make changes to the code that have nothing to do with this problem, they just handle the test for empty dates in a different way, which makes it harder to review the patch. It's best to make patches that only touch the bug.
I see:
and I don't see $date_format added back anywhere, but the variable is still used. That doesn't seem right.
Please see if you can reduce this patch to just the changes needed to eliminate the bug.
Comment #16
arlinsandbulte CreditAttribution: arlinsandbulte commentedYep, I agree. Needs work.
IMO, I prefer not using a continue statements like this. Seems like spaghetti code to me.
Also, I think this is what KarenS meant by her comment in #15. This changes the indentation of everything below which makes it REALLY hard to review as every line changes, even if it is only an indentation change.
First make a patch the does the MINIMUM to fix the bug.
Later we can talk about code style patches.
Comment #17
arlinsandbulte CreditAttribution: arlinsandbulte commentedHere is a re-factored patch that does not change the test for empty dates.
Also, while KarenS was correct that the $date_format declaration was removed, she was incorrect that it was still used elsewhere in the function. After this patch, it is NOT used anywhere else in the function.
Comment #18
KarenS CreditAttribution: KarenS commentedOK, this it's much easier to see what you're doing here. We have a date object to use, we don't need strtotime() or date() functions. In fact we avoid using them because you can't control the timezone in those the way you can with date objects.
Going back to the way this was already fixed in D7, and using the date object we already have, I came up with this patch instead:
Comment #19
KarenS CreditAttribution: KarenS commentedAnd if the start date getting passed in is wrong, that is a calendar issue. This function should assume the right values have already be passed in.
Comment #20
KarenS CreditAttribution: KarenS commentedCommitted this. There may still be a calendar issue for the way the start date is created if you get the wrong value by formatting it. There may already be an issue for that, but if not please open a calendar issue for the problem with the timezone conversion in the start date.
http://drupalcode.org/project/date.git/commit/5004a85
Comment #21.0
(not verified) CreditAttribution: commentedI added a reference to #517186: iCal doesn't take account of timezones in all day events and mentioned that the latest patch fixes that problem, too.