From 39d606ec271d5742f8159f4dbfef683ed669f8d3 Mon Sep 17 00:00:00 2001
From: Paul Holden <paulh@moodle.com>
Date: Mon, 9 May 2022 11:00:56 +0100
Subject: [PATCH] MDL-74656 reportbuilder: ensure nulls preserved in column
 callbacks.

When concatenating column fields in order to perform aggregation on
them (e.g. group concatenation), we need to preserve all null values
in the data passed to each column callback.

Co-authored-by: Carlos Castillo <carlos.castillo@moodle.com>
---
 reportbuilder/classes/local/aggregation/base.php      | 11 ++++++++---
 .../classes/local/aggregation/groupconcat.php         | 11 +++++++++--
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/reportbuilder/classes/local/aggregation/base.php b/reportbuilder/classes/local/aggregation/base.php
index f175133970b..3997e02bef7 100644
--- a/reportbuilder/classes/local/aggregation/base.php
+++ b/reportbuilder/classes/local/aggregation/base.php
@@ -81,9 +81,14 @@ abstract class base {
      *
      * @param string[] $sqlfields
      * @param string $delimeter
+     * @param string $coalescechar
      * @return string
      */
-    final protected static function get_column_fields_concat(array $sqlfields, string $delimeter = ','): string {
+    final protected static function get_column_fields_concat(
+        array $sqlfields,
+        string $delimeter = ',',
+        string $coalescechar = ' '
+    ): string {
         global $DB;
 
         $concatfields = [];
@@ -99,8 +104,8 @@ abstract class base {
                     break;
             }
 
-            // Coalesce all the SQL fields, to remove all nulls.
-            $concatfields[] = "COALESCE({$sqlfield}, ' ')";
+            // Coalesce all the SQL fields. Ensure cross-DB compatibility, and that we always get string data back.
+            $concatfields[] = "COALESCE({$sqlfield}, '{$coalescechar}')";
             $concatfields[] = "'{$delimeter}'";
         }
 
diff --git a/reportbuilder/classes/local/aggregation/groupconcat.php b/reportbuilder/classes/local/aggregation/groupconcat.php
index 9d7daec241e..77c27b579f4 100644
--- a/reportbuilder/classes/local/aggregation/groupconcat.php
+++ b/reportbuilder/classes/local/aggregation/groupconcat.php
@@ -34,6 +34,9 @@ class groupconcat extends base {
     /** @var string Character to use as a delimeter between column fields */
     protected const COLUMN_FIELD_DELIMETER = '<|>';
 
+    /** @var string Character to use a null coalesce value */
+    protected const COLUMN_NULL_COALESCE = '<^>';
+
     /** @var string Character to use as a delimeter between field values */
     protected const FIELD_VALUE_DELIMETER = '<,>';
 
@@ -79,7 +82,7 @@ class groupconcat extends base {
             return parent::get_column_field_sql($sqlfields);
         }
 
-        return self::get_column_fields_concat($sqlfields, self::COLUMN_FIELD_DELIMETER);
+        return self::get_column_fields_concat($sqlfields, self::COLUMN_FIELD_DELIMETER, self::COLUMN_NULL_COALESCE);
     }
 
     /**
@@ -123,7 +126,11 @@ class groupconcat extends base {
                 continue;
             }
 
-            $originalvalue = array_combine($valuenames, $valuedata);
+            // Re-construct original values, also ensuring any nulls contained within are restored.
+            $originalvalue = array_map(static function(string $value): ?string {
+                return $value === self::COLUMN_NULL_COALESCE ? null : $value;
+            }, array_combine($valuenames, $valuedata));
+
             $originalfirstvalue = reset($originalvalue);
 
             // Once we've re-constructed each value, we can apply callbacks to it.