Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-37206

Preload course format renderer in /course/view.php

    Details

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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            marina Marina Glancy added a comment -

            TO INTEGRATORS: please cherry-pick to 2.4

            Show
            marina Marina Glancy added a comment - TO INTEGRATORS: please cherry-pick to 2.4
            Hide
            samhemelryk 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
            samhemelryk 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
            poltawski 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
            poltawski 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 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 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
            samhemelryk Sam Hemelryk added a comment -

            Thanks guys, pretty general consensus reopening now Marina.

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

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

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

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

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

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

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

            All good,
            works as described.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - All good, works as described. Thanks
            Hide
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  11/Mar/13