Moodle
  1. Moodle
  2. MDL-27956

Lesson question import leads to string error

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1
    • Component/s: Lesson
    • Labels:
    • Environment:
      qa.moodle.net
    • Testing Instructions:
      Hide

      1. log in as a teacher
      2. create a new lesson
      3. add a page
      4. click the link "Import questions"

      You should see a page that allows a question file to be imported

      Show
      1. log in as a teacher 2. create a new lesson 3. add a page 4. click the link "Import questions" You should see a page that allows a question file to be imported
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull Master Branch:
      wip-mdl-27956-master
    • Rank:
      17597

      Description

      When editing a lesson, there is an option to import questions (in GIFT format).

      Reproduction steps (on qa.moodle.net):

      1. log in as a teacher
      2. create a new lesson
      3. add a page
      4. click the link "Import questions"

      The following error information is presented

      Invalid get_string() identifier: 'gift' or component 'quiz'
      line 5910 of /lib/moodlelib.php: call to debugging()
      line 6500 of /lib/moodlelib.php: call to core_string_manager->get_string()
      line 880 of /mod/lesson/lib.php: call to get_string()
      line 55 of /mod/lesson/import.php: call to lesson_get_import_export_formats()
      
      ( ! ) Fatal error: Access level to qformat_multianswer::readquestions() must be public (as in class qformat_default) in /html/question/format/multianswer/format.php on line 79
      Call Stack
      #	Time	Memory	Function	Location
      1	0.0066	691520	{main}( )	../import.php:0
      2	0.2115	45708976	lesson_get_import_export_formats( )	../import.php:55
      

        Issue Links

          Activity

          Hide
          moodle.com added a comment -

          It would be worthwhile to check in the lesson code for strings from the Quiz module and check they are all still present.

          Show
          moodle.com added a comment - It would be worthwhile to check in the lesson code for strings from the Quiz module and check they are all still present.
          Hide
          Rajesh Taneja added a comment -

          function scope is different in question/format, as compared to one in mod/lesson/format
          Need top check with Tim before concluding any fix for this.

          Show
          Rajesh Taneja added a comment - function scope is different in question/format, as compared to one in mod/lesson/format Need top check with Tim before concluding any fix for this.
          Hide
          Rajesh Taneja added a comment -

          Solution doesn't break unit test for question/format/xml.
          Also, readquestions is only called from within the class object, so it's safe to make it protected.

          Show
          Rajesh Taneja added a comment - Solution doesn't break unit test for question/format/xml. Also, readquestions is only called from within the class object, so it's safe to make it protected.
          Hide
          Tim Hunt added a comment -

          -1 from me for one part of this change.

          I really don't like moving the strings from quiz -> lesson.

          You are right that these string should not be in the quiz.php lang file. However, they should not be in lesson.php either.

          The correct thing now is

          get_string('pluginname', 'qformat_' . $fileformat)

          like in lib/questionlib.php, and then remove the old quiz strings completely.

          The changes to the includes, and adding protected to the base class, are good.

          Show
          Tim Hunt added a comment - -1 from me for one part of this change. I really don't like moving the strings from quiz -> lesson. You are right that these string should not be in the quiz.php lang file. However, they should not be in lesson.php either. The correct thing now is get_string('pluginname', 'qformat_' . $fileformat) like in lib/questionlib.php, and then remove the old quiz strings completely. The changes to the includes, and adding protected to the base class, are good.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          I'm reopening this, if lesson import is using question importers (qformat plugin), then 100% sure strings should be there and not in quiz nor lesson.

          Perhaps that will require AMOS MOVE on commit? (lol, just saw the commit message when reseting here my integration repo. Surely it needs to be modified to move from mod_quiz to qformat_xxxxx

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited I'm reopening this, if lesson import is using question importers (qformat plugin), then 100% sure strings should be there and not in quiz nor lesson. Perhaps that will require AMOS MOVE on commit? (lol, just saw the commit message when reseting here my integration repo. Surely it needs to be modified to move from mod_quiz to qformat_xxxxx Ciao
          Hide
          Rajesh Taneja added a comment -

          Thanks Tim and Eloy for correcting the solution.
          I have incorporated your feedback. Can you please peer review this for me.
          Also, AMOS move is not required anymore, so removed it from commit message.

          Show
          Rajesh Taneja added a comment - Thanks Tim and Eloy for correcting the solution. I have incorporated your feedback. Can you please peer review this for me. Also, AMOS move is not required anymore, so removed it from commit message.
          Hide
          Tim Hunt added a comment -

          +1 from me now.

          The strings are already in the qformat language files. That is where the question bank looks for them since 2.1, so I had already done the move.

          Show
          Tim Hunt added a comment - +1 from me now. The strings are already in the qformat language files. That is where the question bank looks for them since 2.1, so I had already done the move.
          Hide
          Rajesh Taneja added a comment -

          Thanks Tim

          Show
          Rajesh Taneja added a comment - Thanks Tim
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Looks perfect, thanks. Integrated!

          Show
          Eloy Lafuente (stronk7) added a comment - Looks perfect, thanks. Integrated!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing test without further action. This will be tested by MDLQA-1017 once this meets upstream.

          Show
          Eloy Lafuente (stronk7) added a comment - Passing test without further action. This will be tested by MDLQA-1017 once this meets upstream.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          All git & cvs servers have been updated with these cool changes, so closing, many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - All git & cvs servers have been updated with these cool changes, so closing, many thanks!
          Hide
          Jean-Michel Vedrine added a comment -

          Hello, during this fix the line require_once($CFG->dirroot . '/question/format.php'); was removed from moodle/question/format/xml/format.php and as a result the Manage Questions Types is now broken if you have third party question types installed with import_from_xml / export_to_xml functions (for instance all OU questions types from Tim's git repo like ddwtos gapselect, and also mine ) . Is this change necessary ? if yes what is the recommanded fix for questions types authors ?

          Show
          Jean-Michel Vedrine added a comment - Hello, during this fix the line require_once($CFG->dirroot . '/question/format.php'); was removed from moodle/question/format/xml/format.php and as a result the Manage Questions Types is now broken if you have third party question types installed with import_from_xml / export_to_xml functions (for instance all OU questions types from Tim's git repo like ddwtos gapselect, and also mine ) . Is this change necessary ? if yes what is the recommanded fix for questions types authors ?
          Hide
          Rajesh Taneja added a comment -

          Hello Jean,

          Adding "require_once($CFG->dirroot . '/question/format.php');" to file where import_from_xml/export_to_xml is defined should solve the problem.

          Removal of "require_once($CFG->dirroot . '/question/format.php');" was necessary, as we have two different definitions of "qformat_default" classes, one in mod/lesson/format.php and other in Question/format.php.
          qformat_xml and other formats extends qformat_default. Including "/question/format.php" in "question/format/xml/format.php" forces xml to extend qformat_default class in question/format.php, hence was breaking the lesson question import functionality.

          Hope this explanation helps. Let us know if we can be of any other help.

          Regards
          Rajesh

          Show
          Rajesh Taneja added a comment - Hello Jean, Adding "require_once($CFG->dirroot . '/question/format.php');" to file where import_from_xml/export_to_xml is defined should solve the problem. Removal of "require_once($CFG->dirroot . '/question/format.php');" was necessary, as we have two different definitions of "qformat_default" classes, one in mod/lesson/format.php and other in Question/format.php. qformat_xml and other formats extends qformat_default. Including "/question/format.php" in "question/format/xml/format.php" forces xml to extend qformat_default class in question/format.php, hence was breaking the lesson question import functionality. Hope this explanation helps. Let us know if we can be of any other help. Regards Rajesh
          Hide
          Michael de Raadt added a comment -

          Would this work as a general solution, Rajesh?

          We need to ensure that contributed question types can still be added.

          Show
          Michael de Raadt added a comment - Would this work as a general solution, Rajesh? We need to ensure that contributed question types can still be added.
          Hide
          Rajesh Taneja added a comment -

          Yes Michael, the solution explained to Jean will always work.

          including "question/format.php" explicitly in contributed question types, will ensure that question format always extend qformat_default class from question/format.php.

          FYI:
          This will not break anything under questions plugin, When we rewrite lesson plugin.

          Show
          Rajesh Taneja added a comment - Yes Michael, the solution explained to Jean will always work. including "question/format.php" explicitly in contributed question types, will ensure that question format always extend qformat_default class from question/format.php. FYI: This will not break anything under questions plugin, When we rewrite lesson plugin.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Rajesh, thanks a lot for your explanations. I was unaware of the problem with mod/lesson/format.php.
          A small question for the future : does that means that when lesson will be rewritten it will not be possible to use third party qtypes that have import_from_xml / export_to_xml functions in lessons ?
          Please note that I am not familiar with the lesson mod and the planned functionnalities of the rewrite so maybe your sentence "This will not break anything under questions plugin, When we rewrite lesson plugin" is just the answer to this question ? Regards. Jean-Michel

          Show
          Jean-Michel Vedrine added a comment - Hello Rajesh, thanks a lot for your explanations. I was unaware of the problem with mod/lesson/format.php. A small question for the future : does that means that when lesson will be rewritten it will not be possible to use third party qtypes that have import_from_xml / export_to_xml functions in lessons ? Please note that I am not familiar with the lesson mod and the planned functionnalities of the rewrite so maybe your sentence "This will not break anything under questions plugin, When we rewrite lesson plugin" is just the answer to this question ? Regards. Jean-Michel
          Hide
          Rajesh Taneja added a comment -

          Hello Jean,

          It is too early for me to say anything on lesson rewrite.

          Last what I heard was lesson needs a rewrite and lesson should use questions from question bank.
          Currently, lesson has it's own question bank and doesn't use questions from question bank.
          This is the reason why we have overlaps in lesson and question.

          Saying "This will not break anything under questions plugin, When we rewrite lesson plugin" I mean,
          as we are explicitly setting the base class to be used from question/format.php, we avoid any breaking of code, even if we plan to keep same base class in lesson.

          Hope this helps

          Show
          Rajesh Taneja added a comment - Hello Jean, It is too early for me to say anything on lesson rewrite. Last what I heard was lesson needs a rewrite and lesson should use questions from question bank. Currently, lesson has it's own question bank and doesn't use questions from question bank. This is the reason why we have overlaps in lesson and question. Saying "This will not break anything under questions plugin, When we rewrite lesson plugin" I mean, as we are explicitly setting the base class to be used from question/format.php, we avoid any breaking of code, even if we plan to keep same base class in lesson. Hope this helps
          Hide
          Tim Hunt added a comment -

          It's ugly, but the least-bad hack I can think of to resolve this at the moment is to add at the top of question/format/xml/format.php

          if (!class_exists('qformat_default'))

          { require_once($CFG->dirroot . '/question/format.php'); }
          Show
          Tim Hunt added a comment - It's ugly, but the least-bad hack I can think of to resolve this at the moment is to add at the top of question/format/xml/format.php if (!class_exists('qformat_default')) { require_once($CFG->dirroot . '/question/format.php'); }
          Hide
          Rajesh Taneja added a comment -

          I agree with Tim

          Show
          Rajesh Taneja added a comment - I agree with Tim
          Hide
          Tim Hunt added a comment -

          I just created MDL-28049 to track the regression.

          Show
          Tim Hunt added a comment - I just created MDL-28049 to track the regression.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: