Problem/Motivation

When testing this module, I first found that iCal exports would not import to Outlook or to Apple Mail. The iCal validator (https://icalendar.org/validator.html) reported multiple elements that needed adjusting:

  • Missing required PRODID property near line # 1
  • Reference: RFC 5545 3.6. Calendar Components
  • At least one STANDARD or DAYLIGHT property must be defined here near line # 2
  • Reference: RFC 5545 3.6.5. Time Zone Component
  • Missing DTSTAMP property near line # 6
  • Reference: RFC 5545 3.6.1. Event Component
  • Missing UID property near line # 6
  • Reference: RFC 5545 3.6.1. Event Component
  • Invalid TZID value or missing VTIMEZONE component (America/Chicago) near line # 4
  • Reference: 3.2.19. Time Zone Identifier
  • Invalid TZID value or missing VTIMEZONE component (America/Chicago) near line # 4
  • Reference: 3.2.19. Time Zone Identifier

Proposed resolution

Provide a minimally compatible iCal export by meeting all of the criteria flagged by the iCal validator and demonstrating that events can be imported into calendaring systems like Outlook and Apple Mail.

CommentFileSizeAuthor
#16 addtocal_augment-ical_compatibility-3233526-16.patch17.03 KBmark_fullmer
#16 interdiff_15-16.txt1.27 KBmark_fullmer
#15 interdiff_14-15.patch733 bytesmandclu
#15 addtocal_augment-ical_compatibility-3233526-15.patch17.02 KBmandclu
#14 addtocal_augment-ical_compatibility-3233526-14.patch16.96 KBmark_fullmer
#14 interdiff_11-14.txt4.73 KBmark_fullmer
#11 addtocal_augment-ical_compatibility-3233526-11.patch15.98 KBmark_fullmer
#11 interdiff_10-11.txt13.29 KBmark_fullmer
#11 interdiff_5-11.txt14.5 KBmark_fullmer
#10 addtocal_augment-ical_compatibility-3233526-10.patch2.4 KBmark_fullmer
#10 interdiff_5-10.txt2.4 KBmark_fullmer
#4 3233526-ical-compatibility_3.patch5.56 KBmark_fullmer
#5 addtocal_augment-ical_compatibility-3233526-5.patch5.6 KBmandclu
#5 interdiff_3-5.txt1003 bytesmandclu

Issue fork addtocal_augment-3233526

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mark_fullmer created an issue. See original summary.

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
commented
Title: Timezone format incorrect for iCal » Multiple incompatibilities with iCal format
Issue summary: View changes

mark_fullmer opened merge request !1

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
commented
Status: Active » Needs review
StatusFileSize
new 3233526-ical-compatibility_3.patch5.56 KB

Further review of the iCal spec turned up a number of things that needed to be addressed. The merge request, https://git.drupalcode.org/project/addtocal_augment/-/merge_requests/1 , which is the same as the attached patch, makes this module provide an iCal compatible export.

This approach also reworks a bit of the building of the Google Calendar link & iCal export to make them (hopefully) easier to maintain.

mandclu’s picture

mandclu commented
StatusFileSize
new addtocal_augment-ical_compatibility-3233526-5.patch5.6 KB
new interdiff_3-5.txt1003 bytes

Thanks very much for your work on this!

Overall what you've done is great, but I have two minor quibbles:

1. If we're going to set a UID value, we should probably use the the entity's uuid, if available
2. The validator still throws errors unless we use CRLF as the ical separator

Attached is an updated patch with these changes. Please test, if this works for you as it does in my local I'll get this merged in.

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
commented

If we're going to set a UID value, we should probably use the the entity's uuid, if available.

Yes, that certainly would be more unique. I'm on board with the UUID value.

I did notice, after supplying the original patch, that while the output *does* pass general iCal validation, one version of MS Outlook refused to import the event; simply relocating the DATETIME value resolved that issue. I'll incorporate that into your most recent patch when I test it.

(As an aside, do you have any thoughts/recommendations about automated testing for this? It would be fairly onerous to either mock various date fields or write a Functional test that creates a date field, ideally one based on the smart_date module, and even then, the output isn't going to validate that it is iCal compliant. A quick search didn't turn up a PHP-based iCAL validator API. Should we just hold off on automated testing until this module gets more adoption?)

mandclu’s picture

mandclu commented

The Smart Date Starter Kit does provide the configuration for an Event content type with a Smart Date field. Could that be used for the automated test?

sbentley’s picture

sbentley commented

Hello, thanks to both of you for making this module and also improving it!

I was fixing an issue where tz_prefix returning null when I stumbled on this, which seems to already address the issue with a similar fix to mine. I hadn't come across the other compatibility issues yet, but would like to get ahead of them using this branch.

Is this branch ready for a merge?

Thanks again for all your guys work!

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
commented

Is this branch ready for a merge?

Not quite ready, but I'm working on the final incompatibilities I noted today, and should have a patch ready later today. I'll also document the specific incompatibilities fixed.

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
commented
StatusFileSize
new interdiff_5-10.txt2.4 KB
new addtocal_augment-ical_compatibility-3233526-10.patch2.4 KB

Okay, the attached patch simply fixes some additional incompatibilities/problems, but is otherwise the same as #5. Changes:

1. Added a "download" attribute to the Outlook & iCal links. In Firefox, without this value present, the file is downloaded without a filename (i.e., just the extension, ".ics"), and this is not importable in MS Outlook.
2. Moved the TZID value directly after BEGIN:VTIMEZONE . This was also observed to be necessary in order for Outlook not to error during import (this is NOT reported as a validation error in https://icalendar.org/validator.html , to be clear).
3. Explicitly set the DTSTAMP format to Ymd\\THi00, which is what it should always be. In the previous code, when an event was set to be All Day, the $date_format value was not populated and would cause the DTSTAMP field to render with an empty value.

The attached patch does not include functional tests. If I can get to those today, I'll post an additional patch. If not, I'll leave a comment that this should potentially be evaluated manually & merged in, and tests be added later.

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
commented
StatusFileSize
new interdiff_5-11.txt14.5 KB
new interdiff_10-11.txt13.29 KB
new addtocal_augment-ical_compatibility-3233526-11.patch15.98 KB

Behold, tests!

The interdiff looks like a fair amount of change to the AddToCal plugin class, but it's really just:

  • converting what were global calls to system.site and system.date to use dependency injection so that they can be mocked in a Unit test context, and
  • moving the business logic into a separate function so that the Unit test can more cleanly represent the tested data as array elements rather than the already rendered links (i.e., for ease of developer debugging)

This Drupal project will need to turn on automated testing so that we can see this pass.

The current test coverage checks output for:
- A single event occurrence
- A recurring event
- An all-day event

It does not (cannot) actually validate that the output is iCal complaint. That'll be up to us.

mandclu’s picture

mandclu commented

This looks great! Thanks so much for the work you put into it.

One question: Can you explain why you decided to switch to a PHP DateTime object here in src/Plugin/DateAugmenter/AddToCal.php?

@@ -48,7 +108,7 @@ class AddToCal extends DateAugmenterPluginBase implements PluginFormInterface {
       return;
     }
     $end_fallback = $end ?? $start;
-    $now = new DrupalDateTime();
+    $now = new \DateTime();
     // For a recurring date, determine if the last instance is in the past.
     $upcoming_instance = FALSE;
     // TODO: Validate that if set, $options['ends'] is DrupalDateTime.
mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
commented

One question: Can you explain why you decided to switch to a PHP DateTime object here in src/Plugin/DateAugmenter/AddToCal.php?

Ah, that was an artifact of the iterative process of writing the unit test: using PHP's DateTime was intended to be an intermediary step for building out the test that didn't require dependency injection. We should go back and add dependency injection for DrupalDateTime so that it can be mocked in the Unit test, or use the datetime testing API in https://www.drupal.org/project/datetime_testing .

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
commented
StatusFileSize
new interdiff_11-14.txt4.73 KB
new addtocal_augment-ical_compatibility-3233526-14.patch16.96 KB

The attached patch restores the use of DrupalDateTime, updating the unit test to override the method, since it is a static call, per the "preferred method" described in https://www.drupal.org/docs/automated-testing/phpunit-in-drupal/unit-tes....

mandclu’s picture

mandclu commented
StatusFileSize
new addtocal_augment-ical_compatibility-3233526-15.patch17.02 KB
new interdiff_14-15.patch733 bytes

At first I was surprised you weren't using the time format for rendering the DTSTAMP property, but after doing some investigation I could see that you are correct, and this property must be defined in UTC. I tweaked your code slightly to force UTC as the timezone before rendering the value, and also appended the "Z" at the end, which appears to be best practice, based on examples I could find.

That said, I'm not currently able to test with Outlook, so if you can validate that everything works as intended with this change, I'll get this committed and roll a new release.

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
commented
StatusFileSize
new interdiff_15-16.txt1.27 KB
new addtocal_augment-ical_compatibility-3233526-16.patch17.03 KB

I've verified that with the forced UTC timezone on the DTSTAMP value, ical output is still importable via Outlook (and it validates iCal, as well).

The attached patch just updates the existing test to reflect the UTC change.

In my opinion, this is good to merge now!

  • mandclu committed 9e8fdee on 1.0.x authored by mark_fullmer 
    Issue #3233526 by mark_fullmer, mandclu: Multiple incompatibilities with...
mandclu’s picture

mandclu commented
Status: Needs review » Fixed

Excellent! Merged in, and I'll roll a new release shortly. Thanks again for your work here.

Status: Fixed » Closed (fixed)

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