diff --git a/lib/dml/moodle_database.php b/lib/dml/moodle_database.php index 4828dcf4782..070f0cb9eff 100644 --- a/lib/dml/moodle_database.php +++ b/lib/dml/moodle_database.php @@ -924,6 +924,54 @@ abstract class moodle_database { } } + /** + * Ensures that limit params are numeric and positive integers, to be passed to the database. + * We explicitly treat null, '' and -1 as 0 in order to provide compatibility with how limit + * values have been passed historically. + * + * @param int $limitfrom Where to start results from + * @param int $limitnum How many results to return + * @return array Normalised limit params in array($limitfrom, $limitnum) + */ + protected function normalise_limit_from_num($limitfrom, $limitnum) { + global $CFG; + + // We explicilty treat these cases as 0. + if ($limitfrom === null || $limitfrom === '' || $limitfrom === -1) { + $limitfrom = 0; + } + if ($limitnum === null || $limitnum === '' || $limitnum === -1) { + $limitnum = 0; + } + + if ($CFG->debugdeveloper) { + if (!is_numeric($limitfrom)) { + $strvalue = var_export($limitfrom, true); + debugging("Non-numeric limitfrom parameter detected: $strvalue, did you pass the correct arguments?", + DEBUG_DEVELOPER); + } else if ($limitfrom < 0) { + debugging("Negative limitfrom parameter detected: $limitfrom, did you pass the correct arguments?", + DEBUG_DEVELOPER); + } + + if (!is_numeric($limitnum)) { + $strvalue = var_export($limitnum, true); + debugging("Non-numeric limitnum parameter detected: $strvalue, did you pass the correct arguments?", + DEBUG_DEVELOPER); + } else if ($limitnum < 0) { + debugging("Negative limitnum parameter detected: $limitnum, did you pass the correct arguments?", + DEBUG_DEVELOPER); + } + } + + $limitfrom = (int)$limitfrom; + $limitnum = (int)$limitnum; + $limitfrom = max(0, $limitfrom); + $limitnum = max(0, $limitnum); + + return array($limitfrom, $limitnum); + } + /** * Return tables in database WITHOUT current prefix. * @param bool $usecache if true, returns list of cached tables. diff --git a/lib/dml/mssql_native_moodle_database.php b/lib/dml/mssql_native_moodle_database.php index 70bd6f32bec..0a324b1c176 100644 --- a/lib/dml/mssql_native_moodle_database.php +++ b/lib/dml/mssql_native_moodle_database.php @@ -693,10 +693,9 @@ class mssql_native_moodle_database extends moodle_database { * @throws dml_exception A DML specific exception is thrown for any errors. */ public function get_recordset_sql($sql, array $params=null, $limitfrom=0, $limitnum=0) { - $limitfrom = (int)$limitfrom; - $limitnum = (int)$limitnum; - $limitfrom = ($limitfrom < 0) ? 0 : $limitfrom; - $limitnum = ($limitnum < 0) ? 0 : $limitnum; + + list($limitfrom, $limitnum) = $this->normalise_limit_from_num($limitfrom, $limitnum); + if ($limitfrom or $limitnum) { if ($limitnum >= 1) { // Only apply TOP clause if we have any limitnum (limitfrom offset is handled later) $fetch = $limitfrom + $limitnum; diff --git a/lib/dml/mysqli_native_moodle_database.php b/lib/dml/mysqli_native_moodle_database.php index 34320d95432..3cb62a33ea5 100644 --- a/lib/dml/mysqli_native_moodle_database.php +++ b/lib/dml/mysqli_native_moodle_database.php @@ -912,10 +912,8 @@ class mysqli_native_moodle_database extends moodle_database { * @throws dml_exception A DML specific exception is thrown for any errors. */ public function get_recordset_sql($sql, array $params=null, $limitfrom=0, $limitnum=0) { - $limitfrom = (int)$limitfrom; - $limitnum = (int)$limitnum; - $limitfrom = ($limitfrom < 0) ? 0 : $limitfrom; - $limitnum = ($limitnum < 0) ? 0 : $limitnum; + + list($limitfrom, $limitnum) = $this->normalise_limit_from_num($limitfrom, $limitnum); if ($limitfrom or $limitnum) { if ($limitnum < 1) { @@ -976,10 +974,8 @@ class mysqli_native_moodle_database extends moodle_database { * @throws dml_exception A DML specific exception is thrown for any errors. */ public function get_records_sql($sql, array $params=null, $limitfrom=0, $limitnum=0) { - $limitfrom = (int)$limitfrom; - $limitnum = (int)$limitnum; - $limitfrom = ($limitfrom < 0) ? 0 : $limitfrom; - $limitnum = ($limitnum < 0) ? 0 : $limitnum; + + list($limitfrom, $limitnum) = $this->normalise_limit_from_num($limitfrom, $limitnum); if ($limitfrom or $limitnum) { if ($limitnum < 1) { diff --git a/lib/dml/oci_native_moodle_database.php b/lib/dml/oci_native_moodle_database.php index 0c6f700d5b6..00f79d6576c 100644 --- a/lib/dml/oci_native_moodle_database.php +++ b/lib/dml/oci_native_moodle_database.php @@ -712,11 +712,7 @@ class oci_native_moodle_database extends moodle_database { */ private function get_limit_sql($sql, array $params = null, $limitfrom=0, $limitnum=0) { - $limitfrom = (int)$limitfrom; - $limitnum = (int)$limitnum; - $limitfrom = ($limitfrom < 0) ? 0 : $limitfrom; - $limitnum = ($limitnum < 0) ? 0 : $limitnum; - + list($limitfrom, $limitnum) = $this->normalise_limit_from_num($limitfrom, $limitnum); // TODO: Add the /*+ FIRST_ROWS */ hint if there isn't another hint if ($limitfrom and $limitnum) { diff --git a/lib/dml/pgsql_native_moodle_database.php b/lib/dml/pgsql_native_moodle_database.php index 5c369a73e24..2ee1635843c 100644 --- a/lib/dml/pgsql_native_moodle_database.php +++ b/lib/dml/pgsql_native_moodle_database.php @@ -681,10 +681,9 @@ class pgsql_native_moodle_database extends moodle_database { * @throws dml_exception A DML specific exception is thrown for any errors. */ public function get_recordset_sql($sql, array $params=null, $limitfrom=0, $limitnum=0) { - $limitfrom = (int)$limitfrom; - $limitnum = (int)$limitnum; - $limitfrom = ($limitfrom < 0) ? 0 : $limitfrom; - $limitnum = ($limitnum < 0) ? 0 : $limitnum; + + list($limitfrom, $limitnum) = $this->normalise_limit_from_num($limitfrom, $limitnum); + if ($limitfrom or $limitnum) { if ($limitnum < 1) { $limitnum = "ALL"; @@ -724,10 +723,9 @@ class pgsql_native_moodle_database extends moodle_database { * @throws dml_exception A DML specific exception is thrown for any errors. */ public function get_records_sql($sql, array $params=null, $limitfrom=0, $limitnum=0) { - $limitfrom = (int)$limitfrom; - $limitnum = (int)$limitnum; - $limitfrom = ($limitfrom < 0) ? 0 : $limitfrom; - $limitnum = ($limitnum < 0) ? 0 : $limitnum; + + list($limitfrom, $limitnum) = $this->normalise_limit_from_num($limitfrom, $limitnum); + if ($limitfrom or $limitnum) { if ($limitnum < 1) { $limitnum = "ALL"; diff --git a/lib/dml/sqlsrv_native_moodle_database.php b/lib/dml/sqlsrv_native_moodle_database.php index 6f1252486c3..f6cab36050f 100644 --- a/lib/dml/sqlsrv_native_moodle_database.php +++ b/lib/dml/sqlsrv_native_moodle_database.php @@ -762,10 +762,8 @@ class sqlsrv_native_moodle_database extends moodle_database { * @throws dml_exception A DML specific exception is thrown for any errors. */ public function get_recordset_sql($sql, array $params = null, $limitfrom = 0, $limitnum = 0) { - $limitfrom = (int)$limitfrom; - $limitnum = (int)$limitnum; - $limitfrom = max(0, $limitfrom); - $limitnum = max(0, $limitnum); + + list($limitfrom, $limitnum) = $this->normalise_limit_from_num($limitfrom, $limitnum); if ($limitfrom or $limitnum) { if ($limitnum >= 1) { // Only apply TOP clause if we have any limitnum (limitfrom offset is handled later) diff --git a/lib/dml/tests/dml_test.php b/lib/dml/tests/dml_test.php index ceed3dbc626..869d6baea57 100644 --- a/lib/dml/tests/dml_test.php +++ b/lib/dml/tests/dml_test.php @@ -4997,6 +4997,74 @@ class core_dml_testcase extends database_driver_testcase { $this->assertEquals(2, reset($records)->count); $this->assertEquals(2, end($records)->count); } + + /** + * Test debugging messages about invalid limit number values. + */ + public function test_invalid_limits_debugging() { + $DB = $this->tdb; + $dbman = $DB->get_manager(); + + // Setup test data. + $table = $this->get_test_table(); + $tablename = $table->getName(); + $table->add_field('id', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, XMLDB_SEQUENCE, null); + $table->add_field('course', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, '0'); + $table->add_key('primary', XMLDB_KEY_PRIMARY, array('id')); + $dbman->create_table($table); + $DB->insert_record($tablename, array('course' => '1')); + + // Verify that get_records_sql throws debug notices with invalid limit params. + $DB->get_records_sql("SELECT * FROM {{$tablename}}", null, 'invalid'); + $this->assertDebuggingCalled("Non-numeric limitfrom parameter detected: 'invalid', did you pass the correct arguments?"); + + $DB->get_records_sql("SELECT * FROM {{$tablename}}", null, 1, 'invalid'); + $this->assertDebuggingCalled("Non-numeric limitnum parameter detected: 'invalid', did you pass the correct arguments?"); + + // Verify that get_recordset_sql throws debug notices with invalid limit params. + $rs = $DB->get_recordset_sql("SELECT * FROM {{$tablename}}", null, 'invalid'); + $this->assertDebuggingCalled("Non-numeric limitfrom parameter detected: 'invalid', did you pass the correct arguments?"); + $rs->close(); + + $rs = $DB->get_recordset_sql("SELECT * FROM {{$tablename}}", null, 1, 'invalid'); + $this->assertDebuggingCalled("Non-numeric limitnum parameter detected: 'invalid', did you pass the correct arguments?"); + $rs->close(); + + // Verify that some edge cases do no create debugging messages. + // String form of integer values. + $DB->get_records_sql("SELECT * FROM {{$tablename}}", null, '1'); + $this->assertDebuggingNotCalled(); + $DB->get_records_sql("SELECT * FROM {{$tablename}}", null, 1, '2'); + $this->assertDebuggingNotCalled(); + // Empty strings. + $DB->get_records_sql("SELECT * FROM {{$tablename}}", null, ''); + $this->assertDebuggingNotCalled(); + $DB->get_records_sql("SELECT * FROM {{$tablename}}", null, 1, ''); + $this->assertDebuggingNotCalled(); + // Null values. + $DB->get_records_sql("SELECT * FROM {{$tablename}}", null, null); + $this->assertDebuggingNotCalled(); + $DB->get_records_sql("SELECT * FROM {{$tablename}}", null, 1, null); + $this->assertDebuggingNotCalled(); + + // Verify that empty arrays DO create debugging mesages. + $DB->get_records_sql("SELECT * FROM {{$tablename}}", null, array()); + $this->assertDebuggingCalled("Non-numeric limitfrom parameter detected: array (\n), did you pass the correct arguments?"); + $DB->get_records_sql("SELECT * FROM {{$tablename}}", null, 1, array()); + $this->assertDebuggingCalled("Non-numeric limitnum parameter detected: array (\n), did you pass the correct arguments?"); + + // Verify Negative number handling: + // -1 is explicitly treated as 0 for historical reasons. + $DB->get_records_sql("SELECT * FROM {{$tablename}}", null, -1); + $this->assertDebuggingNotCalled(); + $DB->get_records_sql("SELECT * FROM {{$tablename}}", null, 1, -1); + $this->assertDebuggingNotCalled(); + // Any other negative values should throw debugging messages. + $DB->get_records_sql("SELECT * FROM {{$tablename}}", null, -2); + $this->assertDebuggingCalled("Negative limitfrom parameter detected: -2, did you pass the correct arguments?"); + $DB->get_records_sql("SELECT * FROM {{$tablename}}", null, 1, -2); + $this->assertDebuggingCalled("Negative limitnum parameter detected: -2, did you pass the correct arguments?"); + } } /**