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
    • Rank:
      42874

      Description

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

      • course/

        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: