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:
    • Rank:
      47845

      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.

        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: