diff --git a/privacy/classes/local/request/moodle_content_writer.php b/privacy/classes/local/request/moodle_content_writer.php index 1cf0d453cc0..98b330b0f88 100644 --- a/privacy/classes/local/request/moodle_content_writer.php +++ b/privacy/classes/local/request/moodle_content_writer.php @@ -159,7 +159,7 @@ class moodle_content_writer implements content_writer { * @return string The processed string */ public function rewrite_pluginfile_urls(array $subcontext, $component, $filearea, $itemid, $text) : string { - return str_replace('@@PLUGINFILE@@/', 'files/', $text); + return str_replace('@@PLUGINFILE@@/', $this->get_files_target_path($component, $filearea, $itemid).'/', $text); } /** @@ -188,11 +188,12 @@ class moodle_content_writer implements content_writer { */ public function export_file(array $subcontext, \stored_file $file) : content_writer { if (!$file->is_directory()) { - $subcontextextra = [ - get_string('files'), - $file->get_filepath(), - ]; - $path = $this->get_path(array_merge($subcontext, $subcontextextra), $file->get_filename()); + $pathitems = array_merge( + $subcontext, + [$this->get_files_target_path($file->get_component(), $file->get_filearea(), $file->get_itemid())], + [$file->get_filepath()] + ); + $path = $this->get_path($pathitems, $file->get_filename()); check_dir_exists(dirname($path), true, true); $this->files[$path] = $file; } @@ -285,6 +286,26 @@ class moodle_content_writer implements content_writer { return preg_replace('@' . DIRECTORY_SEPARATOR . '+@', DIRECTORY_SEPARATOR, $filepath); } + /** + * Get a path within a subcontext where exported files should be written to. + * + * @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 path + */ + protected function get_files_target_path($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(DIRECTORY_SEPARATOR, $parts); + } + /** * Write the data to the specified path. * diff --git a/privacy/classes/tests/request/content_writer.php b/privacy/classes/tests/request/content_writer.php index 78c1c62624a..c7e2f02116a 100644 --- a/privacy/classes/tests/request/content_writer.php +++ b/privacy/classes/tests/request/content_writer.php @@ -385,6 +385,10 @@ class content_writer implements \core_privacy\local\request\content_writer { /** * Prepare a text area by processing pluginfile URLs within it. * + * Note that this method does not implement the pluginfile URL rewriting. Such a job tightly depends on how the + * actual writer exports files so it can be reliably tested only in real writers such as + * {@link core_privacy\local\request\moodle_content_writer}. + * * @param array $subcontext The location within the current context that this data belongs. * @param string $component The name of the component that the files belong to. * @param string $filearea The filearea within that component. @@ -393,7 +397,7 @@ class content_writer implements \core_privacy\local\request\content_writer { * @return string The processed string */ public function rewrite_pluginfile_urls(array $subcontext, $component, $filearea, $itemid, $text) : string { - return str_replace('@@PLUGINFILE@@/', 'files/', $text); + return $text; } /** diff --git a/privacy/tests/moodle_content_writer_test.php b/privacy/tests/moodle_content_writer_test.php index e0dcb627cae..f919bfdb2a3 100644 --- a/privacy/tests/moodle_content_writer_test.php +++ b/privacy/tests/moodle_content_writer_test.php @@ -340,7 +340,7 @@ class moodle_content_writer_test extends advanced_testcase { 'component' => 'core_privacy', 'filearea' => 'tests', 'itemid' => 0, - 'path' => '/', + 'path' => '/sub/', 'name' => 'b.txt', 'content' => 'Test file 1', ]; @@ -389,7 +389,7 @@ class moodle_content_writer_test extends advanced_testcase { 'filename' => $file->name, ]; - $file->namepath = $file->path . $file->name; + $file->namepath = '/' . $file->filearea . '/' . ($file->itemid ?: '') . $file->path . $file->name; $file->storedfile = $fs->create_file_from_string($record, $file->content); } @@ -401,14 +401,14 @@ class moodle_content_writer_test extends advanced_testcase { $firstfiles = array_slice($files, 0, 2); foreach ($firstfiles as $file) { - $contextpath = $this->get_context_path($context, [get_string('files')], $file->namepath); + $contextpath = $this->get_context_path($context, ['_files'], $file->namepath); $this->assertTrue($fileroot->hasChild($contextpath)); $this->assertEquals($file->content, $fileroot->getChild($contextpath)->getContent()); } $otherfiles = array_slice($files, 2); foreach ($otherfiles as $file) { - $contextpath = $this->get_context_path($context, [get_string('files')], $file->namepath); + $contextpath = $this->get_context_path($context, ['_files'], $file->namepath); $this->assertFalse($fileroot->hasChild($contextpath)); } } @@ -421,16 +421,16 @@ class moodle_content_writer_test extends advanced_testcase { * @param string $filename File name * @param string $content Content */ - public function test_export_file($filepath, $filename, $content) { + public function test_export_file($filearea, $itemid, $filepath, $filename, $content) { $this->resetAfterTest(); $context = \context_system::instance(); - $filenamepath = $filepath . $filename; + $filenamepath = '/' . $filearea . '/' . ($itemid ?: '') . $filepath . $filename; $filerecord = array( 'contextid' => $context->id, 'component' => 'core_privacy', - 'filearea' => 'tests', - 'itemid' => 0, + 'filearea' => $filearea, + 'itemid' => $itemid, 'filepath' => $filepath, 'filename' => $filename, ); @@ -444,7 +444,7 @@ class moodle_content_writer_test extends advanced_testcase { $fileroot = $this->fetch_exported_content($writer); - $contextpath = $this->get_context_path($context, [get_string('files')], $filenamepath); + $contextpath = $this->get_context_path($context, ['_files'], $filenamepath); $this->assertTrue($fileroot->hasChild($contextpath)); $this->assertEquals($content, $fileroot->getChild($contextpath)->getContent()); } @@ -457,36 +457,50 @@ class moodle_content_writer_test extends advanced_testcase { public function export_file_provider() { return [ 'basic' => [ + 'intro', + 0, '/', 'testfile.txt', 'An example file content', ], 'longpath' => [ + 'attachments', + '12', '/path/within/a/path/within/a/path/', 'testfile.txt', 'An example file content', ], 'pathwithspaces' => [ + 'intro', + 0, '/path with/some spaces/', 'testfile.txt', 'An example file content', ], 'filewithspaces' => [ + 'submission_attachments', + 1, '/path with/some spaces/', 'test file.txt', 'An example file content', ], 'image' => [ + 'intro', + 0, '/', 'logo.png', file_get_contents(__DIR__ . '/fixtures/logo.png'), ], 'UTF8' => [ + 'submission_content', + 2, '/Žluťoučký/', 'koníček.txt', 'koníček', ], 'EUC-JP' => [ + 'intro', + 0, '/言語設定/', '言語設定.txt', '言語設定', @@ -835,4 +849,51 @@ class moodle_content_writer_test extends advanced_testcase { return $rcm->invoke($writer, $subcontext, $name); } } + + /** + * Test correct rewriting of @@PLUGINFILE@@ in the exported contents. + * + * @dataProvider rewrite_pluginfile_urls_provider + * @param string $filearea The filearea within that component. + * @param int $itemid Which item those files belong to. + * @param string $input Raw text as stored in the database. + * @param string $expectedoutput Expected output of URL rewriting. + */ + public function test_rewrite_pluginfile_urls($filearea, $itemid, $input, $expectedoutput) { + + $writer = $this->get_writer_instance(); + $writer->set_context(\context_system::instance()); + + $realoutput = $writer->rewrite_pluginfile_urls([], 'core_test', $filearea, $itemid, $input); + + $this->assertEquals($expectedoutput, $realoutput); + } + + /** + * Provides testable sample data for {@link self::test_rewrite_pluginfile_urls()}. + * + * @return array + */ + public function rewrite_pluginfile_urls_provider() { + return [ + 'zeroitemid' => [ + 'intro', + 0, + '