Moodle
  1. Moodle
  2. MDL-37206

Preload course format renderer in /course/view.php

    Details

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

      Test 1. hardcoded 'weeks' bug fix

      1. Change the default course format to 'topics'
      2. install 3rd party course format
      3. create course in the new format
      4. remove the 3rd party course format (do not click 'uninstall', just delete a folder in /course/format/)
      5. view the course as admin and as a student
      6. developer warning should appear but course should be shown in format set as default ('topics')

      Test 2. preloading renderers

      1. create course in social format (format that does not have renderer)
      2. view the course (/course/view.php) as admin and as a student, make sure there are no errors
      Show
      Test 1. hardcoded 'weeks' bug fix Change the default course format to 'topics' install 3rd party course format create course in the new format remove the 3rd party course format (do not click 'uninstall', just delete a folder in /course/format/) view the course as admin and as a student developer warning should appear but course should be shown in format set as default ('topics') Test 2. preloading renderers create course in social format (format that does not have renderer) view the course (/course/view.php) as admin and as a student, make sure there are no errors
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-37206-master
    • Rank:
      46797

      Description

      Course format renderers may add some JS in their renderers or other actions that may need get_renderer() to be called before output starts. Usually it is so but in /course/view.php output starts before format.php is included.

      It would be nice to preload the course format renderer in /course/view.php before output starts

      Also while fixing I found that 'weeks' format was still hardcoded in /course/view.php, fixed it.

        Issue Links

          Activity

          Hide
          Marina Glancy added a comment -

          TO INTEGRATORS: please cherry-pick to 2.4

          Show
          Marina Glancy added a comment - TO INTEGRATORS: please cherry-pick to 2.4
          Hide
          Sam Hemelryk added a comment -

          Hmmm this really doesn't seem like an ideal solution to me sorry Marina.
          Exception handling is slow and doesn't feel like a good fit to me in situations like this. Really we should be checking the existence of a renderer rather than using exception handling as a safety net.

          Of course that’s just my opinion.
          I'll ask the other integrators and if they disagree with me and see it as fine this can land.
          Gets a -1 from me presently.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hmmm this really doesn't seem like an ideal solution to me sorry Marina. Exception handling is slow and doesn't feel like a good fit to me in situations like this. Really we should be checking the existence of a renderer rather than using exception handling as a safety net. Of course that’s just my opinion. I'll ask the other integrators and if they disagree with me and see it as fine this can land. Gets a -1 from me presently. Many thanks Sam
          Hide
          Dan Poltawski added a comment -

          Sorry, -1 from me. That sort of code has caught me out in the past masking errors and making them undetectable.

          Show
          Dan Poltawski added a comment - Sorry, -1 from me. That sort of code has caught me out in the past masking errors and making them undetectable.
          Hide
          Damyon Wiese added a comment - - edited

          Agreed - the only exception I can see that is likely to be thrown by this code is "coding_exception" which is far too generic to try and catch and hide.

          Show
          Damyon Wiese added a comment - - edited Agreed - the only exception I can see that is likely to be thrown by this code is "coding_exception" which is far too generic to try and catch and hide.
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, pretty general consensus reopening now Marina.

          Show
          Sam Hemelryk added a comment - Thanks guys, pretty general consensus reopening now Marina.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Marina Glancy added a comment -

          Changed the commit. Please cherry-pick in 2.4. Thanks

          Show
          Marina Glancy added a comment - Changed the commit. Please cherry-pick in 2.4. Thanks
          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
          Sam Hemelryk added a comment -

          Thanks Marina, looks much better!
          I've integrated this now and its ready for testing.
          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Marina, looks much better! I've integrated this now and its ready for testing. Cheers Sam
          Hide
          Ankit Agarwal added a comment -

          All good,
          works as described.
          Thanks

          Show
          Ankit Agarwal added a comment - All good, works as described. Thanks
          Hide
          Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: