Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Calendar
    • Labels:
    • Testing Instructions:
      Hide
      1. Goto sitepages>calander>manage subscription
      2. Remove all subscriptions you have there
      3. add this http://ical.mac.com/ical/US32Holidays.ics as subscription
      4. Make sure all events are created properly
      5. Remove the subscription
      6. Make sure all events corresponding to the url are deleted.
      7. repeat step 3 -6 for the two attached file.
      Show
      Goto sitepages>calander>manage subscription Remove all subscriptions you have there add this http://ical.mac.com/ical/US32Holidays.ics as subscription Make sure all events are created properly Remove the subscription Make sure all events corresponding to the url are deleted. repeat step 3 -6 for the two attached file.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-36592-master
    • Rank:
      46077

      Description

      copying my comment from linked issue:-
      While working on this I noticed some files dont work, while others did with the correct format. after spending quite some time debugging the problem I found was the "newline" character used in those files i.e "\n" instead of "\r\n". Which was breaking everything as $ical::unserialize couldnt find the event details as it does an explode on "\r\n" only. I will create a blocker for that .
      try using this file http://ical.mac.com/ical/US32Holidays.ics
      Thanks

      1. AustraliaHolidays.ics
        5 kB
        Michael de Raadt
      2. US32Holidays.ics
        11 kB
        Michael de Raadt

        Issue Links

          Activity

          Hide
          Rossiani Wijaya added a comment -

          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

          Show
          Rossiani Wijaya added a comment - 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
          Hide
          Ankit Agarwal added a comment -

          Thanks Rosie for the review.
          Sending for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Rosie for the review. Sending for integration. Thanks
          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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
          Hide
          CiBoT added a comment -

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

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Ankit Agarwal added a comment - - edited

          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
          Show
          Ankit Agarwal added a comment - - edited Hi Sam, I have removed the code style improvements (my editor was adamant about it ) As we discussed this, this cant be done with explode so I have used the preg_split as before. and updated the readme. 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
          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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
          Hide
          Sam Hemelryk added a comment -

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

          Show
          Sam Hemelryk added a comment - Testing will be performed as part of the QA testing on the linked issue.
          Hide
          Dan Poltawski added a comment -

          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

          Show
          Dan Poltawski added a comment - 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

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: