diff --git a/badges/tests/reportbuilder/datasource/badges_test.php b/badges/tests/reportbuilder/datasource/badges_test.php index 03f90ff3b3f..3644a16522d 100644 --- a/badges/tests/reportbuilder/datasource/badges_test.php +++ b/badges/tests/reportbuilder/datasource/badges_test.php @@ -22,6 +22,7 @@ use core_badges_generator; use core_reportbuilder_generator; use core_reportbuilder_testcase; use core_reportbuilder\local\filters\{boolean_select, date, select, tags, text}; +use core_reportbuilder\manager; defined('MOODLE_INTERNAL') || die(); @@ -37,7 +38,7 @@ require_once("{$CFG->libdir}/badgeslib.php"); * @copyright 2022 Paul Holden * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class badges_test extends core_reportbuilder_testcase { +final class badges_test extends core_reportbuilder_testcase { /** * Test default datasource @@ -200,12 +201,53 @@ class badges_test extends core_reportbuilder_testcase { $this->assertEquals($course->fullname, $coursename); } + /** + * Test creating a report containing "expiry" as both a condition and a filter + * + * This is really testing that it's possible to do so, for a filter instance that returns SQL parameters + */ + public function test_report_expiry_condition_and_filter(): void { + $this->resetAfterTest(); + + /** @var core_badges_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_badges'); + + $badgeone = $generator->create_badge(['name' => 'Badge 1', 'expiredate' => 10]); + $badgetwo = $generator->create_badge(['name' => 'Badge 2', 'expiredate' => 20]); + $badgethree = $generator->create_badge(['name' => 'Badge 3', 'expiredate' => 30]); + + /** @var core_reportbuilder_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); + $report = $generator->create_report(['name' => 'Badges', 'source' => badges::class, 'default' => 0]); + + $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'badge:name']); + $generator->create_condition(['reportid' => $report->get('id'), 'uniqueidentifier' => 'badge:expiry']); + $generator->create_filter(['reportid' => $report->get('id'), 'uniqueidentifier' => 'badge:expiry']); + + // Load report instance, set condition. + $instance = manager::get_report_from_persistent($report); + $instance->set_condition_values([ + 'badge:expiry_operator' => date::DATE_RANGE, + 'badge:expiry_from' => 15, + ]); + + // Set filter. + $content = $this->get_custom_report_content($report->get('id'), 0, [ + 'badge:expiry_operator' => date::DATE_RANGE, + 'badge:expiry_to' => 25, + ]); + + $this->assertEquals([ + [$badgetwo->name], + ], array_map('array_values', $content)); + } + /** * Data provider for {@see test_datasource_filters} * * @return array[] */ - public function datasource_filters_provider(): array { + public static function datasource_filters_provider(): array { return [ // Badge. 'Filter badge name' => ['badge:name', [ diff --git a/reportbuilder/classes/local/helpers/database.php b/reportbuilder/classes/local/helpers/database.php index bb1e5a8ef1d..0277881230d 100644 --- a/reportbuilder/classes/local/helpers/database.php +++ b/reportbuilder/classes/local/helpers/database.php @@ -108,7 +108,7 @@ class database { * primarily to ensure uniqueness when the expression is to be used as part of a larger query * * @param string $sql - * @param array $params + * @param array $params Parameter names * @param callable $callback Method that takes a single string parameter, and returns another string * @return string */ @@ -126,6 +126,27 @@ class database { return $sql; } + /** + * Replace parameter names within given SQL expression, returning updated SQL and parameter elements + * + * {@see sql_replace_parameter_names} + * + * @param string $sql + * @param array $params Parameter name/values + * @param callable $callback + * @return array [$sql, $params] + */ + public static function sql_replace_parameters(string $sql, array $params, callable $callback): array { + $transformedsql = static::sql_replace_parameter_names($sql, array_keys($params), $callback); + + $transformedparams = []; + foreach ($params as $name => $value) { + $transformedparams[$callback($name)] = $value; + } + + return [$transformedsql, $transformedparams]; + } + /** * Generate SQL expression for sorting group concatenated fields * diff --git a/reportbuilder/classes/table/base_report_table.php b/reportbuilder/classes/table/base_report_table.php index 97e53f988d7..d33f8167bf1 100644 --- a/reportbuilder/classes/table/base_report_table.php +++ b/reportbuilder/classes/table/base_report_table.php @@ -74,10 +74,13 @@ abstract class base_report_table extends table_sql implements dynamic, renderabl $wheres[] = $where; } + // Track the index of conditions/filters as we iterate over them. + $conditionindex = $filterindex = 0; + // For each condition, we need to ensure their values are always accounted for in the report. $conditionvalues = $this->report->get_condition_values(); foreach ($this->report->get_active_conditions() as $condition) { - [$conditionsql, $conditionparams] = $this->get_filter_sql($condition, $conditionvalues); + [$conditionsql, $conditionparams] = $this->get_filter_sql($condition, $conditionvalues, 'c' . $conditionindex++); if ($conditionsql !== '') { $joins = array_merge($joins, $condition->get_joins()); $wheres[] = "({$conditionsql})"; @@ -89,7 +92,7 @@ abstract class base_report_table extends table_sql implements dynamic, renderabl if (!$this->editing) { $filtervalues = $this->report->get_filter_values(); foreach ($this->report->get_active_filters() as $filter) { - [$filtersql, $filterparams] = $this->get_filter_sql($filter, $filtervalues); + [$filtersql, $filterparams] = $this->get_filter_sql($filter, $filtervalues, 'f' . $filterindex++); if ($filtersql !== '') { $joins = array_merge($joins, $filter->get_joins()); $wheres[] = "({$filtersql})"; @@ -139,13 +142,24 @@ abstract class base_report_table extends table_sql implements dynamic, renderabl * * @param filter $filter * @param array $filtervalues + * @param string $paramprefix * @return array [$sql, $params] */ - private function get_filter_sql(filter $filter, array $filtervalues): array { + private function get_filter_sql(filter $filter, array $filtervalues, string $paramprefix): array { /** @var base $filterclass */ $filterclass = $filter->get_filter_class(); - return $filterclass::create($filter)->get_sql_filter($filtervalues); + // Retrieve SQL fragments from the filter instance, process parameters if required. + [$sql, $params] = $filterclass::create($filter)->get_sql_filter($filtervalues); + if ($paramprefix !== '' && count($params) > 0) { + return database::sql_replace_parameters( + $sql, + $params, + fn(string $param) => "{$paramprefix}_{$param}", + ); + } + + return [$sql, $params]; } /** diff --git a/reportbuilder/tests/local/helpers/database_test.php b/reportbuilder/tests/local/helpers/database_test.php index a7a89f84f3c..174b03136f6 100644 --- a/reportbuilder/tests/local/helpers/database_test.php +++ b/reportbuilder/tests/local/helpers/database_test.php @@ -30,7 +30,7 @@ use core_user; * @copyright 2020 Paul Holden * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class database_test extends advanced_testcase { +final class database_test extends advanced_testcase { /** * Test generating alias @@ -176,9 +176,11 @@ class database_test extends advanced_testcase { [$param0, $param1, $param10] = ['rbparam0', 'rbparam1', 'rbparam10']; $sql = "SELECT :{$param0} AS field0, :{$param1} AS field1, :{$param10} AS field10" . $DB->sql_null_from_clause(); - $sql = database::sql_replace_parameter_names($sql, [$param0, $param1, $param10], static function(string $param): string { - return "prefix_{$param}"; - }); + $sql = database::sql_replace_parameter_names( + $sql, + [$param0, $param1, $param10], + fn(string $param) => "prefix_{$param}", + ); $record = $DB->get_record_sql($sql, [ "prefix_{$param0}" => 'Zero', @@ -192,4 +194,29 @@ class database_test extends advanced_testcase { 'field10' => 'Ten', ], $record); } + + /** + * Test replacement of parameter names within query, returning both modified query and parameters + */ + public function test_sql_replace_parameters(): void { + global $DB; + + // Predefine parameter names, to ensure they don't overwrite each other. + [$param0, $param1, $param10] = ['rbparam0', 'rbparam1', 'rbparam10']; + + $sql = "SELECT :{$param0} AS field0, :{$param1} AS field1, :{$param10} AS field10" . $DB->sql_null_from_clause(); + [$sql, $params] = database::sql_replace_parameters( + $sql, + [$param0 => 'Zero', $param1 => 'One', $param10 => 'Ten'], + fn(string $param) => "prefix_{$param}", + ); + + $record = $DB->get_record_sql($sql, $params); + + $this->assertEquals((object) [ + 'field0' => 'Zero', + 'field1' => 'One', + 'field10' => 'Ten', + ], $record); + } } diff --git a/reportbuilder/upgrade.txt b/reportbuilder/upgrade.txt index 3e158489546..97fc4a0b596 100644 --- a/reportbuilder/upgrade.txt +++ b/reportbuilder/upgrade.txt @@ -3,6 +3,7 @@ Information provided here is intended especially for developers. === 4.5 === +* New database helper method `sql_replace_parameters` to help ensure uniqueness of parameters within a SQL expression * The following external methods now return tags data relevant to each custom report: - `core_reportbuilder_list_reports` - `core_reportbuilder_retrieve_report`