Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      This patch literately affects every single page of the course module without any functionality change. So basically we need to look for regressions.
      Test 1

      • Browse through the any course
      • Follow any links
      • Edit and save forms
      • Make sure no exceptions are raised on any page
      • Try creating/deleting/editing course
      • Try creating/editing/deleteing/updating activities

      Test 2
      Grep for get_context_instance() in the course folder and make sure no instances are returned

      Test 3 (optional, success gets you a cookie )

      • Test Moodle
      Show
      This patch literately affects every single page of the course module without any functionality change. So basically we need to look for regressions. Test 1 Browse through the any course Follow any links Edit and save forms Make sure no exceptions are raised on any page Try creating/deleting/editing course Try creating/editing/deleteing/updating activities Test 2 Grep for get_context_instance() in the course folder and make sure no instances are returned Test 3 (optional, success gets you a cookie ) Test Moodle
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-34459-master

      Description

      Replace get_context_instance with context_XXXX::instance() in set location (group 4)
      Location

      • course/

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Poltawski added a comment -

            Hi Ankit, I don't think the course/externallib.php changes are right. I think its fine for them to throw exceptions early like that.

            Particularly, this looks like its looking for the exception:

                         try {
            -                self::validate_context($context);
            +                self::validate_context($context, IGNORE_MISSING);
                         } catch (Exception $e) {
                             $exceptionparam = new stdClass();
                             $exceptionparam->message = $e->getMessage();
            

            Show
            Dan Poltawski added a comment - Hi Ankit, I don't think the course/externallib.php changes are right. I think its fine for them to throw exceptions early like that. Particularly, this looks like its looking for the exception: try { - self::validate_context($context); + self::validate_context($context, IGNORE_MISSING); } catch (Exception $e) { $exceptionparam = new stdClass(); $exceptionparam->message = $e->getMessage();
            Hide
            Dan Poltawski added a comment -

            Also, not sure why being unstrict here:

             function get_category_or_system_context($categoryid) {
                 if ($categoryid) {
            -        return get_context_instance(CONTEXT_COURSECAT, $categoryid);
            +        return context_coursecat::instance($categoryid, IGNORE_MISSING);
                 } else {
            -        return get_context_instance(CONTEXT_SYSTEM);
            +        return context_system::instance();
                 }
             }
            

            Show
            Dan Poltawski added a comment - Also, not sure why being unstrict here: function get_category_or_system_context($categoryid) { if ($categoryid) { - return get_context_instance(CONTEXT_COURSECAT, $categoryid); + return context_coursecat::instance($categoryid, IGNORE_MISSING); } else { - return get_context_instance(CONTEXT_SYSTEM); + return context_system::instance(); } }
            Hide
            Ankit Agarwal added a comment -

            Just for records:-

            1. I have changed the first usage, was a typo.
            2. Had a talk with Dan about get_category_or_system_context() and we agreed to use IGNORE_MISSING in there to keep the api consistent with it's previous version.

            Patches updated
            Thanks

            Show
            Ankit Agarwal added a comment - Just for records:- I have changed the first usage, was a typo. Had a talk with Dan about get_category_or_system_context() and we agreed to use IGNORE_MISSING in there to keep the api consistent with it's previous version. Patches updated Thanks
            Hide
            Dan Poltawski added a comment -

            Thanks Ankit, i've integrated that now.

            Show
            Dan Poltawski added a comment - Thanks Ankit, i've integrated that now.
            Hide
            David Monllaó added a comment - - edited

            Tested

            • Creating a course
            • Editing it's parameters
            • Hiding sections (with and without AJAX & Javascript)
            • Moving sections (with and without AJAX & Javascript)
            • Creating activities (with and without AJAX & Javascript)
            • Moving activities (with and without AJAX & Javascript)
            • Duplicating activities (with and without AJAX & Javascript)
            • Hiding and displaying again activities (with and without AJAX & Javascript)
            • Deleting the course

            No problems found, it passes

            Show
            David Monllaó added a comment - - edited Tested Creating a course Editing it's parameters Hiding sections (with and without AJAX & Javascript) Moving sections (with and without AJAX & Javascript) Creating activities (with and without AJAX & Javascript) Moving activities (with and without AJAX & Javascript) Duplicating activities (with and without AJAX & Javascript) Hiding and displaying again activities (with and without AJAX & Javascript) Deleting the course No problems found, it passes
            Hide
            Ankit Agarwal added a comment -

            Congrats David you are entitled to one cookie

            Show
            Ankit Agarwal added a comment - Congrats David you are entitled to one cookie
            Hide
            Aparup Banerjee added a comment -

            yay, it works!

            This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week.

            Thank you all for taking the time to get us here.

            cheers!

            Show
            Aparup Banerjee added a comment - yay, it works! This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week. Thank you all for taking the time to get us here. cheers!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: