From fdf40310146c4dcd12205e0a1dd0cb9932b10477 Mon Sep 17 00:00:00 2001
From: Mark Johnson <mark.johnson@catalyst-eu.net>
Date: Wed, 9 Jul 2025 09:47:46 +0100
Subject: [PATCH] MDL-85975 backup: Handle nulls and arrays in restored
 questiondata

unset_excluded_fields() was using isset() to determine if the field
targeted for removal is present in the provided data structure. However,
isset() returns false if the field exists, but contains null. This means
the field will not be unset when it should be.

This resolves this by changing the isset() to a property_exists() check
for objects, and array_key_exists() check for arrays. It also expands
the function to properly handle arrays of arrays, or arrays of values,
which was not fully covered before and is used by some third-party
question types.
---
 .upgradenotes/MDL-85975-2025092314370040.yml  |  17 ++
 .../moodle2/restore_qtype_plugin.class.php    |  62 ++++++--
 .../tests/restore_qtype_plugin_test.php       | 147 ++++++++++++++++++
 3 files changed, 212 insertions(+), 14 deletions(-)
 create mode 100644 .upgradenotes/MDL-85975-2025092314370040.yml
 create mode 100644 public/backup/moodle2/tests/restore_qtype_plugin_test.php

diff --git a/.upgradenotes/MDL-85975-2025092314370040.yml b/.upgradenotes/MDL-85975-2025092314370040.yml
new file mode 100644
index 00000000000..263b753aca2
--- /dev/null
+++ b/.upgradenotes/MDL-85975-2025092314370040.yml
@@ -0,0 +1,17 @@
+issueNumber: MDL-85975
+notes:
+  core:
+    - message: >
+        `restore_qtype_plugin::unset_excluded_fields` now returns the modified
+        questiondata structure,
+
+        in order to support structures that contain arrays.
+
+        If your qtype plugin overrides
+        `restore_qtype_plugin::remove_excluded_question_data` without
+
+        calling the parent method, you may need to modify your overridden method
+        to use the returned
+
+        value.
+      type: fixed
diff --git a/public/backup/moodle2/restore_qtype_plugin.class.php b/public/backup/moodle2/restore_qtype_plugin.class.php
index 52b10c6a548..4b80a13a022 100644
--- a/public/backup/moodle2/restore_qtype_plugin.class.php
+++ b/public/backup/moodle2/restore_qtype_plugin.class.php
@@ -568,8 +568,7 @@ abstract class restore_qtype_plugin extends restore_plugin {
 
         foreach ($excludefields as $excludefield) {
             $pathparts = explode('/', ltrim($excludefield, '/'));
-            $data = $questiondata;
-            self::unset_excluded_fields($data, $pathparts);
+            $questiondata = self::unset_excluded_fields($questiondata, $pathparts);
         }
 
         return $questiondata;
@@ -581,25 +580,60 @@ abstract class restore_qtype_plugin extends restore_plugin {
      * If any of the elements in the path is an array, this is called recursively on each element in the array to unset fields
      * in each child of the array.
      *
-     * @param stdClass|array $data The questiondata object, or a subsection of it.
+     * @param stdClass|array $data The questiondata structure, or a subsection of it.
      * @param array $pathparts The remaining elements in the path to the excluded field.
-     * @return void
+     * @return stdClass|array The $data structure with excluded fields removed.
      */
-    private static function unset_excluded_fields(stdClass|array $data, array $pathparts): void {
+    private static function unset_excluded_fields(stdClass|array $data, array $pathparts): stdClass|array {
         $element = array_shift($pathparts);
-        if (!isset($data->{$element})) {
-            // This element is not present in the data structure, nothing to unset.
-            return;
+        $unset = false;
+        // Get the current element from the data structure.
+        if (is_object($data)) {
+            if (!property_exists($data, $element)) {
+                // This element is not present in the data structure, nothing to unset.
+                return $data;
+            }
+            $dataelement = $data->{$element};
+        } else { // It's an array.
+            if (!array_key_exists($element, $data)) {
+                return $data;
+            }
+            $dataelement = $data[$element];
         }
-        if (is_object($data->{$element})) {
-            self::unset_excluded_fields($data->{$element}, $pathparts);
-        } else if (is_array($data->{$element})) {
-            foreach ($data->{$element} as $item) {
-                self::unset_excluded_fields($item, $pathparts);
+        // Check if we need to recur, or unset this element.
+        if (is_object($dataelement)) {
+            $dataelement = self::unset_excluded_fields($dataelement, $pathparts);
+        } else if (is_array($dataelement)) {
+            foreach ($dataelement as $key => $item) {
+                if (is_object($item) || is_array($item)) {
+                    // This is an array of objects or arrays, recur.
+                    $dataelement[$key] = self::unset_excluded_fields($item, $pathparts);
+                } else {
+                    // This is an associative array of values, check if they should be removed.
+                    $subelement = reset($pathparts);
+                    if ($key == $subelement) {
+                        unset($dataelement[$key]);
+                    }
+                }
             }
         } else if (empty($pathparts)) {
             // This is the last element of the path and it's a scalar value, unset it.
-            unset($data->{$element});
+            $unset = true;
         }
+        // Write the modified element back to the data structure, or unset it.
+        if (is_object($data)) {
+            if ($unset) {
+                unset($data->{$element});
+            } else {
+                $data->{$element} = $dataelement;
+            }
+        } else {
+            if ($unset) {
+                unset($data[$element]);
+            } else {
+                $data[$element] = $dataelement;
+            }
+        }
+        return $data;
     }
 }
diff --git a/public/backup/moodle2/tests/restore_qtype_plugin_test.php b/public/backup/moodle2/tests/restore_qtype_plugin_test.php
new file mode 100644
index 00000000000..527763281fe
--- /dev/null
+++ b/public/backup/moodle2/tests/restore_qtype_plugin_test.php
@@ -0,0 +1,147 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+namespace core;
+
+/**
+ * Tests for question type restore methods
+ *
+ * @package   core
+ * @copyright 2025 onwards Catalyst IT EU {@link https://catalyst-eu.net}
+ * @author    Mark Johnson <mark.johnson@catalyst-eu.net>
+ * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ * @covers    \restore_qtype_plugin
+ */
+final class restore_qtype_plugin_test extends \basic_testcase {
+    /**
+     * All default and specified fields should be removed from the provided data structure.
+     */
+    public function test_remove_excluded_question_data(): void {
+        global $CFG;
+        require_once($CFG->dirroot . '/backup/moodle2/restore_plugin.class.php');
+        require_once($CFG->dirroot . '/backup/moodle2/restore_qtype_plugin.class.php');
+        $data = (object) [
+            // Default excluded fields should be removed.
+            'id' => 1,
+            'createdby' => 2,
+            'modifiedby' => 3,
+            // This field is not specified for removal, it should remain.
+            'questiontext' => 'Some question text',
+            // Excluded paths that address an array should operate on all items in the array.
+            'hints' => [
+                (object) [
+                    'id' => 4,
+                    'questionid' => 1,
+                    // This field is not specified for removal.
+                    'text' => 'Lorem ipsum',
+                ],
+                (object) [
+                    'id' => 5,
+                    'questionid' => 1,
+                    'text' => 'Lorem ipsum',
+                ],
+            ],
+            'options' => [ // This is an array of arrays, rather than an array of objects. It should be handled the same.
+                [
+                    'id' => 6,
+                    'questionid' => 1,
+                    // This field is not specified for removal.
+                    'option' => true,
+                ],
+                [
+                    'id' => 7,
+                    'questionid' => 1,
+                    'option' => false,
+                ],
+                [
+                    'id' => 8,
+                    'questionid' => 1,
+                    'option' => false,
+                ],
+            ],
+            'custom1' => 'Some custom text',
+            // This field is not specified for removal.
+            'custom2' => 'Some custom text2',
+            // Fields specified for removal should be removed even if they contain null values.
+            'custom3' => null,
+            'customarray' => [
+                (object) [
+                    // Null values should also be removed.
+                    'id' => null,
+                    // This field is not specified for removal.
+                    'text' => 'Custom item text',
+                ],
+                (object) [
+                    'id' => null,
+                    'text' => 'Custom item text2',
+                ],
+            ],
+            'customstructure' => [ // This array contains scalar values, not a list of objects/arrays.
+                'id' => null,
+                'text' => 'Custom structure text',
+                'number' => 1,
+                'bool' => true,
+            ],
+        ];
+
+        $expecteddata = (object) [
+            'questiontext' => 'Some question text',
+            'hints' => [
+                (object) [
+                    'text' => 'Lorem ipsum',
+                ],
+                (object) [
+                    'text' => 'Lorem ipsum',
+                ],
+            ],
+            'options' => [
+                [
+                    'option' => true,
+                ],
+                [
+                    'option' => false,
+                ],
+                [
+                    'option' => false,
+                ],
+            ],
+            'custom2' => 'Some custom text2',
+            'customarray' => [
+                (object) [
+                    'text' => 'Custom item text',
+                ],
+                (object) [
+                    'text' => 'Custom item text2',
+                ],
+            ],
+            'customstructure' => [
+                'text' => 'Custom structure text',
+                'number' => 1,
+            ],
+        ];
+
+        $excludedfields = [
+            '/custom1',
+            '/custom3',
+            '/customarray/id',
+            '/customstructure/id',
+            '/customstructure/bool',
+            // A field that is not in the data structure will be ignored.
+            '/custom4',
+        ];
+        $this->assertEquals($expecteddata, \restore_qtype_plugin::remove_excluded_question_data($data, $excludedfields));
+    }
+}