From 6c8e25fa518e842adc9ed038c516f9acf62f4e25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Mon, 3 Jul 2017 19:21:32 +0200 Subject: [PATCH 1/4] MDL-59443 forms: Give elements a chance to validate submitted values This patch introduces support for an optional method provided by the form element classes to validate the submitted values implicitly - without the need to have the rule explicitly added via the form definition. I am aware this should ideally be added to the HTML_QuickForm_element parent class. But I wanted to avoid modification of that third party library and keep the change in the moodleform layer only. --- lib/formslib.php | 16 +++++++++++++++- lib/upgrade.txt | 4 ++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/formslib.php b/lib/formslib.php index cce38da8563..8df8f62a676 100644 --- a/lib/formslib.php +++ b/lib/formslib.php @@ -572,6 +572,20 @@ abstract class moodleform { $file_val = false; } + // Give the elements a chance to perform an implicit validation. + $element_val = true; + foreach ($mform->_elements as $element) { + if (method_exists($element, 'validateSubmitValue')) { + $value = $mform->getSubmitValue($element->getName()); + $result = $element->validateSubmitValue($value); + if (!empty($result) && is_string($result)) { + $element_val = false; + $mform->setElementError($element->getName(), $result); + } + } + } + + // Let the form instance validate the submitted values. $data = $mform->exportValues(); $moodle_val = $this->validation($data, $files); if ((is_array($moodle_val) && count($moodle_val)!==0)) { @@ -586,7 +600,7 @@ abstract class moodleform { $moodle_val = true; } - $this->_validated = ($internal_val and $moodle_val and $file_val); + $this->_validated = ($internal_val and $element_val and $moodle_val and $file_val); } return $this->_validated; } diff --git a/lib/upgrade.txt b/lib/upgrade.txt index fce1d3ae0b7..0ce524e8d43 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -13,6 +13,10 @@ information provided here is intended especially for developers. * External function core_group_external::get_activity_allowed_groups now returns an additional field: canaccessallgroups. It indicates whether the user will be able to access all the activity groups. * file_get_draft_area_info does not sum the root folder anymore when calculating the foldercount. +* The moodleform element classes can now optionally provide a public function validateSubmitValue(). This method can be + used to perform implicit validation of submitted values - without the need to explicitly add the validation rules to + every form. The method should accept a single parameter with the submitted value. It should return a string with the + eventual validation error, or an empty value if the validation passes. === 3.3.1 === From 572d6fdef30625c0b6be73802b6aa88b764bf6e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Mon, 3 Jul 2017 19:37:41 +0200 Subject: [PATCH 2/4] MDL-59443 forms: Allow implicit validation of filetypes element values The validation obeys the element options 'allowall', 'allowunknown' and 'onlytypes' passed when creating the element. --- lang/en/form.php | 2 ++ lib/form/filetypes.php | 49 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/lang/en/form.php b/lang/en/form.php index 2ed6cbf733c..e13d8b29086 100644 --- a/lang/en/form.php +++ b/lang/en/form.php @@ -42,8 +42,10 @@ $string['err_numeric'] = 'You must enter a number here.'; $string['err_rangelength'] = 'You must enter between {$a->format[0]} and {$a->format[1]} characters here.'; $string['err_required'] = 'You must supply a value here.'; $string['filetypesany'] = 'All file types'; +$string['filetypesnotall'] = 'It is not allowed to select \'All file types\' here'; $string['filetypesnotwhitelisted'] = 'These file types are not allowed here: {$a}'; $string['filetypesothers'] = 'Other files'; +$string['filetypesunknown'] = 'Unknown file types: {$a}'; $string['general'] = 'General'; $string['hideadvanced'] = 'Hide advanced'; $string['hour'] = 'Hour'; diff --git a/lib/form/filetypes.php b/lib/form/filetypes.php index 72993234b18..9e4e3b06a53 100644 --- a/lib/form/filetypes.php +++ b/lib/form/filetypes.php @@ -45,6 +45,9 @@ class MoodleQuickForm_filetypes extends MoodleQuickForm_group { /** @var bool Allow selection of 'All file types' (will be stored as '*'). */ protected $allowall = true; + /** @var bool Skip implicit validation against known file types. */ + protected $allowunknown = false; + /** @var core_form\filetypes_util instance to use as a helper. */ protected $util = null; @@ -56,6 +59,7 @@ class MoodleQuickForm_filetypes extends MoodleQuickForm_group { * @param array $options element options: * 'onlytypes': Allow selection from these file types only; for example ['onlytypes' => ['web_image']]. * 'allowall': Allow to select 'All file types', defaults to true. Does not apply with onlytypes are set. + * 'allowunknown': Skip implicit validation against the list of known file types. * @param array|string $attributes Either a typical HTML attribute string or an associative array */ public function __construct($elementname = null, $elementlabel = null, $options = null, $attributes = null) { @@ -75,6 +79,9 @@ class MoodleQuickForm_filetypes extends MoodleQuickForm_group { if (!$this->onlytypes && array_key_exists('allowall', $options)) { $this->allowall = (bool)$options['allowall']; } + if (array_key_exists('allowunknown', $options)) { + $this->allowunknown = (bool)$options['allowunknown']; + } } $this->util = new filetypes_util(); @@ -196,8 +203,50 @@ class MoodleQuickForm_filetypes extends MoodleQuickForm_group { } $this->setValue($value); return true; + break; + } return parent::onQuickFormEvent($event, $arg, $caller); } + + /** + * Check that the submitted list contains only known and allowed file types. + * + * The validation obeys the element options 'allowall', 'allowunknown' and + * 'onlytypes' passed when creating the element. + * + * @param array $value Submitted value. + * @return string|null Validation error message or null. + */ + public function validateSubmitValue($value) { + + if (!$this->allowall) { + // Assert that there is an actual list provided. + $normalized = $this->util->normalize_file_types($value['filetypes']); + if (empty($normalized) || $normalized == ['*']) { + return get_string('filetypesnotall', 'core_form'); + } + } + + if (!$this->allowunknown) { + // Assert that all file types are known. + $unknown = $this->util->get_unknown_file_types($value['filetypes']); + + if ($unknown) { + return get_string('filetypesunknown', 'core_form', implode(', ', $unknown)); + } + } + + if ($this->onlytypes) { + // Assert that all file types are allowed here. + $notwhitelisted = $this->util->get_not_whitelisted($value['filetypes'], $this->onlytypes); + + if ($notwhitelisted) { + return get_string('filetypesnotwhitelisted', 'core_form', implode(', ', $notwhitelisted)); + } + } + + return; + } } From a1004698757e1df0db5830e1d750236d90ba36f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Mon, 3 Jul 2017 21:03:17 +0200 Subject: [PATCH 3/4] MDL-59443 forms: Consider '*' as a known file type While working on the filetypes element validation, I realized we did not cover the case of selecting 'Any file type'. So the value '*' was falsely reported as unknown file type. --- lib/form/classes/filetypes_util.php | 5 ++++- lib/form/tests/filetypes_util_test.php | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/form/classes/filetypes_util.php b/lib/form/classes/filetypes_util.php index 4eefc07f18e..4bebfd577b1 100644 --- a/lib/form/classes/filetypes_util.php +++ b/lib/form/classes/filetypes_util.php @@ -493,7 +493,10 @@ class filetypes_util { $unknown = []; foreach ($this->normalize_file_types($types) as $type) { - if ($this->is_filetype_group($type)) { + if ($type === '*') { + // Any file is considered as a known type. + continue; + } else if ($this->is_filetype_group($type)) { // The type is a group that exists. continue; } else if ($this->looks_like_mimetype($type)) { diff --git a/lib/form/tests/filetypes_util_test.php b/lib/form/tests/filetypes_util_test.php index d5419cd519a..68eba3a15a2 100644 --- a/lib/form/tests/filetypes_util_test.php +++ b/lib/form/tests/filetypes_util_test.php @@ -412,6 +412,14 @@ class filetypes_util_testcase extends advanced_testcase { */ public function get_unknown_file_types_provider() { return [ + 'Empty list' => [ + 'filetypes' => '', + 'expected' => [], + ], + 'Any file type' => [ + 'filetypes' => '*', + 'expected' => [], + ], 'Unknown extension' => [ 'filetypes' => '.rat', 'expected' => ['.rat'] From ee7e8fdf8e2247c020b8093a89d36b30437e4814 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Wed, 5 Jul 2017 21:00:33 +0200 Subject: [PATCH 4/4] MDL-59443 forms: Add behat tests for the field validation --- lib/form/tests/behat/filetypes.feature | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/form/tests/behat/filetypes.feature b/lib/form/tests/behat/filetypes.feature index b65ca66bff8..cf75dd2b9b3 100644 --- a/lib/form/tests/behat/filetypes.feature +++ b/lib/form/tests/behat/filetypes.feature @@ -24,4 +24,14 @@ Feature: There is a form element allowing to select filetypes Scenario: File types can be provided via direct input with JavaScript enabled Given I set the field "Choose from all file types" to ".png .gif .jpg" When I press "Save changes" - Then the field "Choose from all file types" matches value ".png .gif .jpg" \ No newline at end of file + Then the field "Choose from all file types" matches value ".png .gif .jpg" + + Scenario: File types are validated to be known, unless the field allows unknown be provided + Given I set the field "Choose from all file types" to ".pdf .doesnoexist" + And I set the field "Choose from a limited set" to "doc docx pdf rtf" + And I set the field "Unknown file types are allowed here" to ".neverminditdoesnotexist" + When I press "Save changes" + Then I should see "Unknown file types: .doesnoexist" + And I should see "These file types are not allowed here: .doc, .docx, .rtf" + And I should see "It is not allowed to select 'All file types' here" + And I should not see "Unknown file types: .neverminditdoesnotexist" \ No newline at end of file