refactor: add hidden inputs in question_ui_renderer, don't add one for selects

MHajoha · MHajoha · commit 29df421d01b4 · 2025-02-28T17:18:41.000+01:00
Selects already preserve their value through the added fallback option.
diff --git a/classes/attempt_ui/available_opts_info.php b/classes/attempt_ui/available_opts_info.php
@@ -0,0 +1,44 @@
+<?php
+// This file is part of the QuestionPy Moodle plugin - https://questionpy.org
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+namespace qtype_questionpy\attempt_ui;
+
+/**
+ * Used by {@see question_ui_renderer::extract_available_options()} to hold type, available options and warning opt-out.
+ *
+ * @package    qtype_questionpy
+ * @author     Maximilian Haye
+ * @copyright  2024 TU Berlin, innoCampus {@link https://www.questionpy.org}
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class available_opts_info {
+    /**
+     * Trivial constructor.
+     *
+     * @param string $type
+     * @param array $availableoptions
+     * @param bool $warnonunknownoption
+     */
+    public function __construct(
+        /** @var string $type `radio`, `checkbox` or `select` */
+        public string $type,
+        /** @var string[] $availableoptions */
+        public array $availableoptions,
+        /** @var bool $warnonunknownoption */
+        public bool $warnonunknownoption
+    ) {
+    }
+}
diff --git a/classes/attempt_ui/dom_utils.php b/classes/attempt_ui/dom_utils.php
@@ -115,7 +115,7 @@ public static function set_select_value(DOMElement $select, string $value): void
             } catch (DOMException $e) {
                 // Thrown by createElementNS "If invalid $namespace or $qualifiedName", which are both constants, so
                 // the coding_exception fits.
-                throw new \coding_exception($e->getMessage());
+                throw new coding_exception($e->getMessage());
             }
             if (!$fallbackoption) {
                 debugging("Could not add fallback option element for value '$value', which is no longer available.");
@@ -126,4 +126,28 @@ public static function set_select_value(DOMElement $select, string $value): void
             $select->appendChild($fallbackoption);
         }
     }
+
+    /**
+     * Appends an XHTML hidden input to the given element.
+     *
+     * @param DOMElement $parent
+     * @param string $name
+     * @param string $value
+     * @return DOMElement
+     * @throws coding_exception
+     */
+    public static function add_hidden_input(DOMElement $parent, string $name, string $value): DOMElement {
+        try {
+            $element = $parent->ownerDocument->createElementNS(constants::NAMESPACE_XHTML, 'input');
+        } catch (DOMException $e) {
+            // Thrown by createElementNS "If invalid $namespace or $qualifiedName", which are both constants, so
+            // the coding_exception fits.
+            throw new coding_exception($e->getMessage());
+        }
+        $element->setAttribute('type', 'hidden');
+        $element->setAttribute('name', $name);
+        $element->setAttribute('value', $value);
+        $parent->appendChild($element);
+        return $element;
+    }
 }
diff --git a/classes/attempt_ui/question_ui_renderer.php b/classes/attempt_ui/question_ui_renderer.php
@@ -134,7 +134,7 @@ public function render(): render_result {
             mt_srand($nextseed);
         }
 
-        $warnings = $this->check_for_unknown_options($availableoptions);
+        $warnings = $this->check_for_and_preserve_unknown_options($availableoptions);
         $this->result = new render_result($this->xml->saveHTML(), $warnings);
         return $this->result;
     }
