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

          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