L-electron-Rare/moodle
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

MDL-74449 gradebook: Protect flatten_dependencies_array() a little bit

Browse Source 
It has been detected that the flatten_dependencies_array() was fragile
and leading to wrong results when some incorrect data was passed to it.

This includes:

- Missing elements.
- Null dependencies.
- Non array dependencies.

While the existing behaviour (testing-wise) has been preserved, now the
situations above are better controlled and the function ignores all
those incorrect cases that shouldn't happen ever.

That implies that a good number of notices/warnings/errors aren't
happening anymore. That was impacting both results (when the problems
were  only notices and warnings) and execution (when the problems
were errors).

Covered with tests.
This commit is contained in:
Eloy Lafuente (stronk7)
2022-06-10 18:18:00 +02:00
parent 22dffcd0c2
commit df048de49d
2 changed files with 74 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lib/grade/grade_grade.php
+9 -1
View File
@@ -718,6 +718,14 @@ class grade_grade extends grade_object {
protected static function flatten_dependencies_array(&$dependson, &$dependencydepth) {
// Flatten the nested dependencies - this will handle recursion bombs because it removes duplicates.
$somethingchanged = true;
// First of all, delete any incorrect (not array or individual null) dependency, they aren't welcome.
// TODO: Maybe we should report about this happening, it shouldn't if all dependencies are correct and consistent.
foreach ($dependson as $itemid => $depends) {
$depends = is_array($depends) ? $depends : []; // Only arrays are accepted.
$dependson[$itemid] = array_filter($depends, function($val) { // Only not-null values are accepted.
return !is_null($val);
});
}
while ($somethingchanged) {
$somethingchanged = false;
@@ -725,7 +733,7 @@ class grade_grade extends grade_object {
// Make a copy so we can tell if it changed.
$before = $dependson[$itemid];
foreach ($depends as $subitemid => $subdepends) {
$dependson[$itemid] = array_unique(array_merge($depends, $dependson[$subdepends]));
$dependson[$itemid] = array_unique(array_merge($depends, $dependson[$subdepends] ?? []));
sort($dependson[$itemid], SORT_NUMERIC);
}
if ($before != $dependson[$itemid]) {
lib/grade/tests/grade_grade_test.php
+65
View File
@@ -196,6 +196,11 @@ class core_grade_grade_testcase extends grade_base_testcase {
$this->assertTrue($grade->is_hidden());
}
/**
* Test grade_grade::flatten_dependencies_array()
*
* @covers \grade_grade::flatten_dependencies_array()
*/
public function test_flatten_dependencies() {
// First test a simple normal case.
$a = array(1 => array(2, 3), 2 => array(), 3 => array(4), 4 => array());
@@ -231,6 +236,66 @@ class core_grade_grade_testcase extends grade_base_testcase {
test_grade_grade_flatten_dependencies_array::test_flatten_dependencies_array($a, $b);
$this->assertSame($expecteda, $a);
// Missing first level dependency.
$a = array(1 => array(2, 3), 3 => array(4), 4 => array());
$b = array();
$expecteda = array(1 => array(2, 3, 4), 3 => array(4), 4 => array());
$expectedb = array(1 => 1);
test_grade_grade_flatten_dependencies_array::test_flatten_dependencies_array($a, $b);
$this->assertSame($expecteda, $a);
$this->assertSame($expectedb, $b);
// Missing 2nd level dependency.
$a = array(1 => array(2, 3), 2 => array(), 3 => array(4));
$b = array();
$expecteda = array(1 => array(2, 3, 4), 2 => array(), 3 => array(4));
$expectedb = array(1 => 1);
test_grade_grade_flatten_dependencies_array::test_flatten_dependencies_array($a, $b);
$this->assertSame($expecteda, $a);
$this->assertSame($expectedb, $b);
// Null first level dependency.
$a = array(1 => array(2, null), 2 => array(3), 3 => array(4), 4 => array());
$b = array();
$expecteda = array(1 => array(2, 3, 4), 2 => array(3, 4), 3 => array(4), 4 => array());
$expectedb = array(1 => 2, 2 => 1);
test_grade_grade_flatten_dependencies_array::test_flatten_dependencies_array($a, $b);
$this->assertSame($expecteda, $a);
$this->assertSame($expectedb, $b);
// Null 2nd level dependency.
$a = array(1 => array(2, 3), 2 => array(), 3 => array(4), 4 => array(null));
$b = array();
$expecteda = array(1 => array(2, 3, 4), 2 => array(), 3 => array(4), 4 => array());
$expectedb = array(1 => 1);
test_grade_grade_flatten_dependencies_array::test_flatten_dependencies_array($a, $b);
$this->assertSame($expecteda, $a);
$this->assertSame($expectedb, $b);
// Straight null dependency.
$a = array(1 => array(2, 3), 2 => array(), 3 => array(4), 4 => null);
$b = array();
$expecteda = array(1 => array(2, 3, 4), 2 => array(), 3 => array(4), 4 => array());
$expectedb = array(1 => 1);
test_grade_grade_flatten_dependencies_array::test_flatten_dependencies_array($a, $b);
$this->assertSame($expecteda, $a);
$this->assertSame($expectedb, $b);
// Also incorrect non-array dependency.
$a = array(1 => array(2, 3), 2 => array(), 3 => array(4), 4 => 23);
$b = array();
$expecteda = array(1 => array(2, 3, 4), 2 => array(), 3 => array(4), 4 => array());
$expectedb = array(1 => 1);
test_grade_grade_flatten_dependencies_array::test_flatten_dependencies_array($a, $b);
$this->assertSame($expecteda, $a);
$this->assertSame($expectedb, $b);
}
public function test_grade_grade_min_max() {