@@ -595,11 +595,11 @@ function (array $match) use ($question) {
      * - While we should discourage it, it is possible for inputs to be inside `qpy:if-role` or `qpy:feedback`
      *   elements. {@see question_ui_metadata_extractor} doesn't resolve those.
      *
-     * @return array
-     * @see check_for_unknown_options
+     * @return array<string, available_opts_info>
+     * @see check_for_and_preserve_unknown_options
      */
     private function extract_available_options(): array {
-        $optionsbyname = [];
+        $infobyname = [];
 
         /** @var DOMElement $select */
         foreach ($this->xpath->query('//xhtml:select[not(@qpy:warn-on-unknown-option = "no")]') as $select) {
@@ -608,69 +608,72 @@ private function extract_available_options(): array {
                 continue;
             }
 
-            $values = [];
+            $optvalues = [];
             /** @var DOMElement $option */
             foreach ($this->xpath->query('./xhtml:option | ./xhtml:optgroup/xhtml:option', $select) as $option) {
-                $values[] = $option->hasAttribute('value') ? $option->getAttribute('value') : $option->textContent;
+                $optvalues[] = $option->hasAttribute('value') ? $option->getAttribute('value') : $option->textContent;
             }
 
-            $optionsbyname[$name] = array_unique($values);
+            $warn = $select->getAttributeNS(constants::NAMESPACE_QPY, 'warn-on-unknown-option') !== 'no';
+
+            $infobyname[$name] = new available_opts_info('select', array_unique($optvalues), $warn);
         }
 
-        $ignorednames = [];
         /** @var DOMElement $input */
         foreach ($this->xpath->query('//xhtml:input[(@type="checkbox" or @type="radio")]') as $input) {
             $name = $input->getAttribute('name');
             if (!$name) {
                 continue;
             }
-            if (in_array($name, $ignorednames)) {
-                continue;
-            }
-            if ($input->getAttributeNS(constants::NAMESPACE_QPY, 'warn-on-unknown-option') === 'no') {
-                $ignorednames[] = $name;
-                continue;
-            }
 
-            if (!array_key_exists($name, $optionsbyname)) {
-                $optionsbyname[$name] = [];
+            $info = $infobyname[$name] ??= new available_opts_info($input->getAttribute('type'), [], true);
+
+            if ($input->getAttributeNS(constants::NAMESPACE_QPY, 'warn-on-unknown-option') === 'no') {
+                $info->warnonunknownoption = false;
             }
 
             $value = $input->hasAttribute('value') ? $input->getAttribute('value') : 'on';
-            if (!in_array($value, $optionsbyname[$name])) {
-                $optionsbyname[$name][] = $value;
+            if (!in_array($value, $info->availableoptions)) {
+                $info->availableoptions[] = $value;
             }
         }
 
-        foreach ($ignorednames as $ignoredname) {
-            unset($optionsbyname[$ignoredname]);
-        }
-        foreach ($optionsbyname as &$values) {
-            sort($values);
+        foreach ($infobyname as $info) {
+            sort($info->availableoptions);
         }
 
-        return $optionsbyname;
+        return $infobyname;
     }
 
     /**
-     * Checks if the last response contains values which are invalid.
+     * Checks the last response for invalid values and adds hidden inputs to preserve those invalid values.
      *
-     * @param array $availableoptionsbyname
-     * @return array
+     * @param available_opts_info[] $availableoptsinfobyname
+     * @return invalid_option_warning[]
+     * @throws coding_exception
      * @see extract_available_options
      */
-    private function check_for_unknown_options(array $availableoptionsbyname): array {
+    private function check_for_and_preserve_unknown_options(array $availableoptsinfobyname): array {
         $response = utils::get_last_response($this->attempt);
 
         $warnings = [];
-        foreach ($availableoptionsbyname as $name => $availableoptions) {
+        foreach ($availableoptsinfobyname as $name => $info) {
+            if (!$info->warnonunknownoption) {
+                continue;
+            }
             if (!array_key_exists($name, $response)) {
                 continue;
             }
 
             $lastvalue = $response[$name];
-            if (!in_array($lastvalue, $availableoptions)) {
-                $warnings[] = new invalid_option_warning($name, $lastvalue, $availableoptions);
+            if (in_array($lastvalue, $info->availableoptions)) {
+                continue;
+            }
+
+            $warnings[] = new invalid_option_warning($name, $lastvalue, $info->availableoptions);
+            if ($info->type !== 'select') {
+                // Selects are handled in dom_utils::set_select_value.
+                dom_utils::add_hidden_input($this->xml->documentElement, $this->attempt->get_qt_field_name($name), $lastvalue);
             }
         }
         return $warnings;
diff --git a/renderer.php b/renderer.php
@@ -172,7 +172,6 @@ protected function formulation_controls_feedback_in_iframe(question_attempt $qa,
             $isstudent = $qa->get_step(0)->get_user_id() === $USER->id;
             $warningshtml .= $this->output->render_from_template('qtype_questionpy/render_warnings', [
                 'warnings' => $renderresult->warnings,
-                'get_qt_field_name' => fn($text, $render) => $qa->get_qt_field_name($render(trim($text))),
                 'should_use_list' => count($renderresult->warnings) > 1,
                 'should_show_hint_contact_trainers' => $isstudent,
                 'should_show_hint_editable' => $isstudent && !$options->readonly,
diff --git a/templates/render_warnings.mustache b/templates/render_warnings.mustache
@@ -39,18 +39,13 @@
     {{#should_use_list}}
         <ul>
             {{#warnings}}
-                <li>
-                    {{{localize}}}
-                    <input type="hidden" name="{{#get_qt_field_name}} {{name}} {{/get_qt_field_name}}"
-                           value="{{value}}">
-                </li>
+                <li>{{{localize}}}</li>
             {{/warnings}}
         </ul>
     {{/should_use_list}}
     {{^should_use_list}}
         {{#warnings}}
             <p>{{{localize}}}</p>
-            <input type="hidden" name="{{#get_qt_field_name}} {{name}} {{/get_qt_field_name}}" value="{{value}}">
         {{/warnings}}
     {{/should_use_list}}
 
