Moodle
  1. Moodle
  2. MDL-37634

qtype readquestion method visibility inconsistency

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.4, 2.4, 2.5
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Lesson
    • Labels:
      None
    • Testing Instructions:
      Hide

      Import questoins in as many supported formats as possible onto the question bank and the lesson module.

      You can find example files in places like question/format/xxx/tests/fixtures.

      Show
      Import questoins in as many supported formats as possible onto the question bank and the lesson module. You can find example files in places like question/format/xxx/tests/fixtures.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull Master Branch:
    • Rank:
      47326

      Description

      When trying to import questions with the lesson module I receive a fatal error regarding qformat_learnwise::readquestion() method visibility.

      I haven't gone deep into the code but mod/lesson/format.php->qformat_default->readquestion() has public visibility and question/format.php->qformat_default->readquestion() protected, the different qformats are also using different visibilities but the problem is caused when extending to a lower level of visibility.

      I experience the problem with MDL-37068 integrated but I guess the problem is with mod/lesson/format.php->qformat_default->readquestion() that should be protected.

      Fatal error: Access level to qformat_learnwise::readquestion() must be public (as in class qformat_default) in /home/davidm/Desktop/moodlecode/INTEGRATION/MOODLE_23_STABLE/question/format/learnwise/format.php on line 189
      Call Stack
      #	Time	Memory	Function	Location
      1	0.0004	260392	{main}( )	../import.php:0
      2	0.2158	26662392	lesson_get_import_export_formats( )	../import.php:55
      

        Issue Links

          Activity

          Hide
          David Monllaó added a comment -

          Attaching patch, is applicable to both 23, 24 and master

          Show
          David Monllaó added a comment - Attaching patch, is applicable to both 23, 24 and master
          Hide
          Tim Hunt added a comment -

          Thanks David. +1 from me, so submitting for integration.

          To explain. Years ago, someone made the stupid decision to re-use the same import classes from the Question bank in the Lesson module by using a different base class. It is amazing it works at all. It is certainly fragile, and often breaks, as here.

          Of course, the proper solution is to change the lesson to use the question bank, but we have been waiting 6+ years for that to happen.

          Show
          Tim Hunt added a comment - Thanks David. +1 from me, so submitting for integration. To explain. Years ago, someone made the stupid decision to re-use the same import classes from the Question bank in the Lesson module by using a different base class. It is amazing it works at all. It is certainly fragile, and often breaks, as here. Of course, the proper solution is to change the lesson to use the question bank, but we have been waiting 6+ years for that to happen.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks David, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks David, this has been integrated now.
          Hide
          Jérôme Mouneyrac added a comment -

          Tested on 2.3:
          question bank / question import with: blackboard, xml, aiken.
          lesson / question import with: Aiken, blackboard, blackboard 6+, examview, gift, xml

          Not fatal errors. Passed. Thanks to Tim for all the question samples and thanks to Marina/Barbara for the drag&and drop feature. I really appreciate them for testing.

          Show
          Jérôme Mouneyrac added a comment - Tested on 2.3: question bank / question import with: blackboard, xml, aiken. lesson / question import with: Aiken, blackboard, blackboard 6+, examview, gift, xml Not fatal errors. Passed. Thanks to Tim for all the question samples and thanks to Marina/Barbara for the drag&and drop feature. I really appreciate them for testing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

          (and won't be revisiting it unless some regression is found)

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao
          Hide
          Stefan L added a comment -

          Unfortunately, the patch is just for version 2.3 and higher.
          Any idea for fixing that issue for moodle vesion 2.2x?

          Thanks a lot

          Show
          Stefan L added a comment - Unfortunately, the patch is just for version 2.3 and higher. Any idea for fixing that issue for moodle vesion 2.2x? Thanks a lot
          Hide
          Tim Hunt added a comment -

          It's a one-line change. If you want it in 2.2 (which is no longer supported) you will have to make the change yourself.

          Show
          Tim Hunt added a comment - It's a one-line change. If you want it in 2.2 (which is no longer supported) you will have to make the change yourself.
          Hide
          Stefan L added a comment -

          Ah ok...is it really a one line code change?
          Did you already make a the fix for 2.2x and can tell me where i have to change what?

          Many thanks

          Show
          Stefan L added a comment - Ah ok...is it really a one line code change? Did you already make a the fix for 2.2x and can tell me where i have to change what? Many thanks
          Hide
          Stefan L added a comment -

          Ah... found it. Overseen the diff.
          Thanks...

          Show
          Stefan L added a comment - Ah... found it. Overseen the diff. Thanks...
          Hide
          Jean-Michel Vedrine added a comment -

          In the mod/lesson/format.php file
          find the line:
          function readquestion($lines) {
          and change it to:
          protected function readquestion($lines) {
          (That is to say : just add the word protected and you are done )

          Show
          Jean-Michel Vedrine added a comment - In the mod/lesson/format.php file find the line: function readquestion($lines) { and change it to: protected function readquestion($lines) { (That is to say : just add the word protected and you are done )
          Hide
          Jean-Michel Vedrine added a comment -

          Oops just a minute too late

          Show
          Jean-Michel Vedrine added a comment - Oops just a minute too late
          Hide
          Stefan L added a comment -

          nevertheless, thanks

          Show
          Stefan L added a comment - nevertheless, thanks

            People

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

              Dates

              • Created:
                Updated:
                Resolved: