Moodle
  1. Moodle
  2. MDL-37642

condition_info performance warning displayed after creating course.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.4
    • Fix Version/s: 2.3.5
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      0. In system settings ensure debugging is set to DEBUG_DEVELOPER and debug messages are displayed.

      1. In site admin menu, choose Courses / Add/edit courses.
      2. Click 'Add a new course'
      3. Type junk for full name and shortname and leave all other options default, then click save changes.
      4. Click link in breadcrumb to course main page.

      EXPECTED: Course displays with no warnings.
      BEFORE FIX: A warning (as description) appears next to each section. (This only appears the first time. If you reload it will go away.)

      Show
      0. In system settings ensure debugging is set to DEBUG_DEVELOPER and debug messages are displayed. 1. In site admin menu, choose Courses / Add/edit courses. 2. Click 'Add a new course' 3. Type junk for full name and shortname and leave all other options default, then click save changes. 4. Click link in breadcrumb to course main page. EXPECTED: Course displays with no warnings. BEFORE FIX: A warning (as description) appears next to each section. (This only appears the first time. If you reload it will go away.)
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Rank:
      47335

      Description

      1. Create a course in a Moodle site running integration 2.3.
      2. After creating the course click on the course link in the navigation bar.
      3. You should be given the following warnings -
      Notice: Undefined property: stdClass::$id in course/lib.php on line 2851
      Performance warning: condition_info constructor is faster if you pass in a $item from get_fast_modinfo or the equivalent for sections. [This warning can be disabled, see phpdoc.]
      
          line 417 of /lib/conditionlib.php: call to debugging()
          line 187 of /lib/conditionlib.php: call to condition_info_base->__construct()
          line 419 of /course/format/renderer.php: call to condition_info_section->__construct()
          line 179 of /course/format/renderer.php: call to format_section_renderer_base->section_availability_message()
          line 720 of /course/format/renderer.php: call to format_section_renderer_base->section_header()
          line 45 of /course/format/weeks/format.php: call to format_section_renderer_base->print_multiple_section_page()
          line 281 of /course/view.php: call to require()

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          The issue should be caused if the section object passed to that constructor is not a section_info object.

          The initial investigation for this would be to trace back through the code using the above stack trace and find out where it gets the section object from. The right way is $modinfo->get_section_info or $modinfo->get_section_info_all or some function that comes through to call one of those.

          I don't have time to check on this today, does anybody else? I can look at it in a few days maybe.

          Show
          Sam Marshall added a comment - The issue should be caused if the section object passed to that constructor is not a section_info object. The initial investigation for this would be to trace back through the code using the above stack trace and find out where it gets the section object from. The right way is $modinfo->get_section_info or $modinfo->get_section_info_all or some function that comes through to call one of those. I don't have time to check on this today, does anybody else? I can look at it in a few days maybe.
          Hide
          Mark Nelson added a comment -

          Hi Marina, I experienced this issue again when testing integration 2.3 which brought me back to this tracker issue. I am adding you to this as you have experience with the course formats. Are you able to take a look? If so, that would be great! Thanks.

          Show
          Mark Nelson added a comment - Hi Marina, I experienced this issue again when testing integration 2.3 which brought me back to this tracker issue. I am adding you to this as you have experience with the course formats. Are you able to take a look? If so, that would be great! Thanks.
          Hide
          Sam Marshall added a comment -

          OK I have tracked this down. The problem is in line 689 of course/format/renderer.php where it calls get_course_section. This returns a plain object (as documented in the code).

          I don't think this is an actual performance problem (i.e. if we have to make a query for each section on every course view that would be bad but if we only do it on the initial one, who cares) so to address it in 2.3 only, let's just make it stop the warning appearing in this case. Working on patch.

          Show
          Sam Marshall added a comment - OK I have tracked this down. The problem is in line 689 of course/format/renderer.php where it calls get_course_section. This returns a plain object (as documented in the code). I don't think this is an actual performance problem (i.e. if we have to make a query for each section on every course view that would be bad but if we only do it on the initial one, who cares) so to address it in 2.3 only, let's just make it stop the warning appearing in this case. Working on patch.
          Hide
          Sam Marshall added a comment -

          Here's a patch to fix it, also testing instructions.

          Show
          Sam Marshall added a comment - Here's a patch to fix it, also testing instructions.
          Hide
          Sam Marshall added a comment -

          Requesting peer review for this trivial fix. Note: I think (somebody correct if I'm wrong) this only applies to 2.3 as the course format logic was changed in 2.4.

          Show
          Sam Marshall added a comment - Requesting peer review for this trivial fix. Note: I think (somebody correct if I'm wrong) this only applies to 2.3 as the course format logic was changed in 2.4.
          Hide
          Mark Nelson added a comment -

          Hi Sam, thanks for the patch. I will review this early next week and will confirm whether or not this should be applied to 2.4 and master.

          Show
          Mark Nelson added a comment - Hi Sam, thanks for the patch. I will review this early next week and will confirm whether or not this should be applied to 2.4 and master.
          Hide
          Mark Nelson added a comment -

          Hi Sam, patch works perfectly. Thanks. Submitting to integration.

          Show
          Mark Nelson added a comment - Hi Sam, patch works perfectly. Thanks. Submitting to integration.
          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 (23 only), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23 only), thanks!
          Hide
          Ankit Agarwal added a comment - - edited

          I remember this error. It is not happening any more after the fix. But am getting following error now while creating a new course

          Notice: Undefined property: stdClass::$id in /var/www/int/23/moodle/course/lib.php on line 2851 Call Stack: 0.0008 806472
          1. {main}() /var/www/int/23/moodle/course/view.php:0 0.1958 44055336
          2. core_renderer->header() /var/www/int/23/moodle/course/view.php:232 0.2066 44913064
          3. core_renderer->render_page_layout() /var/www/int/23/moodle/lib/outputrenderers.php:715 0.2070 45019464
          4. include('/var/www/int/23/moodle/theme/base/layout/general.php') /var/www/int/23/moodle/lib/outputrenderers.php:768 0.6444 81721856
          5. block_manager->region_has_content() /var/www/int/23/moodle/theme/base/layout/general.php:7 0.6444 81721856
          6. block_manager->ensure_content_created() /var/www/int/23/moodle/lib/blocklib.php:352 0.6444 81722168
          7. block_manager->create_block_contents() /var/www/int/23/moodle/lib/blocklib.php:989 0.6459 81756632
          8. block_base->get_content_for_output() /var/www/int/23/moodle/lib/blocklib.php:937 0.6459 81759480
          9. block_base->formatted_contents() /var/www/int/23/moodle/blocks/moodleblock.class.php:232 0.6459 81759480
          10. block_news_items->get_content() /var/www/int/23/moodle/blocks/moodleblock.class.php:281 0.6460 81760664
          11. forum_get_course_forum() /var/www/int/23/moodle/blocks/news_items/block_news_items.php:30 0.7696 81930600
          12. add_mod_to_section() /var/www/int/23/moodle/mod/forum/lib.php:3029 
          

          The error goes away after a page refresh.

          Sorry failing this issue.

          Edited: (Eloy) reformatting backtrace.

          Show
          Ankit Agarwal added a comment - - edited I remember this error. It is not happening any more after the fix. But am getting following error now while creating a new course Notice: Undefined property: stdClass::$id in /var/www/int/23/moodle/course/lib.php on line 2851 Call Stack: 0.0008 806472 1. {main}() /var/www/int/23/moodle/course/view.php:0 0.1958 44055336 2. core_renderer->header() /var/www/int/23/moodle/course/view.php:232 0.2066 44913064 3. core_renderer->render_page_layout() /var/www/int/23/moodle/lib/outputrenderers.php:715 0.2070 45019464 4. include('/var/www/int/23/moodle/theme/base/layout/general.php') /var/www/int/23/moodle/lib/outputrenderers.php:768 0.6444 81721856 5. block_manager->region_has_content() /var/www/int/23/moodle/theme/base/layout/general.php:7 0.6444 81721856 6. block_manager->ensure_content_created() /var/www/int/23/moodle/lib/blocklib.php:352 0.6444 81722168 7. block_manager->create_block_contents() /var/www/int/23/moodle/lib/blocklib.php:989 0.6459 81756632 8. block_base->get_content_for_output() /var/www/int/23/moodle/lib/blocklib.php:937 0.6459 81759480 9. block_base->formatted_contents() /var/www/int/23/moodle/blocks/moodleblock.class.php:232 0.6459 81759480 10. block_news_items->get_content() /var/www/int/23/moodle/blocks/moodleblock.class.php:281 0.6460 81760664 11. forum_get_course_forum() /var/www/int/23/moodle/blocks/news_items/block_news_items.php:30 0.7696 81930600 12. add_mod_to_section() /var/www/int/23/moodle/mod/forum/lib.php:3029 The error goes away after a page refresh. Sorry failing this issue. Edited: (Eloy) reformatting backtrace.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ah, Ankit,

          I bet that error is caused because a really nasty regression detected recently for 23_STABLE. See MDL-38173, at the time of writing this it's still not integrated. And it fixes the exactly offending line:

          @@ -2848,7 +2848,7 @@ function add_mod_to_section($mod, $beforemod=NULL) {
                   }
           
                   $DB->set_field("course_sections", "sequence", $newsequence, array("id"=>$section->id));
          -        $DB->set_field("course_modules", "section", $section->id, array("id" => $mod->id));
          +        $DB->set_field("course_modules", "section", $section->id, array("id" => $mod->coursemodule));
          

          Perhaps you could try testing this with that change applied manually there? Or, alternatively, wait for MDL-38173 to land and then retest properly. I'll ping here once that one gets integrated.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ah, Ankit, I bet that error is caused because a really nasty regression detected recently for 23_STABLE. See MDL-38173 , at the time of writing this it's still not integrated. And it fixes the exactly offending line: @@ -2848,7 +2848,7 @@ function add_mod_to_section($mod, $beforemod=NULL) { } $DB->set_field( "course_sections" , "sequence" , $newsequence, array( "id" =>$section->id)); - $DB->set_field( "course_modules" , "section" , $section->id, array( "id" => $mod->id)); + $DB->set_field( "course_modules" , "section" , $section->id, array( "id" => $mod->coursemodule)); Perhaps you could try testing this with that change applied manually there? Or, alternatively, wait for MDL-38173 to land and then retest properly. I'll ping here once that one gets integrated. Ciao
          Hide
          Damyon Wiese added a comment -

          I can confirm that MDL-38173 does fix a warning when viewing a new course (it happens when it adds the latest news forum).

          Show
          Damyon Wiese added a comment - I can confirm that MDL-38173 does fix a warning when viewing a new course (it happens when it adds the latest news forum).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          MDL-38173 has been integrated to I'm reseting this to another testing round. The "Undefined property" shouldn't be happening anymore (fingers crossed).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - MDL-38173 has been integrated to I'm reseting this to another testing round. The "Undefined property" shouldn't be happening anymore (fingers crossed). Ciao
          Hide
          Ankit Agarwal added a comment -

          Works as described now.
          Both errors are fixed now.
          Passing
          Thanks

          Show
          Ankit Agarwal added a comment - Works as described now. Both errors are fixed now. Passing Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

          Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

          Thanks, closing as fixed!

          Show
          Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: