Moodle
  1. Moodle
  2. MDL-34549

Remove all calls to get_context_instance_by_id() from core

    Details

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

      Grep and make sure there is no valid usage of get_context_instance_by_id() in core.

      This requires exploratory testing on the following :-

      1. Roles (assigning system roles, removing roles, creating roles etc)
      2. Bulk user upload
      3. Blocks ( community, html ,quiz results, comments)
      4. Cohorts
      5. Comments
      6. enrollments with various methods including cohort
      7. Filepicker
      8. Grades
      9. Questions
      10. quiz
      11. Forum
      12. Glossaries
      13. Repositories
      14. Web services
      15. unit tests
      Show
      Grep and make sure there is no valid usage of get_context_instance_by_id() in core. This requires exploratory testing on the following :- Roles (assigning system roles, removing roles, creating roles etc) Bulk user upload Blocks ( community, html ,quiz results, comments) Cohorts Comments enrollments with various methods including cohort Filepicker Grades Questions quiz Forum Glossaries Repositories Web services unit tests
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-34549-master
    • Rank:
      42982

      Description

      Remove all usage of get_instance_by_id()from core.

        Issue Links

          Activity

          Hide
          Ankit Agarwal added a comment -

          Requesting a review
          Thanks

          Show
          Ankit Agarwal added a comment - Requesting a review Thanks
          Hide
          Ankit Agarwal added a comment -

          updated summary. We are not deprecating the function here. Just removing its usage from core.

          Show
          Ankit Agarwal added a comment - updated summary. We are not deprecating the function here. Just removing its usage from core.
          Hide
          Frédéric Massart added a comment - - edited

          Hi Ankit, you have all my respect for working on this !

          Could you please justify the use of IGNORE_MISSING for the 3 files under lib/filebrowser/. I would personaly have had MUST_EXIST instead as get_file_info() would return the system context in case of false, and I'd assume that this could lead to having the wrong context.

          I think IGNORE_MISSING is recommended for:

          • weblib:1075
          • weblib:1284
          • question/editlib.php:1714
          • repository/lib:1178

          Cheers! Push for integration whenever ready!

          Show
          Frédéric Massart added a comment - - edited Hi Ankit, you have all my respect for working on this ! Could you please justify the use of IGNORE_MISSING for the 3 files under lib/filebrowser/ . I would personaly have had MUST_EXIST instead as get_file_info() would return the system context in case of false, and I'd assume that this could lead to having the wrong context. I think IGNORE_MISSING is recommended for: weblib:1075 weblib:1284 question/editlib.php:1714 repository/lib:1178 Cheers! Push for integration whenever ready!
          Hide
          Ankit Agarwal added a comment -

          Hi Fred,
          Thanks for your feedback.

          • I am not very sure about what exactly to use for lib/filebrowser . The reason I used IGNORE_MISSING was to keep the api's consistent with there previous behavior. Before the patch when a context is not found in the get_parent() method, system context is used in get_file_info() method. Added IGNORE_MISSING to keep it the same way.
          • weblib:1075 and weblib:1284 , I still think MUST_EXIST is best in this case. Since if we explicitly specify a contextid, it should exist. Also while replacing get_context_instance(), with context_xxx::instance() in MDL-33061 we considered format_text() and format_string() calls to be must exist.
          • question/editlib.php:1714 updated patch to use IGNORE_MISSING
          • repository/lib:1178 updated patch to use IGNORE_MISSING

          Let me know if you think anything else needs to be changed.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Fred, Thanks for your feedback. I am not very sure about what exactly to use for lib/filebrowser . The reason I used IGNORE_MISSING was to keep the api's consistent with there previous behavior. Before the patch when a context is not found in the get_parent() method, system context is used in get_file_info() method. Added IGNORE_MISSING to keep it the same way. weblib:1075 and weblib:1284 , I still think MUST_EXIST is best in this case. Since if we explicitly specify a contextid, it should exist. Also while replacing get_context_instance(), with context_xxx::instance() in MDL-33061 we considered format_text() and format_string() calls to be must exist. question/editlib.php:1714 updated patch to use IGNORE_MISSING repository/lib:1178 updated patch to use IGNORE_MISSING Let me know if you think anything else needs to be changed. Thanks
          Hide
          Frédéric Massart added a comment -

          All good! Thanks!

          Show
          Frédéric Massart added a comment - All good! Thanks!
          Hide
          Ankit Agarwal added a comment -

          Thanks Fred.

          Submitting for integration.

          @integrator
          Master only

          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Fred. Submitting for integration. @integrator Master only Thanks
          Hide
          Aparup Banerjee added a comment -

          Hello guys,
          Thanks for the constructive to and fro here , after some combing through this with a fine toothpick, looks like you've caught everything!

          ah, i just got one query: -

          • question/type/questiontypebase.php - the $context returned by get_context_by_category_id() , if false could trigger some notices @ save_question() in same file. $context in save_question() seems not checked and $context->id is accessed later on?
          Show
          Aparup Banerjee added a comment - Hello guys, Thanks for the constructive to and fro here , after some combing through this with a fine toothpick, looks like you've caught everything! ah, i just got one query: - question/type/questiontypebase.php - the $context returned by get_context_by_category_id() , if false could trigger some notices @ save_question() in same file. $context in save_question() seems not checked and $context->id is accessed later on?
          Hide
          Ankit Agarwal added a comment -

          Hi Aparup,
          Yes it can be an issue if the context is false. The reason I used IGNORE_MISSING was to keep behavior of the internal api ( get_context_by_category_id()) consistent, as it was before. If we change the behavior any child class using this method might break. Also on the other hand $context->id can cause issues if $context is false.
          Not sure which is the best solution for this.

          Cheers

          Show
          Ankit Agarwal added a comment - Hi Aparup, Yes it can be an issue if the context is false. The reason I used IGNORE_MISSING was to keep behavior of the internal api ( get_context_by_category_id()) consistent, as it was before. If we change the behavior any child class using this method might break. Also on the other hand $context->id can cause issues if $context is false. Not sure which is the best solution for this. Cheers
          Hide
          Aparup Banerjee added a comment -

          ah i should have added Tim, added now for his expertise here.

          Show
          Aparup Banerjee added a comment - ah i should have added Tim, added now for his expertise here.
          Hide
          Tim Hunt added a comment -

          You are probably right that the code is a bit dodgy here. However, I can as that I have never seen any problems reported caused by this, either in the tracker or the quiz forum. So, for now, it is OK to just convert the code to the nearest equivalent of how it used to work, which I guess is IGNORE_MISSING. Thanks.

          Show
          Tim Hunt added a comment - You are probably right that the code is a bit dodgy here. However, I can as that I have never seen any problems reported caused by this, either in the tracker or the quiz forum. So, for now, it is OK to just convert the code to the nearest equivalent of how it used to work, which I guess is IGNORE_MISSING. Thanks.
          Hide
          Aparup Banerjee added a comment -

          Thanks all , integrated into master only.

          Testing "exploitively" here should pick out any anomalies.

          Show
          Aparup Banerjee added a comment - Thanks all , integrated into master only. Testing "exploitively" here should pick out any anomalies.
          Hide
          Aparup Banerjee added a comment - - edited

          ah unit tests at CI have led to "
          Test Result (9 failures / +9)
          question_engine_unit_of_work_test.testDown
          question_engine_unit_of_work_test.test_initial_state
          question_engine_unit_of_work_test.test_update_usage
          question_engine_unit_of_work_test.test_add_question
          question_engine_unit_of_work_test.test_add_and_start_question
          question_engine_unit_of_work_test.test_process_action
          question_engine_unit_of_work_test.test_regrade_same_steps
          question_engine_unit_of_work_test.test_regrade_losing_steps
          question_engine_unit_of_work_test.test_tricky_regrade

          will fix that now..

          Show
          Aparup Banerjee added a comment - - edited ah unit tests at CI have led to " Test Result (9 failures / +9) question_engine_unit_of_work_test.testDown question_engine_unit_of_work_test.test_initial_state question_engine_unit_of_work_test.test_update_usage question_engine_unit_of_work_test.test_add_question question_engine_unit_of_work_test.test_add_and_start_question question_engine_unit_of_work_test.test_process_action question_engine_unit_of_work_test.test_regrade_same_steps question_engine_unit_of_work_test.test_regrade_losing_steps question_engine_unit_of_work_test.test_tricky_regrade will fix that now..
          Hide
          Aparup Banerjee added a comment - - edited

          they're all about "dml_missing_record_exception: Can not find data record in database table context. (SELECT * FROM

          {context}

          WHERE id = ?"
          via /var/lib/jenkins/git_repositories/master/question/engine/questionusage.php:711

          Show
          Aparup Banerjee added a comment - - edited they're all about "dml_missing_record_exception: Can not find data record in database table context. (SELECT * FROM {context} WHERE id = ?" via /var/lib/jenkins/git_repositories/master/question/engine/questionusage.php:711
          Hide
          Aparup Banerjee added a comment -

          added ignore_missing there (question/engine/questionusage.php:711) , all testing fine now

          Show
          Aparup Banerjee added a comment - added ignore_missing there (question/engine/questionusage.php:711) , all testing fine now
          Hide
          Ankit Agarwal added a comment -

          Thanks Apu for taking care of that!

          Show
          Ankit Agarwal added a comment - Thanks Apu for taking care of that!
          Hide
          Jason Fowler added a comment -

          two errors with the phpunit test in the DML section that seem unrelated to this, no errors discovered in the basic testing I did of the other areas of moodle though

          Show
          Jason Fowler added a comment - two errors with the phpunit test in the DML section that seem unrelated to this, no errors discovered in the basic testing I did of the other areas of moodle though
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm so proud...of you, many thanks!

          http://youtu.be/n64CdfDRnZY

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm so proud...of you, many thanks! http://youtu.be/n64CdfDRnZY Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: