Cancel ICS file corrupted on Office 365 #1780

[puhr-mde]

Since Office 365 with Outlook for web respects the Contenttype-header attribute für ICalendar-Method there is a need to set the property suitable to your ICal-Method. $this->Encoding can be used to change the method value (Default = REQUEST).

In order to prevent literal errors the propery can be set by contants matching to the RFC.

```const ICAL_METHOD_REQUEST = "REQUEST";
const ICAL_METHOD_PUBLISH = "PUBLISH";
const ICAL_METHOD_REPLY = "REPLY";
const ICAL_METHOD_ADD = "ADD";
const ICAL_METHOD_CANCEL = "CANCEL";
const ICAL_METHOD_REFRESH = "REFRESH";
const ICAL_METHOD_COUNTER = "COUNTER";
const ICAL_METHOD_DECLINECOUNTER = "DECLINECOUNTER";```

[Synchro]

I think it would be safer and simpler to skip this property and extract the method from the Ical property instead, using the permitted values declared in the constants. Otherwise you're leaving it open to the user setting a method that does not match the content.

[Synchro]

Also all references to these constants should be static rather than self to allow overriding.

[puhr-mde]

If you say skippen, do you mean remove it from the Header or ignoring it in processing the icalendar?

[Synchro]

I mean remove it as an instance property and instead select the appropriate method by matching the value within the Ical content. So in the code:

$methods = [
    static::ICAL_METHOD_REQUEST,
    static::ICAL_METHOD_PUBLISH,
    static::ICAL_METHOD_REPLY,
    static::ICAL_METHOD_ADD,
    static::ICAL_METHOD_CANCEL,
    static::ICAL_METHOD_REFRESH,
    static::ICAL_METHOD_COUNTER,
    static::ICAL_METHOD_DECLINECOUNTER
];
foreach ($methods as $method) {
    if (strpos('METHOD:'.$method, $this->Ical) !== false) {
        $body .= $this->getBoundary($this->boundary[1], '', static::CONTENT_TYPE_TEXT_CALENDAR . '; method='.$method, '');
        break;
    }
}

It might be reasonable to make that $method array a constant as well. With this approach, you don't need the IcalMethod property at all. We need a default to fall back to (probably REQUEST) if none match. Also I don't know if the Ical spec permits spaces around the : separator, and whether it is case-insensitive.

[Synchro]

I was wondering why you hadn't used a constant array here - then I checked and found that you couldn't do that prior to PHP 5.6, and we are meant to retain compatibility back to PHP 5.5 in the 6.x branch, so that will have to wait. That said, this should be protected rather rather than private so that users are free to override it to add or remove Ical methods. I'd also call it IcalMethods since it's an array, not a single value.

[puhr-mde]

Yes, the constant thing will not work by now. I also changed it to case-insensitive. I worked with many groupwaresystems, i think it was Zarafa or Groupwise, in some of these ich came up with all lowercase icalendars. I will change it to protected, good idea. The thing with IcalMethods came to me too, just after commiting, sorry for that one.

[Synchro]

See my other comment, and I'd also like to see some test coverage for this please - making sure that an Ical string results in the correct method being selected in the header, and that the default is used in the absence of one, or if it contains an unknown method.

[puhr-mde]

I've added 3 testcases to check correct method an use default method in case of missing or invalid method attribute.

See my other comment

What do you mean with this? Am I still missing something?

[Synchro]

These \r\n sequences won't work because you're using single quotes, and those chars won't get unescaped; changing to double quotes will fix that.

[Synchro]

The default charset is iso-8859-1, so this won't match. Either change the pattern, or add $this->Mail->CharSet = PHPMailer::CHARSET_UTF8; to the test script.

You'll need to change the other instances of these problems too.

[Synchro]

It's failing the coding standard check because you double quoted some strings that didn't need it.

[puhr-mde]

It's failing the coding standard check because you double quoted some strings that didn't need it.

Sorry, I thought its not wanted to mix quotes in one concatenation of a string. It should now match the cs.

[Synchro]

Thank you very much!

[puhr-mde]

Thank you. This was my first contribution/pull to a project on Github, thanks for your patience with all my bugs or conflicts with your cs.

[Synchro]

Congratulations, and I'm honoured that you chose to do that in this project!
Coding standards can be annoying sometimes, but if you use an IDE that supports them (like PHPStorm), it will flag everything before you even commit. Sticking to a predefined CS scheme also removes the need to think about it!