Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Libraries
    • Testing Instructions:
      Hide
      1. Ensure JS is turned on.
      2. Add a bunch of different course modules to a course, including some with images etc.
      3. Delete them all.
      4. Check the data in the database is gone, as well as any uploaded files.
      5. Repeat the above but with JS turned off.
      6. Create an old assignment activity with lots of details.
      7. Run the upgrade_assignment tool and ensure the upgrade worked (see http://docs.moodle.org/23/en/Upgrade_tool on how to use the upgrade tool).
      Show
      Ensure JS is turned on. Add a bunch of different course modules to a course, including some with images etc. Delete them all. Check the data in the database is gone, as well as any uploaded files. Repeat the above but with JS turned off. Create an old assignment activity with lots of details. Run the upgrade_assignment tool and ensure the upgrade worked (see http://docs.moodle.org/23/en/Upgrade_tool on how to use the upgrade tool).
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-37082-master
    • Rank:
      46639

      Description

      Create a core function that can be used by the web service and in core itself to delete module information.

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment - - edited

          Thanks Mark, I had a look to the code:

          • don't catch exception coming from course_delete_module($this->cm->id); to do nothing (even echoing a notification of the error message is not super good).
            If a function throw an exception, it's for a good reason. It's an exceptional error. If you catch it and do nothing, this is going to become a nightmare to debug if anything wrong happen in course_delete_module(). I think you should remove these try catch.
            Few stuff not related to your code but you could consider:
          • just for the info (in case you don't know) avoid the moodle_exception("bla bla bla"). Use the correct way with error code and a matching lang string. When the web service server handles an exception, the server returns the error code which can be used by the client to implement automatic behaviors against it. The only exception I know of to be accepting string in english, and it's recommended like that, is coding_exception()
          • the phpdoc of course_delete_module is incomplete.

          Otherwise nice catch and improvement.

          Show
          Jérôme Mouneyrac added a comment - - edited Thanks Mark, I had a look to the code: don't catch exception coming from course_delete_module($this->cm->id); to do nothing (even echoing a notification of the error message is not super good). If a function throw an exception, it's for a good reason. It's an exceptional error. If you catch it and do nothing, this is going to become a nightmare to debug if anything wrong happen in course_delete_module(). I think you should remove these try catch. Few stuff not related to your code but you could consider: just for the info (in case you don't know) avoid the moodle_exception("bla bla bla"). Use the correct way with error code and a matching lang string. When the web service server handles an exception, the server returns the error code which can be used by the client to implement automatic behaviors against it. The only exception I know of to be accepting string in english, and it's recommended like that, is coding_exception() the phpdoc of course_delete_module is incomplete. Otherwise nice catch and improvement.
          Hide
          Mark Nelson added a comment -

          Hi Jerome, in some places it throws an exception when an issue occurs (course/rest.php), others it displays a message (course/mod.php), that is why it varies. However, I did forget that if a DB query fails an exception will be thrown, so catching all exceptions is not a good idea. I will make the changes we discussed in person using the defined AJAX variable to determine whether to echo or to throw an exception. Thanks.

          Show
          Mark Nelson added a comment - Hi Jerome, in some places it throws an exception when an issue occurs (course/rest.php), others it displays a message (course/mod.php), that is why it varies. However, I did forget that if a DB query fails an exception will be thrown, so catching all exceptions is not a good idea. I will make the changes we discussed in person using the defined AJAX variable to determine whether to echo or to throw an exception. Thanks.
          Hide
          Mark Nelson added a comment -

          After speaking to Dan I am going to just throw an exception in all cases, rather than some cases echoing out a notification and others throwing exceptions. Really, if there is an exception the process should not continue, so outputting notifications rather than exceptions could be seen as being incorrect to begin with.

          Show
          Mark Nelson added a comment - After speaking to Dan I am going to just throw an exception in all cases, rather than some cases echoing out a notification and others throwing exceptions. Really, if there is an exception the process should not continue, so outputting notifications rather than exceptions could be seen as being incorrect to begin with.
          Hide
          Jérôme Mouneyrac added a comment -

          Except my two other optional points, you can send it to integration. Cheers.

          Show
          Jérôme Mouneyrac added a comment - Except my two other optional points, you can send it to integration. Cheers.
          Hide
          Mark Nelson added a comment -

          Hi Jerome, I had a discussion about point 2 with Dan and he has suggested that we should never have started passing language strings to the moodle_exception class. I am going to wait for more discussion on this point before making any changes.

          Show
          Mark Nelson added a comment - Hi Jerome, I had a discussion about point 2 with Dan and he has suggested that we should never have started passing language strings to the moodle_exception class. I am going to wait for more discussion on this point before making any changes.
          Hide
          Mark Nelson added a comment -
          Show
          Mark Nelson added a comment - Jerome, please see https://moodle.org/mod/forum/discuss.php?d=218027
          Hide
          Jérôme Mouneyrac added a comment - - edited

          I understand and agree with what Dan is saying but till we make the change in all core/webservice, we should not stop implementing web services, and the way I explained is the way we go atm. I'll write an issue for Dan's suggestion.

          Show
          Jérôme Mouneyrac added a comment - - edited I understand and agree with what Dan is saying but till we make the change in all core/webservice, we should not stop implementing web services, and the way I explained is the way we go atm. I'll write an issue for Dan's suggestion.
          Hide
          Jérôme Mouneyrac added a comment -

          I created MDL-37166

          Show
          Jérôme Mouneyrac added a comment - I created MDL-37166
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Mark Nelson added a comment -

          Ok, this has been rebased as it constantly causes merging conflicts. Let's hope it goes in next integration cycle.

          Show
          Mark Nelson added a comment - Ok, this has been rebased as it constantly causes merging conflicts. Let's hope it goes in next integration cycle.
          Hide
          Dan Poltawski added a comment -

          Hmm, I am not so sure its worth us renaming this function (and adding another bit of deprecated code), instead, I think we should just add the new functionality to existing function. I don't think it'd be catastrophic for third party modules (and we'd document it in the release notes). This seems better to me than adding another deprecated function.

          Show
          Dan Poltawski added a comment - Hmm, I am not so sure its worth us renaming this function (and adding another bit of deprecated code), instead, I think we should just add the new functionality to existing function. I don't think it'd be catastrophic for third party modules (and we'd document it in the release notes). This seems better to me than adding another deprecated function.
          Hide
          Dan Poltawski added a comment -

          I'm going to ask other integrators about it.

          Show
          Dan Poltawski added a comment - I'm going to ask other integrators about it.
          Hide
          Aparup Banerjee added a comment -

          Hi, not going into this too deep, renaming is really more troublesome for 3rd party. so if it can be lived with, i'd say yea 'lets not awaken all the plugin devs who use this' if we can.

          Show
          Aparup Banerjee added a comment - Hi, not going into this too deep, renaming is really more troublesome for 3rd party. so if it can be lived with, i'd say yea 'lets not awaken all the plugin devs who use this' if we can.
          Hide
          Mark Nelson added a comment -

          Hi Guys, here is the alternative method where we simply change the existing functions logic - https://github.com/markn86/moodle/compare/master...MDL-37082_master_alt

          Show
          Mark Nelson added a comment - Hi Guys, here is the alternative method where we simply change the existing functions logic - https://github.com/markn86/moodle/compare/master...MDL-37082_master_alt
          Hide
          Damyon Wiese added a comment -

          I think this is good. The old function was not doing the complete delete so everywhere it was used there was duplicated code to delete the module from the section and trigger the events.

          The risk I think with just adding to the existing function is that the module delete_instance function will be called twice (for existing code) and that could cause an exception.

          I vote for changing the function name and deprecating. Also - using course_ for the function name prefix for the new function is good practice.

          • Damyon
          Show
          Damyon Wiese added a comment - I think this is good. The old function was not doing the complete delete so everywhere it was used there was duplicated code to delete the module from the section and trigger the events. The risk I think with just adding to the existing function is that the module delete_instance function will be called twice (for existing code) and that could cause an exception. I vote for changing the function name and deprecating. Also - using course_ for the function name prefix for the new function is good practice. Damyon
          Hide
          Jérôme Mouneyrac added a comment -

          In my opinion Mark's solution is the one that doesn't break any 3rd party plugin, the behavior doesn't change as the function is deprecated and available without loading anything (I suppose, I didn't test). But if you want to fix the existing, it's fine too me, it is probable that all devs calling the function expect it to properly delete module. However note if the new code throw exception that were not thrown before (I think it does), then it will break third party plugins.

          Also this is not a proper way to write moodle_Exception, first param should be errorcode...

           moodle_exception("This module is missing mod/$modulename/lib.php"
          

          it you don't want to use errorcode it may be a coding_exception.

          Show
          Jérôme Mouneyrac added a comment - In my opinion Mark's solution is the one that doesn't break any 3rd party plugin, the behavior doesn't change as the function is deprecated and available without loading anything (I suppose, I didn't test). But if you want to fix the existing, it's fine too me, it is probable that all devs calling the function expect it to properly delete module. However note if the new code throw exception that were not thrown before (I think it does), then it will break third party plugins. Also this is not a proper way to write moodle_Exception, first param should be errorcode... moodle_exception( "This module is missing mod/$modulename/lib.php" it you don't want to use errorcode it may be a coding_exception.
          Hide
          Jérôme Mouneyrac added a comment -

          Ah damyon answer before me

          Show
          Jérôme Mouneyrac added a comment - Ah damyon answer before me
          Hide
          Aparup Banerjee added a comment - - edited

          i'm just doing a search now on my plugins checkout locally.. nothing on 'delete_course_module' so far..

          EDIT:

          results (from over 300 hundred are) :
          294/1050.zip/registration/lib.php:917: registration_delete_course_module($cm->id);
          294/1050.zip/registration/lib.php:956:function registration_delete_course_module($id) {
          294/717.zip/registration/lib.php:917: registration_delete_course_module($cm->id);
          294/717.zip/registration/lib.php:956:function registration_delete_course_module($id) {
          48/68.zip/forumng/lib.php:104: if (!delete_course_module($context->instanceid)) {
          98/453.zip/massaction/action.php:268: if (!delete_course_module($cm->id)) {
          98/166.zip/massaction/action.php:264: if (!delete_course_module($cm->id)) {
          99/249.zip/wiziq/auto.php:251: if (! delete_course_module($mod->coursemodule)) {
          99/249.zip/wiziq/auto.php:489: if (! delete_course_module($cm->id)) {

          so thats 4 devs to ping ... note forumng too . so yea not much impact to rename it seems.
          Apu out.

          Show
          Aparup Banerjee added a comment - - edited i'm just doing a search now on my plugins checkout locally.. nothing on 'delete_course_module' so far.. EDIT: results (from over 300 hundred are) : 294/1050.zip/registration/lib.php:917: registration_delete_course_module($cm->id); 294/1050.zip/registration/lib.php:956:function registration_delete_course_module($id) { 294/717.zip/registration/lib.php:917: registration_delete_course_module($cm->id); 294/717.zip/registration/lib.php:956:function registration_delete_course_module($id) { 48/68.zip/forumng/lib.php:104: if (!delete_course_module($context->instanceid)) { 98/453.zip/massaction/action.php:268: if (!delete_course_module($cm->id)) { 98/166.zip/massaction/action.php:264: if (!delete_course_module($cm->id)) { 99/249.zip/wiziq/auto.php:251: if (! delete_course_module($mod->coursemodule)) { 99/249.zip/wiziq/auto.php:489: if (! delete_course_module($cm->id)) { so thats 4 devs to ping ... note forumng too . so yea not much impact to rename it seems. Apu out.
          Hide
          Dan Poltawski added a comment -

          Heh, my initial comment was perhaps a bit strong, I can go either way on this.

          Show
          Dan Poltawski added a comment - Heh, my initial comment was perhaps a bit strong, I can go either way on this.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          From my mind (MDL-30098) I think I liked this last week, with the name change, to clearly state the differences. Just with a call to the new function and the deprecated one in the corresponding upgrade.txt file and a link in the MDL-31207 (Meta 2.5 deprecated) issue. So +1

          Show
          Eloy Lafuente (stronk7) added a comment - From my mind ( MDL-30098 ) I think I liked this last week, with the name change, to clearly state the differences. Just with a call to the new function and the deprecated one in the corresponding upgrade.txt file and a link in the MDL-31207 (Meta 2.5 deprecated) issue. So +1
          Hide
          Dan Poltawski added a comment -

          Well consensus says course_delete_module it is!

          Integrated to master, thanks everyone!

          Show
          Dan Poltawski added a comment - Well consensus says course_delete_module it is! Integrated to master, thanks everyone!
          Hide
          Frédéric Massart added a comment -

          Test passed. Thanks!

          Show
          Frédéric Massart added a comment - Test passed. Thanks!
          Hide
          Damyon Wiese added a comment -

          This change causes unittests to print deprecation warnings - mod/assign/tests/upgradelib_test.php still has calls to delete_course_module.

          Show
          Damyon Wiese added a comment - This change causes unittests to print deprecation warnings - mod/assign/tests/upgradelib_test.php still has calls to delete_course_module.
          Hide
          Dan Poltawski added a comment -

          Sending this back to failed to address the phpunit debug warnings.

          Show
          Dan Poltawski added a comment - Sending this back to failed to address the phpunit debug warnings.
          Hide
          Mark Nelson added a comment -

          I swear I did a grep for all usages of the function .. strange. I have pushed another commit to fix this. Thanks.

          Show
          Mark Nelson added a comment - I swear I did a grep for all usages of the function .. strange. I have pushed another commit to fix this. Thanks.
          Hide
          Mark Nelson added a comment -

          Ok, seems it was only introduced recently, so I did get all usages of that function when I grepped. Woohoo, I am not going crazy (crazier).

          Show
          Mark Nelson added a comment - Ok, seems it was only introduced recently, so I did get all usages of that function when I grepped. Woohoo, I am not going crazy (crazier).
          Hide
          Dan Poltawski added a comment -

          Thanks a lot Mark, i've pulled that in. Will run phpunit again now.

          Show
          Dan Poltawski added a comment - Thanks a lot Mark, i've pulled that in. Will run phpunit again now.
          Hide
          Dan Poltawski added a comment -

          Bad news..

          Show
          Dan Poltawski added a comment - Bad news..
          Hide
          Dan Poltawski added a comment -

          mod_assign_upgradelib_testcase::test_upgrade_upload_assignment
          dml_missing_record_exception: Can not find data record in database. (SELECT cm.*, m.name, md.name AS modname
          FROM

          {course_modules}

          cm
          JOIN

          {modules}

          md ON md.id = cm.module
          JOIN

          {assign}

          m ON m.id = cm.instance

          WHERE m.id = :instance AND md.name = :modulename

          [array (
          'instance' => '1',
          'modulename' => 'assign',
          )])

          /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1404
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/datalib.php:1551
          /Users/Shared/Jenkins/Home/git_repositories/master/mod/assign/lib.php:51
          /Users/Shared/Jenkins/Home/git_repositories/master/course/lib.php:2232
          /Users/Shared/Jenkins/Home/git_repositories/master/mod/assign/tests/upgradelib_test.php:150
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/classes/advanced_testcase.php:76

          Show
          Dan Poltawski added a comment - mod_assign_upgradelib_testcase::test_upgrade_upload_assignment dml_missing_record_exception: Can not find data record in database. (SELECT cm.*, m.name, md.name AS modname FROM {course_modules} cm JOIN {modules} md ON md.id = cm.module JOIN {assign} m ON m.id = cm.instance WHERE m.id = :instance AND md.name = :modulename [array ( 'instance' => '1', 'modulename' => 'assign', )]) /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1404 /Users/Shared/Jenkins/Home/git_repositories/master/lib/datalib.php:1551 /Users/Shared/Jenkins/Home/git_repositories/master/mod/assign/lib.php:51 /Users/Shared/Jenkins/Home/git_repositories/master/course/lib.php:2232 /Users/Shared/Jenkins/Home/git_repositories/master/mod/assign/tests/upgradelib_test.php:150 /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/classes/advanced_testcase.php:76
          Show
          Dan Poltawski added a comment - http://stronk7.doesntexist.com/job/07.%20Run%20phpunit%20UnitTests%20(master)/1812/testReport/junit/(root)/mod_assign_upgradelib_testcase/test_upgrade_offline_assignment/
          Hide
          Damyon Wiese added a comment -

          I'll take a look.

          Show
          Damyon Wiese added a comment - I'll take a look.
          Hide
          Damyon Wiese added a comment -

          I added one patch to the branch to fix the failed unit tests.

          Show
          Damyon Wiese added a comment - I added one patch to the branch to fix the failed unit tests.
          Hide
          Mark Nelson added a comment -

          Hi Dan, sorry about this. I don't know where the description of the deprecation went in the upgrade.txt, but I have added it again. Please find it here https://github.com/markn86/moodle/commit/ae7c52adecb561b9a7a715b471c838341789832b

          Show
          Mark Nelson added a comment - Hi Dan, sorry about this. I don't know where the description of the deprecation went in the upgrade.txt, but I have added it again. Please find it here https://github.com/markn86/moodle/commit/ae7c52adecb561b9a7a715b471c838341789832b
          Hide
          Dan Poltawski added a comment -

          Pulled both fixes in.

          Show
          Dan Poltawski added a comment - Pulled both fixes in.
          Hide
          Dan Poltawski added a comment -

          (I don't know how I miseed the upgrade.txt too)

          Show
          Dan Poltawski added a comment - (I don't know how I miseed the upgrade.txt too)
          Hide
          Jérôme Mouneyrac added a comment - - edited

          If you have time could you fix the exceptions. When they'll reach a web service, the client will see stuff like "This module is missing mod/$modulename/lib.php" as error code and failedtodeletemodulemissinglibfile as debug info. I think it should be the opposite.
          PS: once used by clients, error codes can not be changed anymore without breaking all clients (i.e. I think you really want to fix them now )

          Show
          Jérôme Mouneyrac added a comment - - edited If you have time could you fix the exceptions. When they'll reach a web service, the client will see stuff like "This module is missing mod/$modulename/lib.php" as error code and failedtodeletemodulemissinglibfile as debug info. I think it should be the opposite. PS: once used by clients, error codes can not be changed anymore without breaking all clients (i.e. I think you really want to fix them now )
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: