Moodle
  1. Moodle
  2. MDL-39488

Match question throws PHP error if one or more options have not been selected

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.6, 2.4, 2.5
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Lesson
    • Labels:
      None
    • Rank:
      50156

      Description

      In Moodle 2, with debug mode ON, a Match question throws PHP error if one or more options have not been selected:
      Notice: Undefined index: in moodle\mod\lesson\pagetypes\matching.php on line 188

      Plus: if NO options selected at all, the "No answer given. Please go back and submit an answer." never gets displayed.

      I will submit an easy fix on my github.

        Issue Links

          Activity

          Hide
          Rossiani Wijaya added a comment -

          Hi Joseph,

          Thank you for creating patch for this issue.

          I simplified your patch a little bit to check all empty responses, instead of just checking for the first response. I also updated the noanswer string value to 'One or more questions have no answer given. Please go back and submit an answer'.

          Please feel free to leave any feedback.

          Show
          Rossiani Wijaya added a comment - Hi Joseph, Thank you for creating patch for this issue. I simplified your patch a little bit to check all empty responses, instead of just checking for the first response. I also updated the noanswer string value to 'One or more questions have no answer given. Please go back and submit an answer'. Please feel free to leave any feedback.
          Hide
          Joseph Rézeau added a comment -

          Tested, looks OK to me. Thanks,
          Joseph

          Show
          Joseph Rézeau added a comment - Tested, looks OK to me. Thanks, Joseph
          Hide
          Rossiani Wijaya added a comment -

          Will create patch for 2.4 and 2.3 once it passed peer-review.

          Show
          Rossiani Wijaya added a comment - Will create patch for 2.4 and 2.3 once it passed peer-review.
          Hide
          Ankit Agarwal added a comment - - edited

          Hi guys,
          Am not 100% sure about the patch. All in all it looks good however there a few things to investigate/consider:-

          1. old code assumed $response could or could not be an array, is that situation completely obsolete now?. I don't really know how it could be anything besides array earlier so cannot comment on it.
          2. There is a change in behaviour here we are introducing, now partially submitted answers are not accepted at all, in comparison to previous behaviour which was partially submitted answer was marked as wrong. I will leave it to Rosie to decide if it is okay or not as she is the lesson lead. Although it needs to be documented properly.
          3. Something else to consider is merging two foreach loops together, which loops through $response, will improve performance. But am okay if you think separate loops are better for an early exit.
          4. Also something I noticed (not related to patch). why are we doing:-
                    foreach ($answers as $key=>$answer) {
                        if ($answer->answer !== '' or $answer->response !== '') {
                            $answers[$answer->id] = $answer;
                        }
                        unset($answers[$key]);
                    }
            

            This part

            if ($answer->answer !== '' or $answer->response !== '') {
            

            Don't we trust the data in database? if we dont still I feel that should be part of get_answers().
            Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi guys, Am not 100% sure about the patch. All in all it looks good however there a few things to investigate/consider:- old code assumed $response could or could not be an array, is that situation completely obsolete now?. I don't really know how it could be anything besides array earlier so cannot comment on it. There is a change in behaviour here we are introducing, now partially submitted answers are not accepted at all, in comparison to previous behaviour which was partially submitted answer was marked as wrong. I will leave it to Rosie to decide if it is okay or not as she is the lesson lead. Although it needs to be documented properly. Something else to consider is merging two foreach loops together, which loops through $response, will improve performance. But am okay if you think separate loops are better for an early exit. Also something I noticed (not related to patch). why are we doing:- foreach ($answers as $key=>$answer) { if ($answer->answer !== '' or $answer->response !== '') { $answers[$answer->id] = $answer; } unset($answers[$key]); } This part if ($answer->answer !== '' or $answer->response !== '') { Don't we trust the data in database? if we dont still I feel that should be part of get_answers(). Thanks
          Hide
          Joseph Rézeau added a comment -

          @Ankit,
          1.- No idea why old code was testing whether $response could be an array or not. Weird.
          2.- In my original patch, I suggested retaining the possibility for the student to submit an incomplete answer, i.e. to match some pairs but not all. But, I think it makes sense to "force" the student to submit complete matches and agree with the patch proposed by Rossiani.
          3 & 4 : I don't know.

          Show
          Joseph Rézeau added a comment - @Ankit, 1.- No idea why old code was testing whether $response could be an array or not. Weird. 2.- In my original patch, I suggested retaining the possibility for the student to submit an incomplete answer, i.e. to match some pairs but not all. But, I think it makes sense to "force" the student to submit complete matches and agree with the patch proposed by Rossiani. 3 & 4 : I don't know.
          Hide
          Rossiani Wijaya added a comment -

          Hi Ankit,

          1. $response should be an array. I also don't know why the old code testing it there.
          2. I think it is better to force student to answer all matching question instead of just marking it wrong. This will also provide a warning for student instead of just marking it as wrong.
          3. After some consideration, I decided to merge the two loops together, although it is better to exit earlier. But looking at the code, I think it will simplified the code.
          4. If I recall correctly, $answer->response !== '' was part of fixing the report page. After looking at the code and performed some testing, I found a bug on creating matching pair. when the answer is provided without the matches answer, it gets inserted to DB but didn't get displayed when the user taking the lesson. Then when viewing student's detailed reports, it produced undefined offset error. Create new issue to fix this (MDL-39560).

          Patch updated.

          Sending for integration review.

          Show
          Rossiani Wijaya added a comment - Hi Ankit, 1. $response should be an array. I also don't know why the old code testing it there. 2. I think it is better to force student to answer all matching question instead of just marking it wrong. This will also provide a warning for student instead of just marking it as wrong. 3. After some consideration, I decided to merge the two loops together, although it is better to exit earlier. But looking at the code, I think it will simplified the code. 4. If I recall correctly, $answer->response !== '' was part of fixing the report page. After looking at the code and performed some testing, I found a bug on creating matching pair. when the answer is provided without the matches answer, it gets inserted to DB but didn't get displayed when the user taking the lesson. Then when viewing student's detailed reports, it produced undefined offset error. Create new issue to fix this ( MDL-39560 ). Patch updated. Sending for integration review.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Rossie, just a tiny comment... around mod/lesson/pagetypes/matching.php #177,

          are you sure that "$value == ''" (non strict) is the comparison you want to perform? Note that all these: null, false, 0 also fulfill that comparison. I don't know if in this case any of this values is sensible/possible there, but I prefer you to verify it before integrating this.

          So holding until getting a confirmation / change (if needed).

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Rossie, just a tiny comment... around mod/lesson/pagetypes/matching.php #177, are you sure that "$value == ''" (non strict) is the comparison you want to perform? Note that all these: null, false, 0 also fulfill that comparison. I don't know if in this case any of this values is sensible/possible there, but I prefer you to verify it before integrating this. So holding until getting a confirmation / change (if needed). TIA and ciao
          Hide
          Rossiani Wijaya added a comment -

          Hi Eloy,

          Yes - null, false or 0 are the intended comparisons. So if one of those values are given, it should trigger the if statement.

          Show
          Rossiani Wijaya added a comment - Hi Eloy, Yes - null, false or 0 are the intended comparisons. So if one of those values are given, it should trigger the if statement.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Perfect, thanks for verifying!

          Show
          Eloy Lafuente (stronk7) added a comment - Perfect, thanks for verifying!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23, 24 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 & master), thanks!
          Hide
          Sam Hemelryk added a comment -

          Hmm in two minds as to whether to fail this, or pass it and open a new issue.
          However as the error I am seeing is on the same area (very close proximity) of code that the error in this issues description I think I will fail and we can discuss it.

          Running through the test instructions everything passes as expected.
          I also tryed the following and got errors however:

          1. Preview the question.
          2. Select the same response for every item.
          3. Click submit.

          Doing this I a pair of notices for each option in the question:

          Notice: Undefined index: 3 in /var/www/integration/mod/lesson/pagetypes/matching.php on line 187
          Call Stack
          #	Time	Memory	Function	Location
          1	0.0005	767104	{main}( )	../continue.php:0
          2	0.2667	47899368	lesson_page->record_attempt( )	../continue.php:79
          3	0.2667	47899368	lesson_page_type_matching->check_answer( )	../locallib.php:1927
          
          ( ! ) Notice: Trying to get property of non-object in /var/www/integration/mod/lesson/pagetypes/matching.php on line 187
          Call Stack
          #	Time	Memory	Function	Location
          1	0.0005	767104	{main}( )	../continue.php:0
          2	0.2667	47899368	lesson_page->record_attempt( )	../continue.php:79
          3	0.2667	47899368	lesson_page_type_matching->check_answer( )	../locallib.php:1927
          
          Show
          Sam Hemelryk added a comment - Hmm in two minds as to whether to fail this, or pass it and open a new issue. However as the error I am seeing is on the same area (very close proximity) of code that the error in this issues description I think I will fail and we can discuss it. Running through the test instructions everything passes as expected. I also tryed the following and got errors however: Preview the question. Select the same response for every item. Click submit. Doing this I a pair of notices for each option in the question: Notice: Undefined index: 3 in / var /www/integration/mod/lesson/pagetypes/matching.php on line 187 Call Stack # Time Memory Function Location 1 0.0005 767104 {main}( ) ../ continue .php:0 2 0.2667 47899368 lesson_page->record_attempt( ) ../ continue .php:79 3 0.2667 47899368 lesson_page_type_matching->check_answer( ) ../locallib.php:1927 ( ! ) Notice: Trying to get property of non-object in / var /www/integration/mod/lesson/pagetypes/matching.php on line 187 Call Stack # Time Memory Function Location 1 0.0005 767104 {main}( ) ../ continue .php:0 2 0.2667 47899368 lesson_page->record_attempt( ) ../ continue .php:79 3 0.2667 47899368 lesson_page_type_matching->check_answer( ) ../locallib.php:1927
          Hide
          Rossiani Wijaya added a comment -

          Hi Sam,

          Thank you for catching this bug.

          I had discussion with Sam for the above error, as it is a bit tricky to reproduce it. The bug itself was hiding within re-processing of answers variable where the loop may potentially unsetting the wrong array's index.

          Patches updated.

          Show
          Rossiani Wijaya added a comment - Hi Sam, Thank you for catching this bug. I had discussion with Sam for the above error, as it is a bit tricky to reproduce it. The bug itself was hiding within re-processing of answers variable where the loop may potentially unsetting the wrong array's index. Patches updated.
          Hide
          Sam Hemelryk added a comment -

          Thanks Rosie, I've pulled in those changes now

          Show
          Sam Hemelryk added a comment - Thanks Rosie, I've pulled in those changes now
          Hide
          Sam Hemelryk added a comment -

          Tested again and passed. Thanks Rosie!

          Show
          Sam Hemelryk added a comment - Tested again and passed. Thanks Rosie!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Did you think this day was not going to arrive ever?

          Your patience has been rewarded, yay, sent upstream, thanks!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: