Moodle
  1. Moodle
  2. MDL-39099

Course format protected function cannot be overridden because of private call.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.6, 2.4.3, 2.5
    • Fix Version/s: 2.3.6, 2.4.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      1. Turn on 'Developer' level debugging.
      2. Create a 'Topics' based course with the 'Course layout' formatting option set to 'Show one section per page'.
      3. Copy the method 'section_summary()' contained within '/course/format/renderer.php' and place it within the 'format_topics_renderer' class of '/course/format/topics/renderer.php'.
      4. Go to the course you created.
      5. Observe the error message 'Coding error detected, it must be fixed by a programmer: Unknown method called against format_topics_renderer :: section_activity_summary' displayed.
      6. Apply the patch.
      7. Go back to the course you created, observe that the error message is not displayed and the course renders.
      8. If required, revert the file '/course/format/topics/renderer.php' such that the test code is not integrated.

      Show
      1. Turn on 'Developer' level debugging. 2. Create a 'Topics' based course with the 'Course layout' formatting option set to 'Show one section per page'. 3. Copy the method 'section_summary()' contained within '/course/format/renderer.php' and place it within the 'format_topics_renderer' class of '/course/format/topics/renderer.php'. 4. Go to the course you created. 5. Observe the error message 'Coding error detected, it must be fixed by a programmer: Unknown method called against format_topics_renderer :: section_activity_summary' displayed. 6. Apply the patch. 7. Go back to the course you created, observe that the error message is not displayed and the course renders. 8. If required, revert the file '/course/format/topics/renderer.php' such that the test code is not integrated.
    • Workaround:
      Hide

      Create a new version of 'section_activity_summary()' in the sibling class and have the overridden 'section_summary()' call it instead. But this is substantial duplicated code and the patch is a one word fix.

      Show
      Create a new version of 'section_activity_summary()' in the sibling class and have the overridden 'section_summary()' call it instead. But this is substantial duplicated code and the patch is a one word fix.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      wip-MDL-39099_M23
    • Pull Master Branch:
      wip-MDL-39099_master
    • Rank:
      49242

      Description

      In '/course/format/renderer.php', there is a method which calls a private method:

      protected function section_summary($section, $course, $mods) {
      ...
      $o.= $this->section_activity_summary($section, $course, null);
      

      but because 'section_activity_summary()' is defined as 'private':

      private function section_activity_summary($section, $course, $mods)
      

      a sibling class cannot override the protected method 'section_summary()' say to alter the way the classes are applied to the 'li' tag without resulting in the error:

      Coding error detected, it must be fixed by a programmer: Unknown method called against format_topcoll_renderer :: section_activity_summary

      Therefore 'section_activity_summary()' needs to be changed to:

      protected function section_activity_summary($section, $course, $mods)
      

      To allow the overridden protected method 'section_summary' to call it.

        Issue Links

          Activity

          Hide
          Gareth J Barnard added a comment -

          Dear Marina Glancy and Dan Poltawski,

          Added you as watchers because I feel you should be aware of this .

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Marina Glancy and Dan Poltawski , Added you as watchers because I feel you should be aware of this . Cheers, Gareth
          Hide
          Marina Glancy added a comment - - edited

          Gareth,
          this parent renderer class was not very good idea unfortunately.

          The problem with it is that themes can not override the parent renderer class. I'm sure you remember the discussion in forum. If theme creates it's

          class theme_xxx_format_section_renderer_base extends format_section_renderer_base {
          ...
          }
          

          it is useless because the output API never picks it up.
          Theme should define one class for each format extending format_section_renderer_base:

          class theme_xxx_format_topics_renderer extends format_topics_renderer {
          ...
          }
          class theme_xxx_format_weeks_renderer extends format_weeks_renderer {
          ...
          }
          ...
          

          So basically the less developer-friendly this parent class is now the less formats will use it and less errors happen. We need to convert it to bunch of usefull functions in core_course_renderer or in another new renderer and format plugins must call those functions as $this->page->get_renderer('core', 'course')->function_name().
          Renderers should not extend each other. Only themes can extend renderers and that's what they were designed for.

          We can integrate this change of course but it won't solve the problem unfortunately.

          Show
          Marina Glancy added a comment - - edited Gareth, this parent renderer class was not very good idea unfortunately. The problem with it is that themes can not override the parent renderer class. I'm sure you remember the discussion in forum. If theme creates it's class theme_xxx_format_section_renderer_base extends format_section_renderer_base { ... } it is useless because the output API never picks it up. Theme should define one class for each format extending format_section_renderer_base: class theme_xxx_format_topics_renderer extends format_topics_renderer { ... } class theme_xxx_format_weeks_renderer extends format_weeks_renderer { ... } ... So basically the less developer-friendly this parent class is now the less formats will use it and less errors happen. We need to convert it to bunch of usefull functions in core_course_renderer or in another new renderer and format plugins must call those functions as $this->page->get_renderer('core', 'course')->function_name(). Renderers should not extend each other. Only themes can extend renderers and that's what they were designed for. We can integrate this change of course but it won't solve the problem unfortunately.
          Hide
          Gareth J Barnard added a comment -

          Dear Marina Glancy,

          Thanks . It will solve the problem for me as I need it for my course formats and not themes.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Marina Glancy , Thanks . It will solve the problem for me as I need it for my course formats and not themes. Cheers, Gareth
          Hide
          Dan Poltawski added a comment -

          The design of this renderer may not be great, but I think that making this method protected is 'in the spirit' of what i was originally trying to allow, so my +1 to this.

          Show
          Dan Poltawski added a comment - The design of this renderer may not be great, but I think that making this method protected is 'in the spirit' of what i was originally trying to allow, so my +1 to this.
          Hide
          Dan Poltawski added a comment -

          Gareth, sending for integration - please can you update your branches too? Thanks

          Show
          Dan Poltawski added a comment - Gareth, sending for integration - please can you update your branches too? Thanks
          Hide
          Gareth J Barnard added a comment -

          Dear Dan Poltawski,

          Thanks Dan . All branches rebased.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Dan Poltawski , Thanks Dan . All branches rebased. Cheers, Gareth
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, this as been integrated now.
          Certainly a safe worthwhile change.

          Show
          Sam Hemelryk added a comment - Thanks guys, this as been integrated now. Certainly a safe worthwhile change.
          Hide
          Rossiani Wijaya added a comment -

          This is working as expected.

          Tested for 2.3, 2.4 and master.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working as expected. Tested for 2.3, 2.4 and master. Test passed.
          Hide
          Dan Poltawski added a comment -

          Thanks! You're changes are now spread to the world through this git and our source control repositories.

          No time to rest though, we've got days to make 2.5 the best yet!

          ciao

          Show
          Dan Poltawski added a comment - Thanks! You're changes are now spread to the world through this git and our source control repositories. No time to rest though, we've got days to make 2.5 the best yet! ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: