From eadcdee9365211414cd401d90765951ad39b9add Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Mon, 6 Oct 2014 17:40:12 +0200 Subject: [PATCH 1/2] MDL-46647 grades: Fix fetch_all_helper() towards cross-db That helper, used to fetch information from DB by all the grade_object chidren classes was not behaving properly handling TEXT/CLOB columns. Instead of creating a property within every class listing the existing columns, it seems to be a better solution to instrospect the database metadata (cached) to ensure the correct SQL is generated in every case. --- lib/grade/grade_object.php | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/grade/grade_object.php b/lib/grade/grade_object.php index cceb178a58a..1b9e349ecda 100644 --- a/lib/grade/grade_object.php +++ b/lib/grade/grade_object.php @@ -177,6 +177,8 @@ abstract class grade_object { * @return array|bool Array of object instances or false if not found */ public static function fetch_all_helper($table, $classname, $params) { + global $DB; // Need to introspect DB here. + $instance = new $classname(); $classvars = (array)$instance; @@ -185,14 +187,25 @@ abstract class grade_object { $wheresql = array(); $newparams = array(); + $columns = $DB->get_columns($table); // Cached, no worries. + foreach ($params as $var=>$value) { if (!in_array($var, $instance->required_fields) and !array_key_exists($var, $instance->optional_fields)) { continue; } + if (!array_key_exists($var, $columns)) { // TODO: Should this throw a coding exception? + continue; + } if (is_null($value)) { $wheresql[] = " $var IS NULL "; } else { - $wheresql[] = " $var = ? "; + if ($columns[$var]->meta_type === 'X') { + // We have a text/clob column, use the cross-db method for its comparison. + $wheresql[] = ' ' . $DB->sql_compare_text($var) . ' = ' . $DB->sql_compare_text('?') . ' '; + } else { + // Other columns (varchar, integers...). + $wheresql[] = " $var = ? "; + } $newparams[] = $value; } } From 26331e3d17652eafb59e754fab4bcd67b0baa437 Mon Sep 17 00:00:00 2001 From: Frederic Massart Date: Tue, 7 Oct 2014 10:43:40 +0800 Subject: [PATCH 2/2] MDL-46647 core_grades: Adding tests for fetch_all_helper() --- lib/grade/grade_object.php | 2 +- lib/grade/tests/grade_object_test.php | 77 +++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 lib/grade/tests/grade_object_test.php diff --git a/lib/grade/grade_object.php b/lib/grade/grade_object.php index 1b9e349ecda..ca6b87b2abb 100644 --- a/lib/grade/grade_object.php +++ b/lib/grade/grade_object.php @@ -193,7 +193,7 @@ abstract class grade_object { if (!in_array($var, $instance->required_fields) and !array_key_exists($var, $instance->optional_fields)) { continue; } - if (!array_key_exists($var, $columns)) { // TODO: Should this throw a coding exception? + if (!array_key_exists($var, $columns)) { continue; } if (is_null($value)) { diff --git a/lib/grade/tests/grade_object_test.php b/lib/grade/tests/grade_object_test.php new file mode 100644 index 00000000000..f4e42586032 --- /dev/null +++ b/lib/grade/tests/grade_object_test.php @@ -0,0 +1,77 @@ +. + +/** + * Grade object tests. + * + * @package core_grades + * @category phpunit + * @copyright 2014 Frédéric Massart - FMCorz.net + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +require_once(__DIR__ . '/fixtures/lib.php'); + +/** + * Grade object testcase. + * + * @package core_grades + * @category phpunit + * @copyright 2014 Frédéric Massart - FMCorz.net + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class core_grade_object_testcase extends grade_base_testcase { + + public function test_fetch_all_helper() { + // Simple ID lookup. + $params = array('id' => $this->grade_items[0]->id); + $items = grade_object::fetch_all_helper('grade_items', 'grade_item', $params); + $this->assertCount(1, $items); + $item = array_shift($items); + $this->assertInstanceOf('grade_item', $item); + $this->assertEquals($item->id, $this->grade_items[0]->id); + + // Various parameters lookup, multiple results. + $params = array('courseid' => $this->course->id, 'categoryid' => $this->grade_categories[1]->id); + $items = grade_object::fetch_all_helper('grade_items', 'grade_item', $params); + $this->assertCount(2, $items); + $expecteditems = array($this->grade_items[0]->id => true, $this->grade_items[1]->id => true); + foreach ($items as $item) { + $this->assertInstanceOf('grade_item', $item); + $this->assertArrayHasKey($item->id, $expecteditems); + unset($expecteditems[$item->id]); + } + + // Text column lookup. + $params = array('iteminfo' => $this->grade_items[2]->iteminfo); + $items = grade_object::fetch_all_helper('grade_items', 'grade_item', $params); + $this->assertCount(1, $items); + $item = array_shift($items); + $this->assertInstanceOf('grade_item', $item); + $this->assertEquals($item->id, $this->grade_items[2]->id); + + // Lookup using non-existing columns. + $params = array('doesnotexist' => 'ignoreme', 'id' => $this->grade_items[0]->id); + $items = grade_object::fetch_all_helper('grade_items', 'grade_item', $params); + $this->assertCount(1, $items); + $item = array_shift($items); + $this->assertInstanceOf('grade_item', $item); + $this->assertEquals($item->id, $this->grade_items[0]->id); + } + +}