Moodle
  1. Moodle
  2. MDL-30393

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      33014

      Description

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

        Activity

        Hide
        Jonathan Harker added a comment -

        Git branch, see above.

        Show
        Jonathan Harker added a comment - Git branch, see above.
        Hide
        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
        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
        Peter Bulmer added a comment -

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

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

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

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

        Looks good to me.

        Show
        Peter Bulmer added a comment - Looks good to me.
        Hide
        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
        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
        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
        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 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 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
        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
        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
        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
        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
        Michael de Raadt added a comment -

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

        Show
        Michael de Raadt added a comment - Thanks for your work on this, Jonathan, Peter and Sam.
        Hide
        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
        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
        Aparup Banerjee added a comment -

        no problems seen here.

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

        Show
        Aparup Banerjee added a comment - no problems seen here. passing. (proper functionality is still being worked on in MDL-16660 )
        Hide
        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
        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: