Moodle
  1. Moodle
  2. MDL-27483

Quiz repaginate enter in a endless loop

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      17142

      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';
      }
      }

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

        Activity

        Hide
        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
        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
        Dariem Garces (Demian) added a comment - - edited

        Quick fix in the attached patch

        Show
        Dariem Garces (Demian) added a comment - - edited Quick fix in the attached patch
        Hide
        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
        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
        Dariem Garces (Demian) added a comment -

        Thanks for the tips Tim.

        Show
        Dariem Garces (Demian) added a comment - Thanks for the tips Tim.
        Hide
        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
        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
        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
        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
        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
        Eloy Lafuente (stronk7) added a comment -

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

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

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

        Show
        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: