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

Code to delete a course module should be fully self-contained

    Details

    • Type: Improvement
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.1.4
    • Fix Version/s: DEV backlog
    • Component/s: Course
    • Labels:
    • Affected Branches:
      MOODLE_21_STABLE

      Description

      I'm looking into MDL-31914 and have noticed that the code to delete a module is quite separated and there's room for error.

      • Some of the code is in course/mod.php (e.g. calling the $cm->modname . "_delete_instance" function, deleting area files, trigger the mod_deleted event, log the event, delete_mod_from_section, rebuild_course_cache.
      • Some of this is duplicated in the AJAX delete handler (course/rest.php)
      • The remainder (deleting course modules, course completion, events, etc) is handled in course/lib.php::delete_course_module.

      IMHO this should all be moved to the delete_course_module() function which already takes a course_module ID

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              quen Sam Marshall added a comment -

              Definitely agree removing duplicate code here and putting it in one place so it can be safely called by other parts of code (core and plugin) is important.

              One thing, by 'logging the event', did you mean something to do with the event, or did you mean add_to_log? If the latter, that should be left to 'UI-level code' imo. (For instance, let's say this function is used when deleting a course - you don't want it to log 'delete module' 74 times.)

              Show
              quen Sam Marshall added a comment - Definitely agree removing duplicate code here and putting it in one place so it can be safely called by other parts of code (core and plugin) is important. One thing, by 'logging the event', did you mean something to do with the event, or did you mean add_to_log? If the latter, that should be left to 'UI-level code' imo. (For instance, let's say this function is used when deleting a course - you don't want it to log 'delete module' 74 times.)
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Sorry, should have been clearer. add_to_log should still be handled by the UI IMO.

              Not sure about the events_trigger - it should probably move to course/lib.php but I'm not sure whether things will expect courses to exist when processing triggers for deleted events. The trigger probably still needs to be called but it's something to be aware of...

              Show
              dobedobedoh Andrew Nicols added a comment - Sorry, should have been clearer. add_to_log should still be handled by the UI IMO. Not sure about the events_trigger - it should probably move to course/lib.php but I'm not sure whether things will expect courses to exist when processing triggers for deleted events. The trigger probably still needs to be called but it's something to be aware of...
              Hide
              salvetore Michael de Raadt added a comment -

              Thanks for suggesting that, Andrew.

              It this were to be centralised (and it probably should be), it would probably be necessary to have some form of callback so that modules and other related plugins could complete tasks when a module is deleted. I haven't looked into this at all, so I'm not sure how it currently works.

              Show
              salvetore Michael de Raadt added a comment - Thanks for suggesting that, Andrew. It this were to be centralised (and it probably should be), it would probably be necessary to have some form of callback so that modules and other related plugins could complete tasks when a module is deleted. I haven't looked into this at all, so I'm not sure how it currently works.
              Hide
              marina Marina Glancy added a comment -

              Linking to MDL-36789 as it seems that in some cases deleting the module does not clear the course cache properly and results in corrupted data with following fatal error when trying to view a course or dispay navigation

              Show
              marina Marina Glancy added a comment - Linking to MDL-36789 as it seems that in some cases deleting the module does not clear the course cache properly and results in corrupted data with following fatal error when trying to view a course or dispay navigation

                People

                • Votes:
                  1 Vote for this issue
                  Watchers:
                  2 Start watching this issue

                  Dates

                  • Created:
                    Updated: