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

remove_course_contents() in moodlelib.php fails

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Development in progress
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.5.9, 2.6.6, 2.7.3, 2.8
    • Fix Version/s: None
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      To reproduce failure, a plugin must be used that references the mdl_course_modules entry that corresponds to the instance it is being asked to delete.

      Create a course, add an activity using that above-mentioned plugin. to simulate a problem with the plugin, remove the mdl_course_modules row for that activity, but keep the instance row in the plugin's main table with a `course` column value that associates it with the course.

      When you attempt to delete the course, even though the plugin no longer appears in the course, the instance row will be referenced directly in the plugin's main table, and when it tries to reference the course module for the instance, it should fail-throw an exception-which halts the course deletion.

      With the modified remove_course_contents() function, the course deletion will be completed.

      Show
      To reproduce failure, a plugin must be used that references the mdl_course_modules entry that corresponds to the instance it is being asked to delete. Create a course, add an activity using that above-mentioned plugin. to simulate a problem with the plugin, remove the mdl_course_modules row for that activity, but keep the instance row in the plugin's main table with a `course` column value that associates it with the course. When you attempt to delete the course, even though the plugin no longer appears in the course, the instance row will be referenced directly in the plugin's main table, and when it tries to reference the course module for the instance, it should fail- throw an exception -which halts the course deletion. With the modified remove_course_contents() function, the course deletion will be completed.
    • Workaround:
      Hide

      Manually create a mdl_course_modules row (and context) for the latent plugin instance, or manually remove the plugin's instance in the course you're trying to delete.

      Show
      Manually create a mdl_course_modules row (and context) for the latent plugin instance, or manually remove the plugin's instance in the course you're trying to delete.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE, MOODLE_28_STABLE
    • Pull Master Branch:

      Description

      In certain (unexpected, and out of the ordinary) circumstances where `mdl_course_modules` rows have been removed, but the corresponding plugin's rows have not, users may not be able to delete the course if the plugin references the course module context as part of the [plugin]_delete_instance() method.

      I understand the rarity of the situation where `mdl_course_modules` rows are removed and the plugin's corresponding rows are not. I also understand that it may not be typical to reference the course module context when deleting an instance. The problem is, when coping with varying degrees of quality from third-party sources, the problem becomes less rare, and more typical--rather than what should be, we are left with what is.

      Our production installation has just such a problem using Blackboard's Elluminate plugin. Several courses could not be deleted because an error would be generated by the plugin when attempting to delete Elluminate sessions—the error is caused by the absence of the `mdl_course_modules` row, and hence the absence of the course module’s context, because Elluminate does a capability check as part of the instance deletion.

      As easy as it would be to just blame Blackboard, and say “they shouldn’t do it that way,” I don’t think that’d be fair. When you consider that Moodle should use the `mdl_course_modules` entries as the foundation for what needs to be removed from a course, in the same way it uses `mdl_course_modules` entries as the foundation for what should be displayed in a course, the weakness is in the remove_course_contents() function in moodlelib.php.

      The current approach is to get a list of mod plugins, and query each of the main plugin tables (e.g. `mdl_elluminate`, `mdl_forum`) for any rows that have a particular `course` column value. Then for each of those entries, work backward to the corresponding `mdl_course_modules` entries—and from the current code, there is an expectation that the `mdl_course_modules` row might not exit (v. 2.8 line 5115 – if($cm) test before calling context_helper::delete_instance).

      Yeah, it’s easy to say a plugin should test the course module context for null before calling a routine that requires it. I get that. But if the core Moodle code could do things in such a way that circumvented the problem, while being just as efficient, shouldn’t it?

      So, my suggested improvement to the remove_course_contents is to drive the process from the entries in `mdl_course_modules` rather than the individual plugins’ main table. Attached is the code.

      Firstly, one side benefit is that the lib.php files are loaded only for mods that are known to be used in the course, rather than all of them.

      Secondly, the Moodle core routine does not query the plugin’s main table directly, rather it uses the `mdl_course_modules` row to identify the plugin and then delegates to the plugin. As a fallback, it will still attempt to delete the row from the plugin’s main table directly—even though, in my opinion, it should not do this because of the likely orphaning of related rows in other tables. The remove_course_contents() function shouldn’t need to delete plugin table entities directly; if the plugin fails to delete the entity in its own tables (e.g. failed webservice call, or some other bug in the plugin), then so be it, that problem is on the plugin, but the core Moodle routine should remove the `mdl_course_modules` rows and remove the course.

      Is there anywhere else in Moodle that the different plugins’ main tables are queried directly and the assumption that it has a column named `course` ? Isn’t that what the `mdl_course_modules` table was meant to avoid?

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              Unassigned Unassigned
              Reporter:
              woolardfa@appstate.edu Fred Woolard
              Participants:
              Component watchers:
              Amaia Anabitarte, Carlos Escobedo, Ferran Recio, Ilya Tregubov, Sara Arjona (@sarjona)
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated: