From f181a24a0bc3c991a1f31699885d69e8564bd084 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Thu, 10 May 2018 18:16:09 +1000 Subject: [PATCH 1/6] MDL-62251 backup: Fix replace_tempdir() bug under Windows rename() fails under Windows if the destination file/directory exists. I modified the code to only call $this->get_workdir_path() once as that function creates the directory if doesn't exist. And we don't want that considering the behaviour of rename on Windows. --- backup/converter/convertlib.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/backup/converter/convertlib.php b/backup/converter/convertlib.php index 1fdcf1dcd7c44..9a3f87a11c445 100644 --- a/backup/converter/convertlib.php +++ b/backup/converter/convertlib.php @@ -239,15 +239,17 @@ protected function create_workdir() { protected function replace_tempdir() { global $CFG; + $tempdir = $this->get_tempdir_path(); + if (empty($CFG->keeptempdirectoriesonbackup)) { - fulldelete($this->get_tempdir_path()); + fulldelete($tempdir); } else { - if (!rename($this->get_tempdir_path(), $this->get_tempdir_path() . '_' . $this->get_name() . '_' . $this->id . '_source')) { + if (!rename($tempdir, $tempdir . '_' . $this->get_name() . '_' . $this->id . '_source')) { throw new convert_exception('failed_rename_source_tempdir'); } } - if (!rename($this->get_workdir_path(), $this->get_tempdir_path())) { + if (!rename($this->get_workdir_path(), $tempdir)) { throw new convert_exception('failed_move_converted_into_place'); } } From da3c76158e330a27fb722adbabd424264e37300c Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Thu, 10 May 2018 18:39:49 +1000 Subject: [PATCH 2/6] MDL-62251 task: Fix file_temp_cleanup_task::execute() bug in Windows $CFG->tempdir might contain both / and \ in Windows. Therefore, we need to call realpatch() to be able to compare that with another patch. --- lib/classes/task/file_temp_cleanup_task.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/classes/task/file_temp_cleanup_task.php b/lib/classes/task/file_temp_cleanup_task.php index 7166872e660f0..ac3359cda16bc 100644 --- a/lib/classes/task/file_temp_cleanup_task.php +++ b/lib/classes/task/file_temp_cleanup_task.php @@ -110,7 +110,7 @@ public function execute() { $this->execute_on($CFG->tempdir); // Run on $CFG->backuptempdir too, if different from the default one, '$CFG->tempdir/backup'. - if (realpath(dirname($CFG->backuptempdir)) !== $CFG->tempdir) { + if (realpath(dirname($CFG->backuptempdir)) !== realpath($CFG->tempdir)) { // The $CFG->backuptempdir setting is different from the default '$CFG->tempdir/backup'. $this->execute_on($CFG->backuptempdir); } From 4dfafed5d91112d888069123d0728b407f0e46d9 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Thu, 10 May 2018 20:13:09 +1000 Subject: [PATCH 3/6] MDL-62251 Task: Make test_cron_delete_from_temp Unix independent --- lib/tests/cronlib_test.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/tests/cronlib_test.php b/lib/tests/cronlib_test.php index 8db487a3af776..4c4deef057328 100644 --- a/lib/tests/cronlib_test.php +++ b/lib/tests/cronlib_test.php @@ -171,8 +171,8 @@ public function test_cron_delete_from_temp($nodes, $expected) { $isvalid = true; $isvalid = $isvalid && !$iter->isDot(); // Remove the default $CFG->tempdir/backup directory and $CFG->tempdir/.htaccess file from this comparison. - $isvalid = $isvalid && !($iter->isDir() && ($iter->getRealpath() === "{$tmpdir}/backup")); - $isvalid = $isvalid && !($iter->isFile() && ($iter->getRealpath() === "{$tmpdir}/.htaccess")); + $isvalid = $isvalid && !($iter->isDir() && ($iter->getRealpath() === $tmpdir . DIRECTORY_SEPARATOR . 'backup')); + $isvalid = $isvalid && !($iter->isFile() && ($iter->getRealpath() === $tmpdir . DIRECTORY_SEPARATOR . '.htaccess')); if ($isvalid) { $actual[] = $iter->getRealPath(); } From 44efefcbeb95a15f1f6bb4d3e1b8cd64bbaa6a56 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Thu, 10 May 2018 19:03:36 +1000 Subject: [PATCH 4/6] MDL-62251 Privacy: Fix get_path() and get_full_path() bug in Windows --- .../classes/local/request/moodle_content_writer.php | 8 ++++++-- privacy/tests/moodle_content_writer_test.php | 10 ++++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/privacy/classes/local/request/moodle_content_writer.php b/privacy/classes/local/request/moodle_content_writer.php index c212e271df8dd..2c642512badf3 100644 --- a/privacy/classes/local/request/moodle_content_writer.php +++ b/privacy/classes/local/request/moodle_content_writer.php @@ -272,7 +272,9 @@ protected function get_path(array $subcontext, string $name) : string { // Join the directory together with the name. $filepath = implode(DIRECTORY_SEPARATOR, $path) . DIRECTORY_SEPARATOR . $name; - return preg_replace('@' . DIRECTORY_SEPARATOR . '+@', DIRECTORY_SEPARATOR, $filepath); + // To use backslash, it must be doubled ("\\\\" PHP string). + $separator = str_replace('\\', '\\\\', DIRECTORY_SEPARATOR); + return preg_replace('@(' . $separator . '|/)+@', $separator, $filepath); } /** @@ -291,7 +293,9 @@ protected function get_full_path(array $subcontext, string $name) : string { // Join the directory together with the name. $filepath = implode(DIRECTORY_SEPARATOR, $path); - return preg_replace('@' . DIRECTORY_SEPARATOR . '+@', DIRECTORY_SEPARATOR, $filepath); + // To use backslash, it must be doubled ("\\\\" PHP string). + $separator = str_replace('\\', '\\\\', DIRECTORY_SEPARATOR); + return preg_replace('@(' . $separator . '|/)+@', $separator, $filepath); } /** diff --git a/privacy/tests/moodle_content_writer_test.php b/privacy/tests/moodle_content_writer_test.php index 7e20f64879baa..66e6759b8f183 100644 --- a/privacy/tests/moodle_content_writer_test.php +++ b/privacy/tests/moodle_content_writer_test.php @@ -1162,12 +1162,18 @@ protected function get_context_path($context, $subcontext = null, $name = '') { if (null === $subcontext) { $rcm = $rc->getMethod('get_context_path'); $rcm->setAccessible(true); - return $rcm->invoke($writer); + $path = $rcm->invoke($writer); } else { $rcm = $rc->getMethod('get_path'); $rcm->setAccessible(true); - return $rcm->invoke($writer, $subcontext, $name); + $path = $rcm->invoke($writer, $subcontext, $name); } + + // PHPUnit uses mikey179/vfsStream which is a stream wrapper for a virtual file system that uses '/' + // as the directory separator. + $path = str_replace(DIRECTORY_SEPARATOR, '/', $path); + + return $path; } /** From 07890336ab3df795f5ea40577e853dc563bcc0a6 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Thu, 10 May 2018 19:06:49 +1000 Subject: [PATCH 5/6] MDL-62251 Privacy: Url path separator should be platform independant It should always be forward slash. --- .../local/request/moodle_content_writer.php | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/privacy/classes/local/request/moodle_content_writer.php b/privacy/classes/local/request/moodle_content_writer.php index 2c642512badf3..34dacba09457c 100644 --- a/privacy/classes/local/request/moodle_content_writer.php +++ b/privacy/classes/local/request/moodle_content_writer.php @@ -162,7 +162,7 @@ public function export_custom_file(array $subcontext, $filename, $filecontent) : * @return string The processed string */ public function rewrite_pluginfile_urls(array $subcontext, $component, $filearea, $itemid, $text) : string { - return str_replace('@@PLUGINFILE@@/', $this->get_files_target_path($component, $filearea, $itemid).'/', $text); + return str_replace('@@PLUGINFILE@@/', $this->get_files_target_url($component, $filearea, $itemid).'/', $text); } /** @@ -318,6 +318,25 @@ protected function get_files_target_path($component, $filearea, $itemid) : strin return implode(DIRECTORY_SEPARATOR, $parts); } + /** + * Get a relative url to the directory of the exported files within a subcontext. + * + * @param string $component The name of the component that the files belong to. + * @param string $filearea The filearea within that component. + * @param string $itemid Which item those files belong to. + * @return string The url + */ + protected function get_files_target_url($component, $filearea, $itemid) : string { + // We do not need to include the component because we organise things by context. + $parts = ['_files', $filearea]; + + if (!empty($itemid)) { + $parts[] = $itemid; + } + + return implode('/', $parts); + } + /** * Write the data to the specified path. * From 861425713589f5375fc48e11b5a24d8d2ce22952 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Thu, 10 May 2018 22:57:04 +1000 Subject: [PATCH 6/6] MDL-62251 Privacy: Fix dir separator in export_file() --- privacy/classes/tests/request/content_writer.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/privacy/classes/tests/request/content_writer.php b/privacy/classes/tests/request/content_writer.php index 3bd7e9da749ee..f91025a4e6753 100644 --- a/privacy/classes/tests/request/content_writer.php +++ b/privacy/classes/tests/request/content_writer.php @@ -396,7 +396,10 @@ public function export_area_files(array $subcontext, $component, $filearea, $ite */ public function export_file(array $subcontext, \stored_file $file) : \core_privacy\local\request\content_writer { if (!$file->is_directory()) { - $filepath = explode(DIRECTORY_SEPARATOR, $file->get_filepath()); + $filepath = $file->get_filepath(); + // Directory separator in the stored_file class should always be '/'. The following line is just a fail safe. + $filepath = str_replace(DIRECTORY_SEPARATOR, '/', $filepath); + $filepath = explode('/', $filepath); $filepath[] = $file->get_filename(); $filepath = array_filter($filepath); $filepath = implode('/', $filepath);