Hi Ankit,

The patch looks great. I also tested on 2.4.

[y] Syntax
[y] Output
[y] Whitespace
[-] Language
[-] Databases
[y] Testing
[-] Security
[-] Documentation
[y] Git
[y] Sanity check

Thanks Rosie for the review.
Sending for integration.
Thanks

Hi Ankit,

The bennu lib is an external library, we need to avoid changes to it, or at least minimise changes to ensure should (however unlikely) an updated version get released we can upgrade to it with minimal conflicts.

1/ The white space fixes need to be reverted sorry.
2/ Lets consider the approach taken of modifying the bennu lib.
One of two things needs to happen, I don't mind which way we go, I just thought I'd point out the options available.
a/ If we decide to take the current approach of modifying the bennu lib we need to ensure we note any changes we make in its moodle readme file so that we can reapply them if we upgrade.
Keeping them simple is also a goal so perhaps fixing the line endings as one operation and then splitting in another operation would be favourable as we would have a line that we would need to copy when upgrading bennu rather than a line we would need to modify.
b/ Alternatively we can correct any line ending issues before we call the unserialise function.
This approach would see us fix the line endings before we call serialise. While not as convenient this does mean we don't need to modify the bennu lib and is perhaps more in line with how bennu has been written.

Worth noting is that both the iCal RFC used when this library was written (2445) and the updated iCal RFC (5545) state line endings should in fact be CRLF. What its doing at the moment is correct according to the spec, and if you validate that feed you'll get warnings.

Many thanks
Sam

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

Hi Sam,

1. I have removed the code style improvements (my editor was adamant about it )
2. As we discussed this, this cant be done with explode so I have used the preg_split as before. and updated the readme.
3. Just for note this is not an upstream issue as the standard is "\r\n" (http://www.ietf.org/rfc/rfc2445.txt) but many ical generators donot follow that, so we cannot be that strict.
   Pushing back for integration
   Thanks

Hi Ankit,

This has been integrated now.

Just to clarify for anyone else we discussed this modification of Bennu and while we acknowledge the fix is against the nature of Bennu and would not necessarily be accepted upstream it works and "unblocks" the QA issue.

Cheers
Sam

Testing will be performed as part of the QA testing on the linked issue.

Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week!

ciao
Dan