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

Remove all calls to get_context_instance_by_id() from core

    Details

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

          Attachments

            Issue Links

              Activity

              Hide
              ankit_frenz Ankit Agarwal added a comment -

              Requesting a review
              Thanks

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

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

              Show
              ankit_frenz Ankit Agarwal added a comment - updated summary. We are not deprecating the function here. Just removing its usage from core.
              Hide
              fred 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
              fred 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_frenz 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_frenz 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
              fred Frédéric Massart added a comment -

              All good! Thanks!

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

              Thanks Fred.

              Submitting for integration.

              @integrator
              Master only

              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - Thanks Fred. Submitting for integration. @integrator Master only Thanks
              Hide
              nebgor 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
              nebgor 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_frenz 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_frenz 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
              nebgor Aparup Banerjee added a comment -

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

              Show
              nebgor Aparup Banerjee added a comment - ah i should have added Tim, added now for his expertise here.
              Hide
              timhunt 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
              timhunt 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
              nebgor Aparup Banerjee added a comment -

              Thanks all , integrated into master only.

              Testing "exploitively" here should pick out any anomalies.

              Show
              nebgor Aparup Banerjee added a comment - Thanks all , integrated into master only. Testing "exploitively" here should pick out any anomalies.
              Hide
              nebgor 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
              nebgor 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
              nebgor 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
              nebgor 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
              nebgor Aparup Banerjee added a comment -

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

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

              Thanks Apu for taking care of that!

              Show
              ankit_frenz Ankit Agarwal added a comment - Thanks Apu for taking care of that!
              Hide
              phalacee 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
              phalacee 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

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

              http://youtu.be/n64CdfDRnZY

              Closing as fixed, ciao

              Show
              stronk7 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:
                    Fix Release Date:
                    3/Dec/12