Moodle
  1. Moodle
  2. MDL-29511

Coding error: $seq out of range while reviewing individual steps from question response history

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.2
    • Fix Version/s: 2.1.2
    • Component/s: Quiz
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      1. Create a quiz in adaptive behaviour
      2. Attempt the quiz as a student, submitting several different answers as adaptive behaviour allows
      3. Finish the attempt
      4. Switch back to teacher role
      5. Go to the review attempt page
      6. In the history table under the question, click on step 1 to review the question in that state.

      Show
      1. Create a quiz in adaptive behaviour 2. Attempt the quiz as a student, submitting several different answers as adaptive behaviour allows 3. Finish the attempt 4. Switch back to teacher role 5. Go to the review attempt page 6. In the history table under the question, click on step 1 to review the question in that state.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      19219

      Description

      Reviewing attempt steps fails with the following error:

      Coding error detected, it must be fixed by a programmer: $seq out of range

      Enabling PHP errors reveals the following details:

      Notice: Undefined variable: seq in /home/anttix/products/moodle/2.1+/question/engine/questionattempt.php on line 1204 Call Stack: 0.0001 623680 1.

      {main}

      () /home/anttix/products/moodle/2.1+/mod/quiz/reviewquestion.php:0 0.1193 6124912 2. mod_quiz_renderer->review_question_page() /home/anttix/products/moodle/2.1+/mod/quiz/reviewquestion.php:104 0.1659 8530520 3. quiz_attempt->render_question_at_step() /home/anttix/products/moodle/2.1+/mod/quiz/renderer.php:80 0.1660 8530600 4. question_usage_by_activity->render_question_at_step() /home/anttix/products/moodle/2.1+/mod/quiz/attemptlib.php:968 0.1660 8530600 5. question_attempt->render_at_step() /home/anttix/products/moodle/2.1+/question/engine/questionusage.php:375 0.1661 8533704 6. question_attempt->render() /home/anttix/products/moodle/2.1+/question/engine/questionattempt.php:732 0.1668 8546856 7. question_behaviour->render() /home/anttix/products/moodle/2.1+/question/engine/questionattempt.php:702 0.1674 8559760 8. core_question_renderer->question() /home/anttix/products/moodle/2.1+/question/behaviour/behaviourbase.php:127 0.2399 8650240 9. core_question_renderer->response_history() /home/anttix/products/moodle/2.1+/question/engine/renderer.php:92 0.2424 8665024 10. question_attempt_with_restricted_history->__construct() /home/anttix/products/moodle/2.1+/question/engine/renderer.php:387

      Coding error detected, it must be fixed by a programmer: $seq out of range

      More information about this error
      Stack trace:

      line 1204 of /question/engine/questionattempt.php: coding_exception thrown
      line 387 of /question/engine/renderer.php: call to question_attempt_with_restricted_history->__construct()
      line 92 of /question/engine/renderer.php: call to core_question_renderer->response_history()
      line 127 of /question/behaviour/behaviourbase.php: call to core_question_renderer->question()
      line 702 of /question/engine/questionattempt.php: call to question_behaviour->render()
      line 732 of /question/engine/questionattempt.php: call to question_attempt->render()
      line 375 of /question/engine/questionusage.php: call to question_attempt->render_at_step()
      line 968 of /mod/quiz/attemptlib.php: call to question_usage_by_activity->render_question_at_step()
      line 80 of /mod/quiz/renderer.php: call to quiz_attempt->render_question_at_step()
      line 104 of /mod/quiz/reviewquestion.php: call to mod_quiz_renderer->review_question_page()

        Issue Links

          Activity

          Show
          Antti Andreimann added a comment - Reverting the change that was introduced to fix MDL-28679 will cure the problem. http://git.moodle.org/gw?p=moodle.git;a=blobdiff;f=question/engine/renderer.php;h=6f5570b4521db1089ac3083cf61a892de9380a35;hp=9a0d08b90a0904e3e27a9f3dc73136c5dcf1a769;hb=08d1ba7106bff693a0a4ed4db288c07f4cfbe190;hpb=ce0cb2451b6f5299fc191daa44fe1fcddeac2423
          Hide
          Tim Hunt added a comment -

          Yes, but reverting the previous fix is hardly a proper solution to the problem.

          Show
          Tim Hunt added a comment - Yes, but reverting the previous fix is hardly a proper solution to the problem.
          Hide
          Joseph Rézeau added a comment -

          Cannot reproduce this "new" bug on my current moodle 2.1 version.
          I do not understand "STABLE Sprint 13".
          Waiting for more clear instructions from Antti Andreimann.

          Show
          Joseph Rézeau added a comment - Cannot reproduce this "new" bug on my current moodle 2.1 version. I do not understand "STABLE Sprint 13". Waiting for more clear instructions from Antti Andreimann.
          Hide
          Tim Hunt added a comment -

          This is perfectly reproducible Joseph. I just clarified the wording of the testing instructions.

          Show
          Tim Hunt added a comment - This is perfectly reproducible Joseph. I just clarified the wording of the testing instructions.
          Hide
          Tim Hunt added a comment -

          This should be a proper fix.

          Show
          Tim Hunt added a comment - This should be a proper fix.
          Hide
          Antti Andreimann added a comment -

          Tried it, works for me.

          Show
          Antti Andreimann added a comment - Tried it, works for me.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          1) I must confess I'm a bit lost with the question_attempt->get_full_qa() method returning $this. Why is that needed in question_attempt_with_restricted_history constructor and why the old "$this->baseqa = $baseqa;" was invalid?

          2) Also, the name seems a bit incorrect, so you are implementing it to return only the "base" question_attempt, not the "full" one.

          3) Finally, the question_attempt_with_restricted_history->get_full_qa() is used at all?

          I'm sure I'm missing something, 100% sure, I just cannot imagine what, grrr.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited 1) I must confess I'm a bit lost with the question_attempt->get_full_qa() method returning $this. Why is that needed in question_attempt_with_restricted_history constructor and why the old "$this->baseqa = $baseqa;" was invalid? 2) Also, the name seems a bit incorrect, so you are implementing it to return only the "base" question_attempt, not the "full" one. 3) Finally, the question_attempt_with_restricted_history->get_full_qa() is used at all? I'm sure I'm missing something, 100% sure, I just cannot imagine what, grrr. Ciao
          Hide
          Tim Hunt added a comment -

          OK, so a $qa is basically an array of $steps - with a bit of extra data.

          In a few places we need to sort-of go back in time, and pretend that only the first few steps exist. question_attempt_with_restricted_history is an adaptor that does that. It wraps around a $qa and pretends that only some steps exist, but otherwise can be used like a $qa.

          The $qawithrestrictedhistory is based on the real $qa, which I why I called the field $this->baseqa. $this->fullqa might be a better name, I agree (I did not think of it before) but $baseqa was already used, so don't really want to change it now.

          It turns out that in a couple of places (like render_history), it is necessary to be able to get back the full history, whether you start with a $qa or a $qawithrestrictedhistory. For that to work smoothly, you need to implement the method get_full_qa() in the base class (where it does not do anything) and then override it in the subclass.

          To work out why you need to do $this->baseqa = $baseqa->get_full_qa();
          1. Review a quiz attempt, where the quiz was in adaptive mode, and so one of the questions has 5 steps in the question history table. Click on the step 2 link to open the review of the question in the past state in a pop-up window. (That creates question_attempt_with_restricted_history limited to just the few steps). Now think about what it has to do to display the information on the 4th row of the history table in that pop-up window. Starting with the history limited to 2 steps, it has to create a version of the history limited to 4 steps.

          Show
          Tim Hunt added a comment - OK, so a $qa is basically an array of $steps - with a bit of extra data. In a few places we need to sort-of go back in time, and pretend that only the first few steps exist. question_attempt_with_restricted_history is an adaptor that does that. It wraps around a $qa and pretends that only some steps exist, but otherwise can be used like a $qa. The $qawithrestrictedhistory is based on the real $qa, which I why I called the field $this->baseqa. $this->fullqa might be a better name, I agree (I did not think of it before) but $baseqa was already used, so don't really want to change it now. It turns out that in a couple of places (like render_history), it is necessary to be able to get back the full history, whether you start with a $qa or a $qawithrestrictedhistory. For that to work smoothly, you need to implement the method get_full_qa() in the base class (where it does not do anything) and then override it in the subclass. To work out why you need to do $this->baseqa = $baseqa->get_full_qa(); 1. Review a quiz attempt, where the quiz was in adaptive mode, and so one of the questions has 5 steps in the question history table. Click on the step 2 link to open the review of the question in the past state in a pop-up window. (That creates question_attempt_with_restricted_history limited to just the few steps). Now think about what it has to do to display the information on the 4th row of the history table in that pop-up window. Starting with the history limited to 2 steps, it has to create a version of the history limited to 4 steps.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks for the explanations here and over chat, Tim. I think I got it now!

          Basically it is the dual possibility of accessing to the (original) whole history from some parts against both one question_attempt and one question_attempt_with_restricted_history the cause for that method to be overridden, so it is guaranteed that the complete history is returned always, no matter you "chain" restricted history ones.

          I think I can integrate this...doing...

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks for the explanations here and over chat, Tim. I think I got it now! Basically it is the dual possibility of accessing to the (original) whole history from some parts against both one question_attempt and one question_attempt_with_restricted_history the cause for that method to be overridden, so it is guaranteed that the complete history is returned always, no matter you "chain" restricted history ones. I think I can integrate this...doing...
          Hide
          Rajesh Taneja added a comment -

          Works Great
          Thanks for fixing this Tim.

          Show
          Rajesh Taneja added a comment - Works Great Thanks for fixing this Tim.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          git repositories have been updated with your awesome changes, thanks! Closing.

          Show
          Eloy Lafuente (stronk7) added a comment - git repositories have been updated with your awesome changes, thanks! Closing.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: