-
Bug
-
Resolution: Fixed
-
Minor
-
3.11.11, 4.0.5, 4.1
When bulk deleting users there is a situation where a dmlexception can be thrown if a bad record is hit.
public function delete($source = null) { |
global $DB;
|
$transaction = $DB->start_delegated_transaction();
|
$success = parent::delete($source);
|
// If the grade was deleted successfully trigger a grade_deleted event. if ($success && !empty($this->grade_item)) { |
\core\event\grade_deleted::create_from_grade($this)->trigger(); |
}
|
|
$transaction->allow_commit();
|
return $success; |
}
|
Because the exception is thrown from the parent::delete($source) the allow_commit() is never executed resulting in the rest of the bulk process falling under the transaction that will never be resolved and thus rolled back on process exit. For us, this prevented all future user deletions from occurring.
This is our current solution in case anyone else is running into the same issue. I haven't got the time at the moment to write a test for this change. But I'll try to put something together if I find time in the next few weeks.
public function delete($source = null) { |
global $DB;
|
$transaction = $DB->start_delegated_transaction();
|
$success = false; |
try { |
$success = parent::delete($source);
|
// If the grade was deleted successfully trigger a grade_deleted event. |
if ($success && !empty($this->grade_item)) { |
$this->load_grade_item(); |
\core\event\grade_deleted::create_from_grade($this)->trigger(); |
}
|
$transaction->allow_commit();
|
} catch(Exception $e) { |
$DB->rollback_delegated_transaction($transaction, $e);
|
}
|
return $success; |
}
|
Technically, given that the dml exception is almost always going to be due to a missing record $transaction->allow_commit(); could be called in the catch, but I felt it was safer in the case where some other cause is to blame to simply unroll the delete. Would love feedback if anyone is more familiar with this area of the code.