diff --git a/reportbuilder/classes/local/aggregation/avg.php b/reportbuilder/classes/local/aggregation/avg.php index 16ff16a9c4b..57d73b13a2b 100644 --- a/reportbuilder/classes/local/aggregation/avg.php +++ b/reportbuilder/classes/local/aggregation/avg.php @@ -70,9 +70,10 @@ class avg extends base { * @param mixed $value * @param array $values * @param array $callbacks - * @return mixed + * @param int $columntype + * @return string */ - public static function format_value($value, array $values, array $callbacks) { + public static function format_value($value, array $values, array $callbacks, int $columntype): string { return format_float((float) reset($values), 1); } } diff --git a/reportbuilder/classes/local/aggregation/base.php b/reportbuilder/classes/local/aggregation/base.php index f175133970b..6ddb1316fa8 100644 --- a/reportbuilder/classes/local/aggregation/base.php +++ b/reportbuilder/classes/local/aggregation/base.php @@ -126,9 +126,10 @@ abstract class base { * @param mixed $value * @param array $values * @param array $callbacks Array of column callbacks, {@see column::add_callback} for definition + * @param int $columntype The original type of the column, to ensure it is preserved for callbacks * @return mixed */ - public static function format_value($value, array $values, array $callbacks) { + public static function format_value($value, array $values, array $callbacks, int $columntype) { foreach ($callbacks as $callback) { [$callable, $arguments] = $callback; $value = ($callable)($value, (object) $values, $arguments); diff --git a/reportbuilder/classes/local/aggregation/count.php b/reportbuilder/classes/local/aggregation/count.php index e3bb2108088..e93d400c413 100644 --- a/reportbuilder/classes/local/aggregation/count.php +++ b/reportbuilder/classes/local/aggregation/count.php @@ -72,9 +72,10 @@ class count extends base { * @param mixed $value * @param array $values * @param array $callbacks - * @return mixed + * @param int $columntype + * @return int */ - public static function format_value($value, array $values, array $callbacks) { + public static function format_value($value, array $values, array $callbacks, int $columntype): int { return (int) reset($values); } } diff --git a/reportbuilder/classes/local/aggregation/countdistinct.php b/reportbuilder/classes/local/aggregation/countdistinct.php index ffcd20011e5..3ddd961da09 100644 --- a/reportbuilder/classes/local/aggregation/countdistinct.php +++ b/reportbuilder/classes/local/aggregation/countdistinct.php @@ -86,9 +86,10 @@ class countdistinct extends base { * @param mixed $value * @param array $values * @param array $callbacks - * @return mixed + * @param int $columntype + * @return int */ - public static function format_value($value, array $values, array $callbacks) { + public static function format_value($value, array $values, array $callbacks, int $columntype): int { return (int) reset($values); } } diff --git a/reportbuilder/classes/local/aggregation/groupconcat.php b/reportbuilder/classes/local/aggregation/groupconcat.php index 9d7daec241e..e3572a9eba7 100644 --- a/reportbuilder/classes/local/aggregation/groupconcat.php +++ b/reportbuilder/classes/local/aggregation/groupconcat.php @@ -104,9 +104,10 @@ class groupconcat extends base { * @param mixed $value * @param array $values * @param array $callbacks + * @param int $columntype * @return mixed */ - public static function format_value($value, array $values, array $callbacks) { + public static function format_value($value, array $values, array $callbacks, int $columntype) { $formattedvalues = []; // Store original names of all values that would be present without aggregation. @@ -123,11 +124,11 @@ class groupconcat extends base { continue; } - $originalvalue = array_combine($valuenames, $valuedata); - $originalfirstvalue = reset($originalvalue); + $originalvalues = array_combine($valuenames, $valuedata); + $originalvalue = column::get_default_value($originalvalues, $columntype); // Once we've re-constructed each value, we can apply callbacks to it. - $formattedvalues[] = parent::format_value($originalfirstvalue, $originalvalue, $callbacks); + $formattedvalues[] = parent::format_value($originalvalue, $originalvalues, $callbacks, $columntype); } return implode(', ', $formattedvalues); diff --git a/reportbuilder/classes/local/aggregation/percent.php b/reportbuilder/classes/local/aggregation/percent.php index 29b9878196c..de10bfa00d0 100644 --- a/reportbuilder/classes/local/aggregation/percent.php +++ b/reportbuilder/classes/local/aggregation/percent.php @@ -69,9 +69,10 @@ class percent extends base { * @param mixed $value * @param array $values * @param array $callbacks - * @return mixed + * @param int $columntype + * @return string */ - public static function format_value($value, array $values, array $callbacks) { - return format::percent(reset($values)); + public static function format_value($value, array $values, array $callbacks, int $columntype): string { + return format::percent((float) reset($values)); } } diff --git a/reportbuilder/classes/local/aggregation/sum.php b/reportbuilder/classes/local/aggregation/sum.php index 119fb52f508..54a27754136 100644 --- a/reportbuilder/classes/local/aggregation/sum.php +++ b/reportbuilder/classes/local/aggregation/sum.php @@ -70,9 +70,10 @@ class sum extends base { * @param mixed $value * @param array $values * @param array $callbacks - * @return mixed + * @param int $columntype + * @return int */ - public static function format_value($value, array $values, array $callbacks) { + public static function format_value($value, array $values, array $callbacks, int $columntype): int { return (int) reset($values); } } diff --git a/reportbuilder/classes/local/helpers/format.php b/reportbuilder/classes/local/helpers/format.php index 0ac617e6e40..a727bed428f 100644 --- a/reportbuilder/classes/local/helpers/format.php +++ b/reportbuilder/classes/local/helpers/format.php @@ -23,9 +23,6 @@ use stdClass; /** * Class containing helper methods for formatting column data via callbacks * - * Note that type hints for each $value argument are avoided to allow for these callbacks to be executed when columns are - * aggregated using one of the "Group concatenation" methods, where the value is typically stringified - * * @package core_reportbuilder * @copyright 2021 Sara Arjona based on Alberto Lara Hernández code. * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later @@ -40,8 +37,8 @@ class format { * @param string|null $format Format string for strftime * @return string */ - public static function userdate($value, stdClass $row, ?string $format = null): string { - return $value ? userdate((int) $value, $format) : ''; + public static function userdate(int $value, stdClass $row, ?string $format = null): string { + return $value ? userdate($value, $format) : ''; } /** @@ -50,8 +47,8 @@ class format { * @param bool $value * @return string */ - public static function boolean_as_text($value): string { - return (bool) $value ? get_string('yes') : get_string('no'); + public static function boolean_as_text(bool $value): string { + return $value ? get_string('yes') : get_string('no'); } /** @@ -60,7 +57,7 @@ class format { * @param float $value * @return string */ - public static function percent($value): string { - return format_float((float) $value, 1) . '%'; + public static function percent(float $value): string { + return format_float($value, 1) . '%'; } } diff --git a/reportbuilder/classes/local/report/column.php b/reportbuilder/classes/local/report/column.php index d0177c3e4e6..fc174af00af 100644 --- a/reportbuilder/classes/local/report/column.php +++ b/reportbuilder/classes/local/report/column.php @@ -645,13 +645,14 @@ final class column { * Return the default column value, that being the value of it's first field * * @param array $values + * @param int $columntype * @return mixed */ - private function get_default_value(array $values) { + public static function get_default_value(array $values, int $columntype) { $value = reset($values); // Ensure default value is cast to it's strict type. - switch ($this->get_type()) { + switch ($columntype) { case self::TYPE_INTEGER: case self::TYPE_TIMESTAMP: $value = (int) $value; @@ -675,11 +676,11 @@ final class column { */ public function format_value(array $row) { $values = $this->get_values($row); - $value = $this->get_default_value($values); + $value = self::get_default_value($values, $this->type); // If column is being aggregated then defer formatting to them, otherwise loop through all column callbacks. if (!empty($this->aggregation)) { - $value = $this->aggregation::format_value($value, $values, $this->callbacks); + $value = $this->aggregation::format_value($value, $values, $this->callbacks, $this->type); } else { foreach ($this->callbacks as $callback) { [$callable, $arguments] = $callback; diff --git a/reportbuilder/tests/local/report/column_test.php b/reportbuilder/tests/local/report/column_test.php index 4442747fd54..721b1a7b3b8 100644 --- a/reportbuilder/tests/local/report/column_test.php +++ b/reportbuilder/tests/local/report/column_test.php @@ -311,11 +311,11 @@ class column_test extends advanced_testcase { } /** - * Data provider for {@see test_format_value} + * Data provider for {@see test_get_default_value} and {@see test_format_value} * * @return array[] */ - public function format_value_provider(): array { + public function column_type_provider(): array { return [ [column::TYPE_INTEGER, 42], [column::TYPE_TEXT, 'Hello'], @@ -327,13 +327,31 @@ class column_test extends advanced_testcase { } /** - * Test that column value is returned as correctly (value plus type) + * Test default value is returned from selected values, with correct type * * @param int $columntype * @param mixed $value * @param mixed|null $expected Expected value, or null to indicate it should be identical to value * - * @dataProvider format_value_provider + * @dataProvider column_type_provider + */ + public function test_get_default_value(int $columntype, $value, $expected = null): void { + $defaultvalue = column::get_default_value([ + 'value' => $value, + 'foo' => 'bar', + ], $columntype); + + $this->assertSame($expected ?? $value, $defaultvalue); + } + + /** + * Test that column value is returned correctly, with correct type + * + * @param int $columntype + * @param mixed $value + * @param mixed|null $expected Expected value, or null to indicate it should be identical to value + * + * @dataProvider column_type_provider */ public function test_format_value(int $columntype, $value, $expected = null): void { $column = $this->create_column('test') diff --git a/reportbuilder/upgrade.txt b/reportbuilder/upgrade.txt index 098f51098a0..6f350d353a6 100644 --- a/reportbuilder/upgrade.txt +++ b/reportbuilder/upgrade.txt @@ -3,9 +3,10 @@ Information provided here is intended especially for developers. === 4.1 === * 'set_default_per_page' and 'get_default_per_page' methods have been added to \local\report\base class - to manage the default displayed rows per page. + to manage the default displayed rows per page. * Added two new methods in the datasource class: - add_all_from_entity() to add all columns/filters/conditions from the given entity to the report at once - add_all_from_entities() to add all columns/filters/conditions from all the entities added to the report at once -======= * New database helper methods for generating multiple unique values: `generate_aliases` and `generate_param_names` +* The base aggregation `format_value` method has a `$columntype` argument in order to preserve type during aggregation. When + defining column callbacks, strict typing will now be preserved in your callback methods when the column is being aggregated