Moodle
  1. Moodle
  2. MDL-31915

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

    Details

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

      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

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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 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 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: