MDL-74675 reportbuilder: preserve column type during aggregation.

When applying callbacks during the group concatenation aggregation types, ensure the column type is preserved for it's default value.
2025-01-19 06:18:28 +01:00 · 2022-05-05 17:10:30 +01:00 · 2022-05-05 17:10:30 +01:00 · ea45aefe62
commit ea45aefe62
parent 6c114e2a80
11 changed files with 59 additions and 35 deletions
--- 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);
    }
 }
--- 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);
--- 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);
    }
 }
--- 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);
    }
 }
--- 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);
--- 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));
    }
 }
--- 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);
    }
 }
--- 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 <sara@moodle.com> based on Alberto Lara Hernández <albertolara@moodle.com> 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) . '%';
    }
 }
--- 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;
--- 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')
--- 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