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

      Description

      Remove all usage of get_instance_by_id()from core.

        Gliffy Diagrams

          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: