Skip to content

Commit

Permalink
MDL-68875 privacy: Keep moodle_content_writer->get_path() the same
Browse files Browse the repository at this point in the history
In MDL-62853 a new clean_param(PARAM_PATH) was added to the
moodle_content_writer->get_path() method. And this caused some
Windows tests to start failing.

The problem is that clean_param(PARAM_PATH) does normalise directory
separators to be always forward backslashes and that's normally ok
but the get_path() method has some DIRECTORY_SEPARATOR dependent code
that stopped working under windows.

After analysing various solutions, and trying to keep the behavior
EXACTLY like it was before MDL-62853, but with the cleaning included
we have applied 2 changes:

b) Move the clean_param() to later within the array_map() function,
that way the code there, that uses DIRECTORY_SEPARATOR will continue
working the same.

b) As far as there are more DIRECTORY_SEPARATOR dependent code later
in the function, also perform a str_replace() to convert back to the
OS directory separator.

Those 2 points together (a and b) make the behavior to be 100% the
original one, with separators being kept and the paths being cleaned.

This solution corresponds 100% with the proposed fixes named 3) and
4) in the issue.

Final note... all that DIRECTORY_SEPARATOR maybe needs a review because
it really shouldn't be used unless strictly needed. But that falls out
from this issue which goal was to keep things safely working like they
were before the regression (but with the cleaning applied).
  • Loading branch information
stronk7 committed Jun 2, 2020
1 parent 111b293 commit de24516
Showing 1 changed file with 9 additions and 3 deletions.
12 changes: 9 additions & 3 deletions privacy/classes/local/request/moodle_content_writer.php
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,6 @@ protected function get_path(array $subcontext, string $name) : string {
// This weird code is to look for a subcontext that contains a number and append an '_' to the front.
// This is because there seems to be some weird problem with array_merge_recursive used in finalise_content().
$subcontext = array_map(function($data) {
$data = clean_param($data, PARAM_PATH);
if (stripos($data, DIRECTORY_SEPARATOR) !== false) {
$newpath = explode(DIRECTORY_SEPARATOR, $data);
$newpath = array_map(function($value) {
Expand All @@ -295,11 +294,18 @@ protected function get_path(array $subcontext, string $name) : string {
}
return $value;
}, $newpath);
return implode(DIRECTORY_SEPARATOR, $newpath);
$data = implode(DIRECTORY_SEPARATOR, $newpath);
} else if (is_numeric($data)) {
$data = '_' . $data;
}
return $data;
// Because clean_param() normalises separators to forward-slashes
// and because there is code DIRECTORY_SEPARATOR dependent after
// this array_map(), we ensure we get the original separator.
// Note that maybe we could leave the clean_param() alone, but
// surely that means that the DIRECTORY_SEPARATOR dependent
// code is not needed at all. So better keep existing behavior
// until this is revisited.
return str_replace('/', DIRECTORY_SEPARATOR, clean_param($data, PARAM_PATH));
}, $subcontext);

// Combine the context path, and the subcontext data.
Expand Down

0 comments on commit de24516

Please sign in to comment.