Merge branch 'MDL-76582-master' of https://github.com/NashTechOpenUni…

…versity/moodle

question/engine/datalib.php

@@ -1674,13 +1674,13 @@ class question_file_saver implements question_response_files {
      *
      * @param int $draftitemid the draft area to save the files from.
      * @param string $component the component for the file area to save into.
-     * @param string $filearea the name of the file area to save into.
+     * @param string $uncleanedfilearea the name of the file area to save into - but before it has been cleaned up.
      * @param string $text optional content containing file links.
      */
-    public function __construct($draftitemid, $component, $filearea, $text = null) {
+    public function __construct($draftitemid, $component, $uncleanedfilearea, $text = null) {
         $this->draftitemid = $draftitemid;
         $this->component = $component;
-        $this->filearea = $filearea;
+        $this->filearea = self::clean_file_area_name($uncleanedfilearea);
         $this->value = $this->compute_value($draftitemid, $text);
     }
 
@@ -1744,6 +1744,29 @@ public function save_files($itemid, $context) {
                 $this->component, $this->filearea, $itemid);
     }
 
+    /**
+     * Clean up a possible file area name to ensure that it matches the required rules.
+     *
+     * @param string $uncleanedfilearea the proposed file area name (e.g. 'response_-attachments').
+     * @return string a similar valid file area name. E.g: response_attachments.
+     */
+    public static function clean_file_area_name(string $uncleanedfilearea): string {
+        $filearea = $uncleanedfilearea;
+        if ($filearea !== clean_param($filearea, PARAM_AREA)) {
+            // Only lowercase ascii letters, numbers and underscores are allowed.
+            // Remove the invalid character in the filearea string.
+            $filearea = preg_replace('~[^a-z0-9_]~', '', core_text::strtolower($filearea));
+            // Replace multiple underscore to a single underscore.
+            $filearea = preg_replace('~_+~', '_', $filearea);
+            // If, after attempted cleaning, the filearea is not valid, throw a clear error to avoid subtle bugs.
+            if ($filearea !== clean_param($filearea, PARAM_AREA)) {
+                throw new coding_exception('Name ' . $filearea .
+                    ' cannot be used with question_file_saver because it does not match the rules for file area names');
+            }
+        }
+        return $filearea;
+    }
+
     /**
      * Get the files that were submitted.
      * @return array of stored_files objects.

question/engine/questionattempt.php

@@ -622,7 +622,8 @@ public function prepare_response_files_draft_itemid($name, $contextid) {
 
         // No files yet.
         $draftid = 0; // Will be filled in by file_prepare_draft_area.
-        file_prepare_draft_area($draftid, $contextid, 'question', 'response_' . $name, null);
+        $filearea = question_file_saver::clean_file_area_name('response_' . $name);
+        file_prepare_draft_area($draftid, $contextid, 'question', $filearea, null);
         return $draftid;
     }
 

question/engine/questionattemptstep.php

@@ -248,6 +248,7 @@ public function set_qt_var($name, $value) {
      * type question_attempt::PARAM_FILES.
      *
      * @param string $name the name of the associated variable.
+     * @param int $contextid contextid of the question attempt
      * @return array of {@link stored_files}.
      */
     public function get_qt_files($name, $contextid) {
@@ -261,8 +262,9 @@ public function get_qt_files($name, $contextid) {
         }
 
         $fs = get_file_storage();
+        $filearea = question_file_saver::clean_file_area_name('response_' . $name);
         $this->files[$name] = $fs->get_area_files($contextid, 'question',
-                'response_' . $name, $this->id, 'sortorder', false);
+                $filearea, $this->id, 'sortorder', false);
 
         return $this->files[$name];
     }
@@ -287,13 +289,14 @@ public function prepare_response_files_draft_itemid($name, $contextid) {
      *
      * @param string $name the variable name the files belong to.
      * @param int $contextid the id of the context the quba belongs to.
-     * @param string $text the text to update the URLs in.
+     * @param string|null $text the text to update the URLs in.
      * @return array(int, string) the draft itemid and the text with URLs rewritten.
      */
     public function prepare_response_files_draft_itemid_with_text($name, $contextid, $text) {
+        $filearea = question_file_saver::clean_file_area_name('response_' . $name);
         $draftid = 0; // Will be filled in by file_prepare_draft_area.
         $newtext = file_prepare_draft_area($draftid, $contextid, 'question',
-                'response_' . $name, $this->id, null, $text);
+                $filearea, $this->id, null, $text);
         return array($draftid, $newtext);
     }
 
@@ -306,12 +309,13 @@ public function prepare_response_files_draft_itemid_with_text($name, $contextid,
      * @param string $text the text to update the URLs in.
      * @param int $contextid the id of the context the quba belongs to.
      * @param string $name the variable name the files belong to.
-     * @param array $extra extra file path components.
+     * @param array $extras extra file path components.
      * @return string the rewritten text.
      */
     public function rewrite_response_pluginfile_urls($text, $contextid, $name, $extras) {
+        $filearea = question_file_saver::clean_file_area_name('response_' . $name);
         return question_rewrite_question_urls($text, 'pluginfile.php', $contextid,
-                'question', 'response_' . $name, $extras, $this->id);
+                'question', $filearea, $extras, $this->id);
     }
 
     /**

question/engine/tests/datalib_test.php

@@ -250,4 +250,31 @@ public function test_save_and_load_an_empty_usage() {
         // Delete it.
         question_engine::delete_questions_usage_by_activity($quba->get_id());
     }
+
+    /**
+     * Test cases for {@see test_get_file_area_name()}.
+     *
+     * @return array test cases
+     */
+    public function get_file_area_name_cases(): array {
+        return [
+            'simple variable' => ['response_attachments', 'response_attachments'],
+            'behaviour variable' => ['response_5:answer', 'response_5answer'],
+            'variable with special character' => ['response_5:answer', 'response_5answer'],
+            'multiple underscores in different places' => ['response_weird____variable__name', 'response_weird_variable_name'],
+        ];
+    }
+
+    /**
+     * Test get_file_area_name.
+     *
+     * @covers \question_file_saver::clean_file_area_name
+     * @dataProvider get_file_area_name_cases
+     *
+     * @param string $uncleanedfilearea
+     * @param string $expectedfilearea
+     */
+    public function test_clean_file_area_name(string $uncleanedfilearea, string $expectedfilearea): void {
+        $this->assertEquals($expectedfilearea, \question_file_saver::clean_file_area_name($uncleanedfilearea));
+    }
 }