Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-27483

Quiz repaginate enter in a endless loop

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.1, 2.2
    • Fix Version/s: 2.0.4, 2.1.1
    • Component/s: Quiz
    • Labels:
    • Environment:
      Moodle 2.0.1 - Moodle 2.0.3
    • Database:
      Any
    • Testing Instructions:
      Hide

      1. Create a quiz and turning variable layout/Question order in "shuffled randomly" before adding any questions.
      2. Edit the quiz settings, change the number of questions per page, tick the repaginate now box, then save changes.

      Show
      1. Create a quiz and turning variable layout/Question order in "shuffled randomly" before adding any questions. 2. Edit the quiz settings, change the number of questions per page, tick the repaginate now box, then save changes.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      A quiz instance without questions and variable layout/Question order turned in "shuffled randomly" creates an endless loop in mod/quiz/locallib.php line 348.

      To make a quick fix i made this:

      function quiz_repaginate($layout, $perpage, $shuffle = false) {
      $layout = str_replace(',0', '', $layout); // remove existing page breaks
      $questions = explode(',', $layout);
      //remove empty pages from beginning

      if(count($questions)!==1){ //only paginate if there is at least one question
      while (reset($questions) == '0')

      { array_shift($questions); }

      if ($shuffle)

      { shuffle($questions); }

      $i = 1;
      $layout = '';
      foreach ($questions as $question) {
      if ($perpage and $i > $perpage)

      { $layout .= '0,'; $i = 1; }

      $layout .= $question.',';
      $i++;
      }
      return $layout.'0';
      }
      }

        Gliffy Diagrams

        1. MDL-27483.patch
          0.3 kB
          Dariem Garces (Demian)

          Activity

          Hide
          dougiamas Martin Dougiamas added a comment -

          Thanks Dariem. Could you please post fixes as patch files (diff) ... it makes it much easier to see the fix.

          Show
          dougiamas Martin Dougiamas added a comment - Thanks Dariem. Could you please post fixes as patch files (diff) ... it makes it much easier to see the fix.
          Hide
          darkath Dariem Garces (Demian) added a comment - - edited

          Quick fix in the attached patch

          Show
          darkath Dariem Garces (Demian) added a comment - - edited Quick fix in the attached patch
          Hide
          timhunt Tim Hunt added a comment -

          Thank you for your contribution to Moodle. However:

          1. This function really should have unit tests. (Most of the other functions that do things to the quiz layout already do.)

          2. I don't think your change is the best way to fix this. It is simpler and more robust to just change the == to === in the while.

          3. Please use 'unified' diffs. They are much more human-readable, and also much more likely to apply in future if other changes are made to the code. (http://docs.moodle.org/en/Development:How_to_create_a_patch)

          4. The white-space in your patch is almost entirely wrong. Please see http://docs.moodle.org/en/Development:Coding_style#If_.2F_else

          Show
          timhunt Tim Hunt added a comment - Thank you for your contribution to Moodle. However: 1. This function really should have unit tests. (Most of the other functions that do things to the quiz layout already do.) 2. I don't think your change is the best way to fix this. It is simpler and more robust to just change the == to === in the while. 3. Please use 'unified' diffs. They are much more human-readable, and also much more likely to apply in future if other changes are made to the code. ( http://docs.moodle.org/en/Development:How_to_create_a_patch ) 4. The white-space in your patch is almost entirely wrong. Please see http://docs.moodle.org/en/Development:Coding_style#If_.2F_else
          Hide
          darkath Dariem Garces (Demian) added a comment -

          Thanks for the tips Tim.

          Show
          darkath Dariem Garces (Demian) added a comment - Thanks for the tips Tim.
          Hide
          tlevi Tony Levi added a comment -

          At a guess from code, 2.1 has missed out on this problem (it should be checked!), but 20_STABLE still needs a fix committed.
          This is our patch for the issue: https://github.com/tlevi/moodle/tree/mdl27483

          When there are no elements, reset is returning false, and (false == '0') is true in php, so the loop continues.
          Just making the compare strict (===) solves this.

          Show
          tlevi Tony Levi added a comment - At a guess from code, 2.1 has missed out on this problem (it should be checked!), but 20_STABLE still needs a fix committed. This is our patch for the issue: https://github.com/tlevi/moodle/tree/mdl27483 When there are no elements, reset is returning false, and (false == '0') is true in php, so the loop continues. Just making the compare strict (===) solves this.
          Hide
          timhunt Tim Hunt added a comment -

          Thank you for diagnosing this and putting the fix on github.

          Although the 2.1 code is probably OK, I think a could of quiz_clean_layout calls would make it more robust, so I am making some changes there.

          Integrators: Please cherry-pick the master change to 2.1, as well as merging the indicated changes to 2.0 and master. Thanks.

          Show
          timhunt Tim Hunt added a comment - Thank you for diagnosing this and putting the fix on github. Although the 2.1 code is probably OK, I think a could of quiz_clean_layout calls would make it more robust, so I am making some changes there. Integrators: Please cherry-pick the master change to 2.1, as well as merging the indicated changes to 2.0 and master. Thanks.
          Hide
          stronk7 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
          stronk7 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
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks (master and 20 merged. 21 cherry picked from master).

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks (master and 20 merged. 21 cherry picked from master).
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Congratulations - this fix has just been released in the weeklies.

          Show
          samhemelryk Sam Hemelryk added a comment - Congratulations - this fix has just been released in the weeklies.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                1/Aug/11