Details
-
Type:
Bug
-
Status: Closed
-
Priority:
Minor
-
Resolution: Fixed
-
Affects Version/s: 3.1.1, 3.2, 3.3
-
Component/s: Course
-
Testing Instructions:
-
Affected Branches:MOODLE_31_STABLE, MOODLE_32_STABLE, MOODLE_33_STABLE
-
Fixed Branches:MOODLE_31_STABLE, MOODLE_32_STABLE
-
Pull from Repository:
-
Pull Master Branch:
-
Pull Master Diff URL:
Description
Consider this code snippet from course/modlib.php
try { |
$returnfromfunc = $addinstancefunction($moduleinfo, $mform); |
} catch (moodle_exception $e) { |
$returnfromfunc = $e; |
}
|
if (!$returnfromfunc or !is_number($returnfromfunc)) { |
// Undo everything we can. This is not necessary for databases which |
// support transactions, but improves consistency for other databases. |
$modcontext = context_module::instance($moduleinfo->coursemodule); |
context_helper::delete_instance(CONTEXT_MODULE, $moduleinfo->coursemodule); |
$DB->delete_records('course_modules', array('id'=>$moduleinfo->coursemodule)); |
|
|
if ($e instanceof moodle_exception) { |
throw $e; |
} else if (!is_number($returnfromfunc)) { |
print_error('invalidfunction', '', course_get_url($course, $moduleinfo->section)); |
} else { |
print_error('cannotaddnewmodule', '', course_get_url($course, $moduleinfo->section), $moduleinfo->modulename); |
}
|
}
|
It's entirely possible to reach the test for variable $e (if ($e instanceof...) without $e being defined thus throwing an error. The 'try' at the start of the snippet could quite easily return $returnfromfunc as a 'false' value without throwing an exception... in which case $e would not exist.
The test for $e needs an additional isset() or similar check to be safe.
Turned up in a help request in the forums.
Just my $00.02 but (unless you are dealing with some external system over which you have no control) try...catch is almost always a very bad idea and should be removed.