diff --git a/.upgradenotes/MDL-83718-2024111916571879.yml b/.upgradenotes/MDL-83718-2024111916571879.yml new file mode 100644 index 00000000000..ee1b8fffc85 --- /dev/null +++ b/.upgradenotes/MDL-83718-2024111916571879.yml @@ -0,0 +1,9 @@ +issueNumber: MDL-83718 +notes: + core_reportbuilder: + - message: >- + Report table instances no longer populate the `countsql` and + `countparams` class properties. Instead calling code can access + `totalrows` to obtain the same value, rather than manually counting via + the prior properties + type: changed diff --git a/reportbuilder/classes/external/custom_report_data_exporter.php b/reportbuilder/classes/external/custom_report_data_exporter.php index 3faf5777e2c..c39765ba106 100644 --- a/reportbuilder/classes/external/custom_report_data_exporter.php +++ b/reportbuilder/classes/external/custom_report_data_exporter.php @@ -77,7 +77,6 @@ class custom_report_data_exporter extends exporter { * @return array */ protected function get_other_values(renderer_base $output): array { - global $DB; /** @var datasource $report */ $report = $this->related['report']; @@ -101,7 +100,7 @@ class custom_report_data_exporter extends exporter { return [ 'headers' => $table->headers, 'rows' => $tablerows, - 'totalrowcount' => $DB->count_records_sql($table->countsql, $table->countparams), + 'totalrowcount' => $table->totalrows, ]; } } diff --git a/reportbuilder/classes/external/system_report_data_exporter.php b/reportbuilder/classes/external/system_report_data_exporter.php index 58389e28fc7..e8d1e1ab411 100644 --- a/reportbuilder/classes/external/system_report_data_exporter.php +++ b/reportbuilder/classes/external/system_report_data_exporter.php @@ -77,7 +77,6 @@ class system_report_data_exporter extends exporter { * @return array */ protected function get_other_values(renderer_base $output): array { - global $DB; /** @var system_report $report */ $report = $this->related['report']; @@ -109,7 +108,7 @@ class system_report_data_exporter extends exporter { return [ 'headers' => array_values($tableheaders), 'rows' => $tablerows, - 'totalrowcount' => $DB->count_records_sql($table->countsql, $table->countparams), + 'totalrowcount' => $table->totalrows, ]; } } diff --git a/reportbuilder/classes/local/helpers/schedule.php b/reportbuilder/classes/local/helpers/schedule.php index 86ba8d5b6cd..e92498f38a2 100644 --- a/reportbuilder/classes/local/helpers/schedule.php +++ b/reportbuilder/classes/local/helpers/schedule.php @@ -143,12 +143,11 @@ class schedule { * @return int */ public static function get_schedule_report_count(model $schedule): int { - global $DB; - $table = custom_report_table_view::create($schedule->get('reportid')); $table->setup(); + $table->query_db(0, false); - return $DB->count_records_sql($table->countsql, $table->countparams); + return $table->totalrows; } /** @@ -163,7 +162,6 @@ class schedule { require_once("{$CFG->libdir}/filelib.php"); $table = custom_report_table_view::create($schedule->get('reportid')); - $table->setup(); $table->query_db(0, false); diff --git a/reportbuilder/classes/local/report/column.php b/reportbuilder/classes/local/report/column.php index 54501bc91f5..f0d96f85aff 100644 --- a/reportbuilder/classes/local/report/column.php +++ b/reportbuilder/classes/local/report/column.php @@ -578,14 +578,11 @@ final class column { } // Check whether sortfield refers to field SQL. - foreach ($fieldsalias as $field) { - if (strcasecmp($sortfield, $field['sql']) === 0) { - $sortfield = $field['alias']; - break; - } - } - - return $sortfield; + return str_ireplace( + array_column($fieldsalias, 'sql'), + array_column($fieldsalias, 'alias'), + $sortfield, + ); }, $this->sortfields); } diff --git a/reportbuilder/classes/table/base_report_table.php b/reportbuilder/classes/table/base_report_table.php index 3ca83e63b27..7bf5c954fa9 100644 --- a/reportbuilder/classes/table/base_report_table.php +++ b/reportbuilder/classes/table/base_report_table.php @@ -116,15 +116,6 @@ abstract class base_report_table extends table_sql implements dynamic, renderabl $from .= ' ' . implode(' ', array_unique($joins)); $this->set_sql($fields, $from, $wheresql, $params); - - $counttablealias = database::generate_alias(); - $this->set_count_sql(" - SELECT COUNT(1) - FROM (SELECT {$fields} - FROM {$from} - WHERE {$wheresql} - {$this->groupbysql} - ) {$counttablealias}", $params); } /** @@ -165,13 +156,13 @@ abstract class base_report_table extends table_sql implements dynamic, renderabl /** * Generate suitable SQL for the table * + * @param bool $includesort * @return string */ - protected function get_table_sql(): string { + protected function get_table_sql(bool $includesort = true): string { $sql = "SELECT {$this->sql->fields} FROM {$this->sql->from} WHERE {$this->sql->where} {$this->groupbysql}"; - $sort = $this->get_sql_sort(); - if ($sort) { + if ($includesort && ($sort = $this->get_sql_sort())) { $sql .= " ORDER BY {$sort}"; } @@ -188,10 +179,25 @@ abstract class base_report_table extends table_sql implements dynamic, renderabl global $DB; if (!$this->is_downloading()) { - $this->pagesize($pagesize, $DB->count_records_sql($this->countsql, $this->countparams)); - $this->rawdata = $DB->get_recordset_sql($this->get_table_sql(), $this->sql->params, $this->get_page_start(), - $this->get_page_size()); + // Initially set the page size, so the following SQL read has correct values. + $this->pagesize($pagesize, 0); + + $countedcolumn = database::generate_alias(); + $countedrecordset = $DB->get_counted_recordset_sql( + $this->get_table_sql(false), + $countedcolumn, + $this->get_sql_sort(), + $this->sql->params, + (int) $this->get_page_start(), + (int) $this->get_page_size(), + ); + + // Now set the total page size. + $countedsize = (int) ($countedrecordset->current()->{$countedcolumn} ?? 0); + $this->pagesize($pagesize, $countedsize); + + $this->rawdata = $countedrecordset; } else { $this->rawdata = $DB->get_recordset_sql($this->get_table_sql(), $this->sql->params); } diff --git a/reportbuilder/tests/local/report/column_test.php b/reportbuilder/tests/local/report/column_test.php index 8ad01c67e20..10b82f62745 100644 --- a/reportbuilder/tests/local/report/column_test.php +++ b/reportbuilder/tests/local/report/column_test.php @@ -475,7 +475,7 @@ final class column_test extends advanced_testcase { /** * Test retrieving sort fields */ - public function test_get_sortfields(): void { + public function test_get_sort_fields(): void { $column = $this->create_column('test') ->set_index(1) ->add_fields('t.foo, t.bar, t.baz') @@ -487,7 +487,7 @@ final class column_test extends advanced_testcase { /** * Test retrieving sort fields when an aliased field is set as sortable */ - public function test_get_sortfields_with_field_alias(): void { + public function test_get_sort_fields_with_field_alias(): void { $column = $this->create_column('test') ->set_index(1) ->add_field('t.foo') @@ -497,10 +497,22 @@ final class column_test extends advanced_testcase { $this->assertEquals(['c1_lionel'], $column->get_sort_fields()); } + /** + * Test retrieving sort fields that contain references to fields within complex snippet + */ + public function test_get_sort_fields_complex(): void { + $column = $this->create_column('test') + ->set_index(1) + ->add_fields('t.foo, t.bar') + ->set_is_sortable(true, ['CASE WHEN 1=1 THEN t.foo ELSE t.bar END']); + + $this->assertEquals(['CASE WHEN 1=1 THEN c1_foo ELSE c1_bar END'], $column->get_sort_fields()); + } + /** * Test retrieving sort fields when an unknown field is set as sortable */ - public function test_get_sortfields_unknown_field(): void { + public function test_get_sort_fields_unknown_field(): void { $column = $this->create_column('test') ->set_index(1) ->add_fields('t.foo')