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 Master Branch:

      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
      

        Gliffy Diagrams

          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: