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

      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()

        Gliffy Diagrams

          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: