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

make_categories_list() is not letting itself use cached category list

    Details

    • Testing Instructions:
      Hide

      For this test you need to login in two browsers as different users (it must be a consistent session)
      Set $CFG->frontpage to display combo list

      1. Window1: login as user who can create course categories, create some categories
      2. Window2: login as user who can not see hidden categories (just ordinary student), click on any course category, make sure the dropdown with the list of categories on the top of the page is correct
      3. Window1: add new category
      4. Window2: make sure the list of categories is updated
      5. Window1: hide any category
      6. Window2: make sure the list of categories is updated
      Show
      For this test you need to login in two browsers as different users (it must be a consistent session) Set $CFG->frontpage to display combo list Window1: login as user who can create course categories, create some categories Window2: login as user who can not see hidden categories (just ordinary student), click on any course category, make sure the dropdown with the list of categories on the top of the page is correct Window1: add new category Window2: make sure the list of categories is updated Window1: hide any category Window2: make sure the list of categories is updated
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      mdl26dev-MDL-40130-let-course-category-caching-be-used

      Description

      In lib/coursecatlib.php, in the function make_categories_list:

              $baselist = $coursecatcache->get($basecachekey);
              if ($baselist !== false) {
                  $baselist = false;
              }
      

      This odd code results in the category list being cached later in the function, but never used.

      On a Moodle with 1315 course categories, /course/index.php?id=x consistently results in about 2700 database queries.

      If this is changed to:

              if (!$baselist = $coursecatcache->get($basecachekey)) {
                  $baselist = false;
              }
      

      Then the same page consistently makes about 100 database queries (at least when the list is still cached).

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              brianking Brian King added a comment -

              Hi Marina,

              I see it was your commit that introduced this code. Is the category cache intentionally not being used for some reason, or was this just a mistake?

              Cheers,
              Brian

              Show
              brianking Brian King added a comment - Hi Marina, I see it was your commit that introduced this code. Is the category cache intentionally not being used for some reason, or was this just a mistake? Cheers, Brian
              Hide
              salvetore Michael de Raadt added a comment -

              Thanks for raising that, Brian. Hopefully we can get it peer reviewed soon.

              Show
              salvetore Michael de Raadt added a comment - Thanks for raising that, Brian. Hopefully we can get it peer reviewed soon.
              Hide
              marina Marina Glancy added a comment -

              Thanks a lot Brian for spotting this. I'm sure it's enough just to remove code:
              if ($baselist !== false) {
              $baselist = false;
              }
              It is still possible that all categories are hidden and the baselist for user is an empty array.
              This looks like some test code that was committed by mistake.

              (sorry for spam, accidentally removed comment)

              Show
              marina Marina Glancy added a comment - Thanks a lot Brian for spotting this. I'm sure it's enough just to remove code: if ($baselist !== false) { $baselist = false; } It is still possible that all categories are hidden and the baselist for user is an empty array. This looks like some test code that was committed by mistake. (sorry for spam, accidentally removed comment)
              Hide
              brianking Brian King added a comment -

              OK, I've provided a new branch that just removes

                      if ($baselist !== false) {
                          $baselist = false;
                      }
              

              Cheers

              Show
              brianking Brian King added a comment - OK, I've provided a new branch that just removes if ($baselist !== false) { $baselist = false; } Cheers
              Hide
              marina Marina Glancy added a comment -

              Thank you Brian, submitting for integration

              Show
              marina Marina Glancy added a comment - Thank you Brian, submitting for integration
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Good spotting thanks Brian, this has been integrated now.

              Show
              samhemelryk Sam Hemelryk added a comment - Good spotting thanks Brian, this has been integrated now.
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Tested and passed, thanks guys

              Show
              samhemelryk Sam Hemelryk added a comment - Tested and passed, thanks guys
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Thanks for giving me joys and smiles
              Thanks for sharing my trouble's pile

              Thanks for wipeing the tears of my eye
              Thanks for showing me the glad view of sky

              Thanks for lending me your shoulders to lean
              Thanks for giving my words a proper mean

              Thanks for telling me the value of life
              Thanks for showing me the rules to survive

              Thanks for lending me the sympathetic ears
              Thanks for showing how much you care

              From all this what I mean in the end
              Is thanks for being my special friend.

              – Seema Chowdhury

              Sent upstream so... closing, thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Thanks for giving me joys and smiles Thanks for sharing my trouble's pile Thanks for wipeing the tears of my eye Thanks for showing me the glad view of sky Thanks for lending me your shoulders to lean Thanks for giving my words a proper mean Thanks for telling me the value of life Thanks for showing me the rules to survive Thanks for lending me the sympathetic ears Thanks for showing how much you care From all this what I mean in the end Is thanks for being my special friend. – Seema Chowdhury Sent upstream so... closing, thanks!

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    8/Jul/13