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

      Description

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

        Gliffy Diagrams

          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: