Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-30393

Small fixes to lib/bennu to correctly support RFC-2445

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2
    • Component/s: Calendar
    • Labels:
    • Testing Instructions:
      Hide

      This should result in two observable bug fixes - when the calendar is dealing with an iCalendar event or file:

      1. it should no longer produce an error if it contains a blank line, and
      2. it should now accept CN parameter values that contain double quotes.

      This would only matter when importing calendar data, which Moodle does not currently do, although there is proposed code over at MDL-16660 which does.

      Show
      This should result in two observable bug fixes - when the calendar is dealing with an iCalendar event or file: 1. it should no longer produce an error if it contains a blank line, and 2. it should now accept CN parameter values that contain double quotes. This would only matter when importing calendar data, which Moodle does not currently do, although there is proposed code over at MDL-16660 which does.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:

      Description

      1. RFC-2445: Correctly ignore blank lines
      2. RFC-2445: Fix quoted parameter values
      3. misc: Add PHP 5.x parent constructors

        Gliffy Diagrams

          Activity

          Hide
          jonathan Jonathan Harker added a comment -

          Git branch, see above.

          Show
          jonathan Jonathan Harker added a comment - Git branch, see above.
          Hide
          salvetore Michael de Raadt added a comment -

          Hi, Jonathan.

          Would you like to push this through peer review and onto integration, otherwise it will take a while for this to be integrated.

          Show
          salvetore Michael de Raadt added a comment - Hi, Jonathan. Would you like to push this through peer review and onto integration, otherwise it will take a while for this to be integrated.
          Hide
          peterbulmer Peter Bulmer added a comment -

          Found 1 line that wasn't compliant with coding standards & advised Jon what this was.

          Show
          peterbulmer Peter Bulmer added a comment - Found 1 line that wasn't compliant with coding standards & advised Jon what this was.
          Hide
          jonathan Jonathan Harker added a comment -

          Duly noted and fixed - updated branch (see above).

          Show
          jonathan Jonathan Harker added a comment - Duly noted and fixed - updated branch (see above).
          Hide
          peterbulmer Peter Bulmer added a comment -

          Looks good to me.

          Show
          peterbulmer Peter Bulmer added a comment - Looks good to me.
          Hide
          peterbulmer Peter Bulmer added a comment -

          Not quite sure of the exact workflow here.

          I've checked over the diff & while I'm not a subject expert, these changes make good sense to me & it seems to be compliant to coding standards.

          Show
          peterbulmer Peter Bulmer added a comment - Not quite sure of the exact workflow here. I've checked over the diff & while I'm not a subject expert, these changes make good sense to me & it seems to be compliant to coding standards.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hi guys,

          I've had a quick look at the patch prior to integration, on the surface the changes look good however there is one thing missing lib/bennu/readme_moodle.txt needs to be updated with details of these changes.
          As for integration I've never looked at the bennu lib before so I will check with Eloy to see if he is, if he isn't or his plates full I'll pick this up a bit later on.
          Surely though if you are able to update that readme with details it will help the integration process.

          Cheers
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi guys, I've had a quick look at the patch prior to integration, on the surface the changes look good however there is one thing missing lib/bennu/readme_moodle.txt needs to be updated with details of these changes. As for integration I've never looked at the bennu lib before so I will check with Eloy to see if he is, if he isn't or his plates full I'll pick this up a bit later on. Surely though if you are able to update that readme with details it will help the integration process. Cheers Sam
          Hide
          jonathan Jonathan Harker added a comment -

          The reason I didn't update the readme is because I intend to push the same changes to upstream lib/bennu once I track down my SourceForge login/key details!

          Show
          jonathan Jonathan Harker added a comment - The reason I didn't update the readme is because I intend to push the same changes to upstream lib/bennu once I track down my SourceForge login/key details!
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Ok no probs Jon, I was in fact going to ask you what the case with upstream was so that answers my next question as well!
          I'll review this for integration now

          Show
          samhemelryk Sam Hemelryk added a comment - Ok no probs Jon, I was in fact going to ask you what the case with upstream was so that answers my next question as well! I'll review this for integration now
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Jon this has been integrated now.
          Could you please add some testing instructions to this issue so that the testers know what to cover.

          Cheers
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Jon this has been integrated now. Could you please add some testing instructions to this issue so that the testers know what to cover. Cheers Sam
          Hide
          salvetore Michael de Raadt added a comment -

          Thanks for your work on this, Jonathan, Peter and Sam.

          Show
          salvetore Michael de Raadt added a comment - Thanks for your work on this, Jonathan, Peter and Sam.
          Hide
          nebgor Aparup Banerjee added a comment -

          not sure how to test this exactly.. i've tried multiple lines and some being blank... i don't see any errors. where do i put in CN parameter values?

          Show
          nebgor Aparup Banerjee added a comment - not sure how to test this exactly.. i've tried multiple lines and some being blank... i don't see any errors. where do i put in CN parameter values?
          Hide
          nebgor Aparup Banerjee added a comment -

          no problems seen here.

          passing. (proper functionality is still being worked on in MDL-16660)

          Show
          nebgor Aparup Banerjee added a comment - no problems seen here. passing. (proper functionality is still being worked on in MDL-16660 )
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          And this has landed upstream, just on time for the upcoming new releases week. Thanks for it!

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - And this has landed upstream, just on time for the upcoming new releases week. Thanks for it! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                5/Dec/11