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

          Attachments

            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