Skip to content

Commit

Permalink
MDL-62003 privacy: Consistently export files into _files folder
Browse files Browse the repository at this point in the history 
There were two major issues with the previous implementation:

* The exported folder name was localised so it was "Files" or "Soubory"
  etc depending on the current language. Yet URLs referring to the files
  in that folder were always rewritten with hard-coded English "files".
* Files from all fileareas and itemids were all exported to a single
  target directory. So if there were two files with the same name being
  exported from multiple areas (such as submission_content and
  submission_attachment in the workshop module), one would overwrite
  another.

The patch addresses these issues as follows:

* To unify the folder name and also to minimise the risk of conflict
  with a subcontext folder, we now always export stored files under
  "_files" folder.
* Under that folder, there is a subdirectory with the area name and then
  eventually another subdirectory with non-zero itemid. And there
  finally the stored_file is exported to under its own file path.
  • Loading branch information
@mudrd8mz
mudrd8mz committed Apr 18, 2018
1 parent 109eb11 commit c6f9dce
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 16 deletions.
33 changes: 27 additions & 6 deletions privacy/classes/local/request/moodle_content_writer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,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@@/', 'files/', $text);
return str_replace('@@PLUGINFILE@@/', $this->get_files_target_path($component, $filearea, $itemid).'/', $text);
}

/**
Expand Down Expand Up @@ -188,11 +188,12 @@ public function export_area_files(array $subcontext, $component, $filearea, $ite
*/
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;
}
Expand Down Expand Up @@ -285,6 +286,26 @@ protected function get_full_path(array $subcontext, string $name) : string {
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.
*
Expand Down
6 changes: 5 additions & 1 deletion privacy/classes/tests/request/content_writer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,10 @@ public function get_custom_file(array $subcontext = [], $filename = null) {
/**
* 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.
Expand All @@ -393,7 +397,7 @@ public function get_custom_file(array $subcontext = [], $filename = null) {
* @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;
}

/**
Expand Down
79 changes: 70 additions & 9 deletions privacy/tests/moodle_content_writer_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ public function test_export_area_files() {
'component' => 'core_privacy',
'filearea' => 'tests',
'itemid' => 0,
'path' => '/',
'path' => '/sub/',
'name' => 'b.txt',
'content' => 'Test file 1',
];
Expand Down Expand Up @@ -389,7 +389,7 @@ public function test_export_area_files() {
'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);
}

Expand All @@ -401,14 +401,14 @@ public function test_export_area_files() {

$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));
}
}
Expand All @@ -421,16 +421,16 @@ public function test_export_area_files() {
* @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,
);
Expand All @@ -444,7 +444,7 @@ public function test_export_file($filepath, $filename, $content) {

$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());
}
Expand All @@ -457,36 +457,50 @@ public function test_export_file($filepath, $filename, $content) {
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',
'言語設定',
Expand Down Expand Up @@ -835,4 +849,51 @@ protected function get_context_path($context, $subcontext = null, $name = '') {
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,
'<p><img src="@@PLUGINFILE@@/hello.gif" /></p>',
'<p><img src="_files/intro/hello.gif" /></p>',
],
'nonzeroitemid' => [
'submission_content',
34,
'<p><img src="@@PLUGINFILE@@/first.png" alt="First" /></p>',
'<p><img src="_files/submission_content/34/first.png" alt="First" /></p>',
],
'withfilepath' => [
'post_content',
9889,
'<a href="@@PLUGINFILE@@/embedded/docs/muhehe.exe">Click here!</a>',
'<a href="_files/post_content/9889/embedded/docs/muhehe.exe">Click here!</a>',
],
];
}
}

0 comments on commit c6f9dce

Please sign in to comment.