Call to a member function get_modulename() on bool

[golenkovm]

Found a few 4.1 sites that started throwing error on the admin/tool/fix_delete_modules/index.php page:

The ad-hoc task that page is failing has the following customdata: {"cms":{"14587":{"id":"14587"}},"userid":"401","realuserid":"401"}

[djarran]

The ad-hoc task / course module causing this issue does not contain an instance_id. This throws an exception when trying to delete the instance (calling specifically mod_hvp)

    $deleteinstancefunction = $modulename . '_delete_instance';

    if (!$deleteinstancefunction($cm->instance)) {
        throw new moodle_exception('cannotdeletemoduleinstance', '', '', null,
            "Cannot delete the module $modulename (instance).");
    }

When opening the index page, there are many calls to get_deletemodules. This removes a module if it has no moduleinstanceid. This was added as part of #22 to fix unit tests (unit tests were retested after removing the changes to this function and they still pass):

    public function get_deletemodules($remove = true) {
        // Unset if the module is already deleted successfully.
        // This is only required if MDL-80930 is not applied.
        // MDL-80930 is integrated, there should be no "NULL moduleinstanceid".
        foreach ($this->deletemodules as $key => $deletemodule) {
            // No instanceid.
            if (is_null($deletemodule->moduleinstanceid) && !$remove) {
                unset($this->deletemodules[$key]);
            }
        }
        return $this->deletemodules;
    }

After this function, null is returned (with the given customdata), causing an error when querying the DB:

    private function get_gradestable(delete_task $deletetask) {
        ....
        $deletemodule = current($deletetask->get_deletemodules()); // Only one module in the task.

        if ($records = $DB->get_records('grade_items',
                                        array('itemmodule' => $deletemodule->get_modulename(),
                                              'iteminstance' => $deletemodule->moduleinstanceid,
                                              'courseid' => $deletemodule->courseid))) {

            // Get count of grades for this grade item & add to record.
            foreach ($records as $rkey => $record) {
                $gradescount = $DB->count_records('grade_grades', array('itemid' => $rkey));
                $recordarray = (array) $record;
                $recordarray = array('grades_count' => "$gradescount") + $recordarray;
                $records[$rkey] = (object) $recordarray;
            }

[tuanngocnguyen]

Hi @djarran

Regards
This was added as part of https://github.com/catalyst/moodle-tool_fix_delete_modules/pull/22 to fix unit tests (unit tests were retested after removing the changes to this function and they still pass)
As in the comment // This is only required if MDL-80930 is not applied. So it will fail unit test if MDL-80930 is not applied.
This is for backward compatibility where MDL-80930 is not applied, that is, to make the unit test work regardless MDL-80930 is applied or not

        if (is_null($deletemodule->moduleinstanceid) {
            unset($this->deletemodules[$key]);
        }

probably, this should be run on unit test only

[djarran]

Hi @tuanngocnguyen, thanks for the clarification. While maybe it should be run on unit test only, it looks like we've hit a bit of a weird edge case / bug with the specific course module in question.

{"cms":{"14587":{"id":"14587"}},"userid":"401","realuserid":"401"}

The $this->deletemodules array is populated by the set_deletemodules_from_customdata function:

    public function set_deletemodules_from_customdata(\stdClass $customdata) {
        global $DB;
        ....
        $cms = (array) $customdata->cms;
        $this->deletemodules = array();
        foreach ($cms as $cmdata) {
            $instanceid = isset($cmdata->instance) ? $cmdata->instance : null;
            if (!isset($instanceid)) { // Attempt to retrieve from database.
                if ($record = $DB->get_field('course_modules', 'instance', array('id' => $cmdata->id))) {
                    $instanceid = $record;
                }
            }

With MDL-80930 applied, it seems like the instance is no longer passed to $cmdata (will need to verify this to see if it ever was), hence the $instanceid is initially set to null from the ternary. As such, we query the database to get the instance column in the course_modules table. This is what the table looks like:

select id, course, module, instance from mdl_course_modules where id = 14587;
  id    | course   | module    | instance    |
 14587  |    647   |     67    |        0    | 

So once we do this, $record will be (string) "0". This is a falsy value, and so the conditional is evaluated to false, and the instance remains null