Moodle
  1. Moodle
  2. MDL-38051

Annihilate all the CRLFs present in some question/fixtures...

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.4, 2.4.1
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      1) Verify the patch only contains whitespace changes at EOL.

      git diff git diff --ignore-space-at-eol

      2) Verify all question tests pass.

      3) Verify that, once integrated, the CI servers are right now passing the "illegal whitespace" jobs.

      Show
      1) Verify the patch only contains whitespace changes at EOL. git diff git diff --ignore-space-at-eol 2) Verify all question tests pass. 3) Verify that, once integrated, the CI servers are right now passing the "illegal whitespace" jobs.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:

      Description

      Ignited by CONTRIB-4158 and detected by MDL-38040, it was detected that some question/fixtures do contain CRLFs and can be safely transformed to proper LFs.

      (See https://tracker.moodle.org/browse/CONTRIB-4158?focusedCommentId=203237&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-203237 for more info)

      The offending files are under:

      +question/format/blackboard/tests/fixtures/
      +question/format/blackboard_six/tests/fixtures/
      +question/format/xml/tests/fixtures/

      So this issue is about to:

      1) Take rid of all those CRLFs by changing them to LFs.
      2) In the CI servers take those directories out from quarantine.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Jean-Michel Vedrine added a comment -

            Hello Eloy,
            I made a branch for master. Will make others branchs when master is OK
            I think I know how to verify I only introduced line ending changes as git diff --ignore-space-at-eol master..MDL-38051 return nothing
            But how should I search to verify I didn't missed any CRLF ?
            Sorry to be such a noob at using git diff and git grep

            Show
            Jean-Michel Vedrine added a comment - Hello Eloy, I made a branch for master. Will make others branchs when master is OK I think I know how to verify I only introduced line ending changes as git diff --ignore-space-at-eol master.. MDL-38051 return nothing But how should I search to verify I didn't missed any CRLF ? Sorry to be such a noob at using git diff and git grep
            Hide
            Jean-Michel Vedrine added a comment -

            Hello,
            I have solved my problem and I am now able to search for all CRLF so I am quite sure I have suppressed all of them.
            In fact question\format\blackboard\tests\fixtures\sample_blackboard.dat and question\format\blackboard_six\tests\fixtures\sample_blackboard_pool.dat was the same offending file that I copied from blackboard to blackboard_six when I added to blackboard_six the ability to also import files that were previously imported by the blackboard format.
            One thing is still surprising me: how I added a CRLF to one of the lines in question\format\xml\tests\fixtures\truefalse.xml ?
            Now that master branch is done, I will create branchs for MOODLE_STABLE_24 and MOODLE_STABLE_23.

            Show
            Jean-Michel Vedrine added a comment - Hello, I have solved my problem and I am now able to search for all CRLF so I am quite sure I have suppressed all of them. In fact question\format\blackboard\tests\fixtures\sample_blackboard.dat and question\format\blackboard_six\tests\fixtures\sample_blackboard_pool.dat was the same offending file that I copied from blackboard to blackboard_six when I added to blackboard_six the ability to also import files that were previously imported by the blackboard format. One thing is still surprising me: how I added a CRLF to one of the lines in question\format\xml\tests\fixtures\truefalse.xml ? Now that master branch is done, I will create branchs for MOODLE_STABLE_24 and MOODLE_STABLE_23.
            Hide
            Jean-Michel Vedrine added a comment -

            Hi Eloy, can you have a look to see if everything is OK ? Additionally, if there is no problem, I don't think I can send this to integration myself, so you will have to take care of this too Thanks.

            Show
            Jean-Michel Vedrine added a comment - Hi Eloy, can you have a look to see if everything is OK ? Additionally, if there is no problem, I don't think I can send this to integration myself, so you will have to take care of this too Thanks.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Sending to integration, looks perfect, thanks!

            I suppose that the "isolated" CRLF cases come from some copy/paste action or so, don't know really.

            Note to integrators: Immediately BEFORE sending this to integration.git you must:

            1) Revert the 402ac743b300f5d59e723a51c01f9a16065afc6f commit, so the question/format/xxxx directories fixed here will again examined by the CI servers.

            2) Execute manually the "Moodle CI Auto Upgrade" job, so the CI servers will get the changes performed in 1).

            Then and only then, push this issues to integration.git and verify that the whitespace jobs remain stable and passing (that's point 3 of testing instructions).

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Sending to integration, looks perfect, thanks! I suppose that the "isolated" CRLF cases come from some copy/paste action or so, don't know really. Note to integrators: Immediately BEFORE sending this to integration.git you must: 1) Revert the 402ac743b300f5d59e723a51c01f9a16065afc6f commit, so the question/format/xxxx directories fixed here will again examined by the CI servers. 2) Execute manually the "Moodle CI Auto Upgrade" job, so the CI servers will get the changes performed in 1). Then and only then, push this issues to integration.git and verify that the whitespace jobs remain stable and passing (that's point 3 of testing instructions). Ciao
            Hide
            Dan Poltawski added a comment -

            Integrated to master, 24 and 23, thanks Jean-Michel

            Show
            Dan Poltawski added a comment - Integrated to master, 24 and 23, thanks Jean-Michel
            Hide
            Dan Poltawski added a comment -

            The diff looks good, the CI server has passed the whitespace tests, just waiting for phpunit now

            Show
            Dan Poltawski added a comment - The diff looks good, the CI server has passed the whitespace tests, just waiting for phpunit now
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Passing this on behalf of Dan, unit tests are passing ok.

            Show
            Eloy Lafuente (stronk7) added a comment - Passing this on behalf of Dan, unit tests are passing ok.
            Hide
            Damyon Wiese added a comment -

            Thanks for your hard work - this issue has made it! Moodle is now a little bit better.

            Regards, Damyon

            Show
            Damyon Wiese added a comment - Thanks for your hard work - this issue has made it! Moodle is now a little bit better. Regards, Damyon

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: