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

get_fast_modinfo should throw debug warnings about missing course->sectioncache

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1, 2.4
    • Fix Version/s: 2.3.2
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide
      • Ensure that you're enrolled on at least one course
      • View your /my home page
        • Confirm that no errors were shown

      master instructions

      • open /blocks/course_overview/locallib.php in your editor
      • find block_course_overview_get_sorted_courses()
      • remove the 'sectioncache from the enrol_get_my_courses() call
      • refresh your /my page
        • Confirm that a debug message was shown

      MOODLE_23_STABLE instructions

      • open blocks/course_overview/block_course_overview.php in your editor
      • find get_content()
      • remove the 'sectioncache from the enrol_get_my_courses() call
      • refresh your /my page
        • Confirm that a debug message was shown
      Show
      Ensure that you're enrolled on at least one course View your /my home page Confirm that no errors were shown master instructions open /blocks/course_overview/locallib.php in your editor find block_course_overview_get_sorted_courses() remove the 'sectioncache from the enrol_get_my_courses() call refresh your /my page Confirm that a debug message was shown MOODLE_23_STABLE instructions open blocks/course_overview/block_course_overview.php in your editor find get_content() remove the 'sectioncache from the enrol_get_my_courses() call refresh your /my page Confirm that a debug message was shown
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-34936-master-1

      Description

      As an addendum to MDL-34783, I was thinking we should probably warn if the provided course is missing the sectioncache property. This is already the case for the modinfo property.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              dobedobedoh Andrew Nicols added a comment -

              I think that this is worth including in 2.3 because missing sectioncaches can have a huge knock on performance.

              Show
              dobedobedoh Andrew Nicols added a comment - I think that this is worth including in 2.3 because missing sectioncaches can have a huge knock on performance.
              Hide
              fred Frédéric Massart added a comment - - edited

              Hi Andrew,

              thanks for working on this. While reviewing this I wondered about:

              • Shouldn't we set the debugging level to developer when calling debugging(), I guess it could potentially display a lot of notices.
              • It appears that your are displaying the notice when the property is not set to prevent course_modinfo to reload the course in the constructor, but in the later there is a check for sessioncache too, and the reload is forced when either sessioncache or modinfo are empty. Shouldn't we warn the developer when sessioncache is missing as well? (Well, I should have looked at your patch, not at my local file lol...)

              If you feel like this is not appropriate, please feel free to push for integration.

              Cheers \o/

              Edit: I don't seem to be awake, sorry! Could you also add some testing instructions if possible?

              Show
              fred Frédéric Massart added a comment - - edited Hi Andrew, thanks for working on this. While reviewing this I wondered about: Shouldn't we set the debugging level to developer when calling debugging(), I guess it could potentially display a lot of notices. It appears that your are displaying the notice when the property is not set to prevent course_modinfo to reload the course in the constructor, but in the later there is a check for sessioncache too, and the reload is forced when either sessioncache or modinfo are empty. Shouldn't we warn the developer when sessioncache is missing as well? (Well, I should have looked at your patch, not at my local file lol...) If you feel like this is not appropriate, please feel free to push for integration. Cheers \o/ Edit: I don't seem to be awake, sorry! Could you also add some testing instructions if possible?
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Discussed in the Developer chat - debugging() uses debug_normal and we feel that this should be sufficient.
              A broken sectioncache is important enough to let people know as loudly as possible as it has a massively detrimental effect on performance.

              I've rebased on integration branches too (aren't I nice).

              Andrew

              Show
              dobedobedoh Andrew Nicols added a comment - Discussed in the Developer chat - debugging() uses debug_normal and we feel that this should be sufficient. A broken sectioncache is important enough to let people know as loudly as possible as it has a massively detrimental effect on performance. I've rebased on integration branches too (aren't I nice). Andrew
              Hide
              nebgor Aparup Banerjee added a comment -

              Thanks, that integrated into 23 and master.

              (chat ref: http://moodle.org/local/chatlogs/index.php?conversationid=10907)

              Show
              nebgor Aparup Banerjee added a comment - Thanks, that integrated into 23 and master. (chat ref: http://moodle.org/local/chatlogs/index.php?conversationid=10907 )
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Nice one, just served to detect 2 more cases where we were missing "sectioncache".

              See MDL-35089 for more information, yay unit tests coverage!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Nice one, just served to detect 2 more cases where we were missing "sectioncache". See MDL-35089 for more information, yay unit tests coverage!
              Hide
              dobedobedoh Andrew Nicols added a comment -

              I love how this issue apparently 'caused' the regression rather than highlighting it

              Show
              dobedobedoh Andrew Nicols added a comment - I love how this issue apparently 'caused' the regression rather than highlighting it
              Hide
              nebgor Aparup Banerjee added a comment -

              lol, i've fixed that up.

              Show
              nebgor Aparup Banerjee added a comment - lol, i've fixed that up.
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              works as described.
              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - works as described. Thanks
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              I'm so proud...of you, many thanks!

              http://youtu.be/n64CdfDRnZY

              Closing as fixed, ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - I'm so proud...of you, many thanks! http://youtu.be/n64CdfDRnZY Closing as fixed, ciao

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    10/Sep/12