Skip to content

Commit 9b6ecb1

Browse files
marxjohnsonmarxjohnson
committed
MDL-85975 backup: Exclude null values in 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.
1 parent 4376763 commit 9b6ecb1

File tree

2 files changed

+196
-14
lines changed

2 files changed

+196
-14
lines changed

‎backup/moodle2/restore_qtype_plugin.class.php‎

Lines changed: 48 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -568,8 +568,7 @@ public static function remove_excluded_question_data(stdClass $questiondata, arr
568568

569569
foreach ($excludefields as $excludefield) {
570570
$pathparts = explode('/', ltrim($excludefield, '/'));
571-
$data = $questiondata;
572-
self::unset_excluded_fields($data, $pathparts);
571+
$questiondata = self::unset_excluded_fields($questiondata, $pathparts);
573572
}
574573

575574
return $questiondata;
@@ -581,25 +580,60 @@ public static function remove_excluded_question_data(stdClass $questiondata, arr
581580
* If any of the elements in the path is an array, this is called recursively on each element in the array to unset fields
582581
* in each child of the array.
583582
*
584-
* @param stdClass|array $data The questiondata object, or a subsection of it.
583+
* @param array|stdClass $data The questiondata structure, or a subsection of it.
585584
* @param array $pathparts The remaining elements in the path to the excluded field.
586-
* @return void
585+
* @return array|stdClass The $data structure with excluded fields removed.
587586
*/
588-
private static function unset_excluded_fields(stdClass|array $data, array $pathparts): void {
587+
private static function unset_excluded_fields(array|stdClass $data, array $pathparts): array|stdClass {
589588
$element = array_shift($pathparts);
590-
if (!isset($data->{$element})) {
591-
// This element is not present in the data structure, nothing to unset.
592-
return;
589+
$unset = false;
590+
// Get the current element from the data structure.
591+
if (is_object($data)) {
592+
if (!property_exists($data, $element)) {
593+
// This element is not present in the data structure, nothing to unset.
594+
return $data;
595+
}
596+
$dataelement = $data->{$element};
597+
} else { // It's an array.
598+
if (!array_key_exists($element, $data)) {
599+
return $data;
600+
}
601+
$dataelement = $data[$element];
593602
}
594-
if (is_object($data->{$element})) {
595-
self::unset_excluded_fields($data->{$element}, $pathparts);
596-
} else if (is_array($data->{$element})) {
597-
foreach ($data->{$element} as $item) {
598-
self::unset_excluded_fields($item, $pathparts);
603+
// Check if we need to recur, or unset this element.
604+
if (is_object($dataelement)) {
605+
$dataelement = self::unset_excluded_fields($dataelement, $pathparts);
606+
} else if (is_array($dataelement)) {
607+
foreach ($dataelement as $key => $item) {
608+
if (is_object($item) || is_array($item)) {
609+
// This is an array of objects or arrays, recur.
610+
$dataelement[$key] = self::unset_excluded_fields($item, $pathparts);
611+
} else {
612+
// This is an associative array of values, check if they should be removed.
613+
$subelement = reset($pathparts);
614+
if ($key == $subelement) {
615+
unset($dataelement[$key]);
616+
}
617+
}
599618
}
600619
} else if (empty($pathparts)) {
601620
// This is the last element of the path and it's a scalar value, unset it.
602-
unset($data->{$element});
621+
$unset = true;
622+
}
623+
// Write the modified element back to the data structure, or unset it.
624+
if (is_object($data)) {
625+
if ($unset) {
626+
unset($data->{$element});
627+
} else {
628+
$data->{$element} = $dataelement;
629+
}
630+
} else {
631+
if ($unset) {
632+
unset($data[$element]);
633+
} else {
634+
$data[$element] = $dataelement;
635+
}
603636
}
637+
return $data;
604638
}
605639
}

‎backup/moodle2/tests/restore_qtype_plugin_test.php‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
<?php
2+
// This file is part of Moodle - http://moodle.org/
3+
//
4+
// Moodle is free software: you can redistribute it and/or modify
5+
// it under the terms of the GNU General Public License as published by
6+
// the Free Software Foundation, either version 3 of the License, or
7+
// (at your option) any later version.
8+
//
9+
// Moodle is distributed in the hope that it will be useful,
10+
// but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12+
// GNU General Public License for more details.
13+
//
14+
// You should have received a copy of the GNU General Public License
15+
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.
16+
17+
namespace core_backup;
18+
19+
/**
20+
* Tests for question type restore methods
21+
*
22+
* @package core_backup
23+
* @copyright 2025 onwards Catalyst IT EU {@link https://catalyst-eu.net}
24+
* @author Mark Johnson <mark.johnson@catalyst-eu.net>
25+
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
26+
* @covers \restore_qtype_plugin
27+
*/
28+
final class restore_qtype_plugin_test extends \basic_testcase {
29+
/**
30+
* All default and specified fields should be removed from the provided data structure.
31+
*/
32+
public function test_remove_excluded_question_data(): void {
33+
global $CFG;
34+
require_once($CFG->dirroot . '/backup/moodle2/restore_plugin.class.php');
35+
require_once($CFG->dirroot . '/backup/moodle2/restore_qtype_plugin.class.php');
36+
$data = (object) [
37+
// Default excluded fields should be removed.
38+
'id' => 1,
39+
'createdby' => 2,
40+
'modifiedby' => 3,
41+
// This field is not specified for removal, it should remain.
42+
'questiontext' => 'Some question text',
43+
// Excluded paths that address an array should operate on all items in the array.
44+
'hints' => [
45+
(object) [
46+
'id' => 4,
47+
'questionid' => 1,
48+
// This field is not specified for removal.
49+
'text' => 'Lorem ipsum',
50+
],
51+
(object) [
52+
'id' => 5,
53+
'questionid' => 1,
54+
'text' => 'Lorem ipsum',
55+
],
56+
],
57+
'options' => [ // This is an array of arrays, rather than an array of objects. It should be handled the same.
58+
[
59+
'id' => 6,
60+
'questionid' => 1,
61+
// This field is not specified for removal.
62+
'option' => true,
63+
],
64+
[
65+
'id' => 7,
66+
'questionid' => 1,
67+
'option' => false,
68+
],
69+
[
70+
'id' => 8,
71+
'questionid' => 1,
72+
'option' => false,
73+
],
74+
],
75+
'custom1' => 'Some custom text',
76+
// This field is not specified for removal.
77+
'custom2' => 'Some custom text2',
78+
// Fields specified for removal should be removed even if they contain null values.
79+
'custom3' => null,
80+
'customarray' => [
81+
(object) [
82+
// Null values should also be removed.
83+
'id' => null,
84+
// This field is not specified for removal.
85+
'text' => 'Custom item text',
86+
],
87+
(object) [
88+
'id' => null,
89+
'text' => 'Custom item text2',
90+
],
91+
],
92+
'customstructure' => [ // This array contains scalar values, not a list of objects/arrays.
93+
'id' => null,
94+
'text' => 'Custom structure text',
95+
'number' => 1,
96+
'bool' => true,
97+
],
98+
];
99+
100+
$expecteddata = (object) [
101+
'questiontext' => 'Some question text',
102+
'hints' => [
103+
(object) [
104+
'text' => 'Lorem ipsum',
105+
],
106+
(object) [
107+
'text' => 'Lorem ipsum',
108+
],
109+
],
110+
'options' => [
111+
[
112+
'option' => true,
113+
],
114+
[
115+
'option' => false,
116+
],
117+
[
118+
'option' => false,
119+
],
120+
],
121+
'custom2' => 'Some custom text2',
122+
'customarray' => [
123+
(object) [
124+
'text' => 'Custom item text',
125+
],
126+
(object) [
127+
'text' => 'Custom item text2',
128+
],
129+
],
130+
'customstructure' => [
131+
'text' => 'Custom structure text',
132+
'number' => 1,
133+
],
134+
];
135+
136+
$excludedfields = [
137+
'/custom1',
138+
'/custom3',
139+
'/customarray/id',
140+
'/customstructure/id',
141+
'/customstructure/bool',
142+
// A field that is not in the data structure will be ignored.
143+
'/custom4',
144+
];
145+
$this->assertEquals($expecteddata, \restore_qtype_plugin::remove_excluded_question_data($data, $excludedfields));
146+
147+
}
148+
}

0 commit comments

Comments
 (0)