Details

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

      Description

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

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              jerome 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
              jerome 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
              markn 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
              markn 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
              markn 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
              markn 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
              jerome Jérôme Mouneyrac added a comment -

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

              Show
              jerome Jérôme Mouneyrac added a comment - Except my two other optional points, you can send it to integration. Cheers.
              Hide
              markn 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
              markn 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
              markn Mark Nelson added a comment -
              Show
              markn Mark Nelson added a comment - Jerome, please see https://moodle.org/mod/forum/discuss.php?d=218027
              Hide
              jerome 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
              jerome 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
              jerome Jérôme Mouneyrac added a comment -

              I created MDL-37166

              Show
              jerome Jérôme Mouneyrac added a comment - I created MDL-37166
              Hide
              poltawski 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
              poltawski 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
              markn 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
              markn 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
              poltawski 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
              poltawski 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
              poltawski Dan Poltawski added a comment -

              I'm going to ask other integrators about it.

              Show
              poltawski Dan Poltawski added a comment - I'm going to ask other integrators about it.
              Hide
              nebgor 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
              nebgor 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
              markn 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
              markn 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 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 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
              jerome 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
              jerome 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
              jerome Jérôme Mouneyrac added a comment -

              Ah damyon answer before me

              Show
              jerome Jérôme Mouneyrac added a comment - Ah damyon answer before me
              Hide
              nebgor 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
              nebgor 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
              poltawski Dan Poltawski added a comment -

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

              Show
              poltawski Dan Poltawski added a comment - Heh, my initial comment was perhaps a bit strong, I can go either way on this.
              Hide
              stronk7 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
              stronk7 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
              poltawski Dan Poltawski added a comment -

              Well consensus says course_delete_module it is!

              Integrated to master, thanks everyone!

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

              Test passed. Thanks!

              Show
              fred Frédéric Massart added a comment - Test passed. Thanks!
              Hide
              damyon 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 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
              poltawski Dan Poltawski added a comment -

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

              Show
              poltawski Dan Poltawski added a comment - Sending this back to failed to address the phpunit debug warnings.
              Hide
              markn 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
              markn 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
              markn 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
              markn 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
              poltawski Dan Poltawski added a comment -

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

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

              Bad news..

              Show
              poltawski Dan Poltawski added a comment - Bad news..
              Hide
              poltawski 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
              poltawski 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
              poltawski 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 Damyon Wiese added a comment -

              I'll take a look.

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

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

              Show
              damyon Damyon Wiese added a comment - I added one patch to the branch to fix the failed unit tests.
              Hide
              markn 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
              markn 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
              poltawski Dan Poltawski added a comment -

              Pulled both fixes in.

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

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

              Show
              poltawski Dan Poltawski added a comment - (I don't know how I miseed the upgrade.txt too)
              Hide
              jerome 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
              jerome 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
              stronk7 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
              stronk7 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:
                    Fix Release Date:
                    14/May/13