Moodle
  1. Moodle
  2. MDL-34936

get_fast_modinfo should throw debug warnings about missing course->sectioncache

    Details

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

      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.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          Aparup Banerjee added a comment -

          Thanks, that integrated into 23 and master.

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

          Show
          Aparup Banerjee added a comment - Thanks, that integrated into 23 and master. (chat ref: http://moodle.org/local/chatlogs/index.php?conversationid=10907 )
          Hide
          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
          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
          Andrew Nicols added a comment -

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

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

          lol, i've fixed that up.

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

          works as described.
          Thanks

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

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

          http://youtu.be/n64CdfDRnZY

          Closing as fixed, ciao

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