From 3c0ba94e879702b3c2f20d4cb4f9120a0bfdd9fc Mon Sep 17 00:00:00 2001 From: Frederic Massart Date: Wed, 8 May 2013 15:50:33 +0800 Subject: [PATCH 1/5] MDL-38885 form: Recursively clean parameters based on right type --- lib/formslib.php | 88 ++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 77 insertions(+), 11 deletions(-) diff --git a/lib/formslib.php b/lib/formslib.php index 201da0c097c..b4decbd0c00 100644 --- a/lib/formslib.php +++ b/lib/formslib.php @@ -1673,6 +1673,80 @@ class MoodleQuickForm extends HTML_QuickForm_DHTMLRulesTableless { $this->_types = $paramtypes + $this->_types; } + /** + * Return the type(s) to use to clean an element. + * + * In the case where the element has an array as a value, we will try to obtain a + * type defined for that specific key, and recursively until done. + * + * This method does not work reverse, you cannot pass a nested element and hoping to + * fallback on the clean type of a parent. This method intends to be used with the + * main element, which will generate child types if needed, not the other way around. + * + * Example scenario: + * + * You have defined a new repeated element containing a text field called 'foo'. + * By default there will always be 2 occurence of 'foo' in the form. Even though + * you've set the type on 'foo' to be PARAM_INT, for some obscure reason, you want + * the first value of 'foo', to be PARAM_FLOAT, which you set using setType: + * $mform->setType('foo[0]', PARAM_FLOAT). + * + * Now if you call this method passing 'foo', along with the submitted values of 'foo': + * array(0 => '1.23', 1 => '10'), you will get an array telling you that the key 0 is a + * FLOAT and 1 is an INT. If you had passed 'foo[1]', along with its value '10', you would + * get the default value returned. + * + * @param string $elementname name of the element. + * @param mixed $value value that should be cleaned. + * @return string|array constant value or array of constant values (PARAM_...) + */ + public function getCleanType($elementname, $value, $default = PARAM_RAW) { + $type = $default; + if (array_key_exists($elementname, $this->_types)) { + $type = $this->_types[$elementname]; + } + if (is_array($value)) { + $default = $type; + $type = array(); + foreach ($value as $subkey => $subvalue) { + $typekey = "$elementname" . "[$subkey]"; + if (array_key_exists($typekey, $this->_types)) { + $subtype = $this->_types[$typekey]; + } else { + $subtype = $default; + } + if (is_array($subvalue)) { + $type[$subkey] = $this->getCleanType($typekey, $subvalue, $subtype); + } else { + $type[$subkey] = $subtype; + } + } + } + return $type; + } + + /** + * Return the cleaned value using the passed type(s). + * + * @param mixed $value value that has to be cleaned. + * @param int|array $type constant value to use to clean (PARAM_...), typically returned by {@link self::getCleanType()}. + * @return mixed cleaned up value. + */ + public function getCleanedValue($value, $type) { + if (is_array($type) && is_array($value)) { + foreach ($type as $key => $param) { + $value[$key] = $this->getCleanedValue($value[$key], $param); + } + } else if (!is_array($type) && !is_array($value)) { + $value = clean_param($value, $type); + } else if (!is_array($type) && is_array($value)) { + $value = clean_param_array($value, $type, true); + } else { + throw new coding_exception('Unexpected type or value received in MoodleQuickForm::cleanValue()'); + } + return $value; + } + /** * Updates submitted values * @@ -1685,17 +1759,9 @@ class MoodleQuickForm extends HTML_QuickForm_DHTMLRulesTableless { if (empty($submission)) { $this->_submitValues = array(); } else { - foreach ($submission as $key=>$s) { - if (array_key_exists($key, $this->_types)) { - $type = $this->_types[$key]; - } else { - $type = PARAM_RAW; - } - if (is_array($s)) { - $submission[$key] = clean_param_array($s, $type, true); - } else { - $submission[$key] = clean_param($s, $type); - } + foreach ($submission as $key => $s) { + $type = $this->getCleanType($key, $s); + $submission[$key] = $this->getCleanedValue($s, $type); } $this->_submitValues = $submission; $this->_flagSubmitted = true; From 8a9c3f4951e05eea80775d8c82d00a64434376c7 Mon Sep 17 00:00:00 2001 From: Frederic Massart Date: Wed, 8 May 2013 15:51:54 +0800 Subject: [PATCH 2/5] MDL-38885 form: Advanced type checking on special elements --- lib/formslib.php | 55 +++++++++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 19 deletions(-) diff --git a/lib/formslib.php b/lib/formslib.php index b4decbd0c00..7df034d9b58 100644 --- a/lib/formslib.php +++ b/lib/formslib.php @@ -1270,25 +1270,42 @@ abstract class moodleform { $mform = $this->_form; foreach ($mform->_elements as $element) { - switch ($element->getType()) { - case 'hidden': - case 'text': - case 'url': - $key = $element->getName(); - // For repeated elements we need to look for - // the "main" type, not for the one present - // on each repetition. All the stuff in formslib - // (repeat_elements(), updateSubmission()... seems - // to work that way. - $pos = strpos($key, '['); - if ($pos !== false) { - $key = substr($key, 0, $pos); - } - if (!array_key_exists($key, $mform->_types)) { - debugging("Did you remember to call setType() for '$key'? ". - 'Defaulting to PARAM_RAW cleaning.', DEBUG_DEVELOPER); - } - break; + $group = false; + $elements = array($element); + + if ($element->getType() == 'group') { + $group = $element; + $elements = $element->getElements(); + } + + foreach ($elements as $index => $element) { + switch ($element->getType()) { + case 'hidden': + case 'text': + case 'url': + if ($group) { + $name = $group->getElementName($index); + } else { + $name = $element->getName(); + } + $key = $name; + $found = array_key_exists($key, $mform->_types); + // For repeated elements we need to look for + // the "main" type, not for the one present + // on each repetition. All the stuff in formslib + // (repeat_elements(), updateSubmission()... seems + // to work that way. + while (!$found && strrpos($key, '[') !== false) { + $pos = strrpos($key, '['); + $key = substr($key, 0, $pos); + $found = array_key_exists($key, $mform->_types); + } + if (!$found) { + debugging("Did you remember to call setType() for '$name'? ". + 'Defaulting to PARAM_RAW cleaning.', DEBUG_DEVELOPER); + } + break; + } } } } From 9654f68ae922c24c7270b769cf24917e2c1c1c14 Mon Sep 17 00:00:00 2001 From: Frederic Massart Date: Thu, 9 May 2013 10:49:06 +0800 Subject: [PATCH 3/5] MDL-38885 form: More unit testing for missing type detection --- lib/tests/formslib_test.php | 103 +++++++++++++++++++++++++++++++++++- 1 file changed, 102 insertions(+), 1 deletion(-) diff --git a/lib/tests/formslib_test.php b/lib/tests/formslib_test.php index 793eb18ab7f..0ad6a4b5fba 100644 --- a/lib/tests/formslib_test.php +++ b/lib/tests/formslib_test.php @@ -209,9 +209,20 @@ class formslib_testcase extends advanced_testcase { $mform->display(); } + public function test_settype_debugging_url() { + $this->resetAfterTest(true); + $this->setAdminUser(); + $mform = new formslib_settype_debugging_url(); + $this->assertDebuggingCalled("Did you remember to call setType() for 'urltest'? Defaulting to PARAM_RAW cleaning."); + + // Check form still there though + $this->expectOutputRegex('/]*name="urltest"[^>]*type="text/'); + $mform->display(); + } + public function test_settype_debugging_repeat() { $mform = new formslib_settype_debugging_repeat(); - $this->assertDebuggingCalled("Did you remember to call setType() for 'repeattest'? Defaulting to PARAM_RAW cleaning."); + $this->assertDebuggingCalled("Did you remember to call setType() for 'repeattest[0]'? Defaulting to PARAM_RAW cleaning."); // Check form still there though $this->expectOutputRegex('/]*name="repeattest[^>]*type="text/'); @@ -225,6 +236,38 @@ class formslib_testcase extends advanced_testcase { $this->expectOutputRegex('/]*name="repeattest[^>]*type="text/'); $mform->display(); } + + public function test_settype_debugging_group() { + $mform = new formslib_settype_debugging_group(); + $this->assertDebuggingCalled("Did you remember to call setType() for 'groupel1'? Defaulting to PARAM_RAW cleaning."); + $this->expectOutputRegex('/]*name="groupel1"[^>]*type="text/'); + $this->expectOutputRegex('/]*name="groupel2"[^>]*type="text/'); + $mform->display(); + } + + public function test_settype_debugging_namedgroup() { + $mform = new formslib_settype_debugging_namedgroup(); + $this->assertDebuggingCalled("Did you remember to call setType() for 'namedgroup[groupel1]'? Defaulting to PARAM_RAW cleaning."); + $this->expectOutputRegex('/]*name="namedgroup\[groupel1\]"[^>]*type="text/'); + $this->expectOutputRegex('/]*name="namedgroup\[groupel2\]"[^>]*type="text/'); + $mform->display(); + } + + public function test_settype_debugging_funky_name() { + $mform = new formslib_settype_debugging_funky_name(); + $this->assertDebuggingCalled("Did you remember to call setType() for 'blah[foo][bar][1]'? Defaulting to PARAM_RAW cleaning."); + $this->expectOutputRegex('/]*name="blah\[foo\]\[bar\]\[0\]"[^>]*type="text/'); + $this->expectOutputRegex('/]*name="blah\[foo\]\[bar\]\[1\]"[^>]*type="text/'); + $mform->display(); + } + + public function test_settype_debugging_type_inheritance() { + $mform = new formslib_settype_debugging_type_inheritance(); + $this->expectOutputRegex('/]*name="blah\[foo\]\[bar\]\[0\]"[^>]*type="text/'); + $this->expectOutputRegex('/]*name="blah\[bar\]\[foo\]\[1\]"[^>]*type="text/'); + $this->expectOutputRegex('/]*name="blah\[any\]\[other\]\[2\]"[^>]*type="text/'); + $mform->display(); + } } @@ -277,6 +320,15 @@ class formslib_settype_debugging_hidden extends moodleform { } } +// Used to test debugging is called when hidden added without setType. +class formslib_settype_debugging_url extends moodleform { + public function definition() { + $mform = $this->_form; + + $mform->addElement('url', 'urltest', 'urltest'); + } +} + // Used to test debugging is called when repeated text added without setType. class formslib_settype_debugging_repeat extends moodleform { public function definition() { @@ -302,3 +354,52 @@ class formslib_settype_debugging_repeat_ok extends moodleform { $this->repeat_elements($repeatels, 2, array('repeattest' => array('type' => PARAM_RAW)), 'numtexts', 'addtexts'); } } + +// Used to test if debugging is called when a group contains elements without type. +class formslib_settype_debugging_group extends moodleform { + public function definition() { + $mform = $this->_form; + $group = array( + $mform->createElement('text', 'groupel1', 'groupel1'), + $mform->createElement('text', 'groupel2', 'groupel2') + ); + $mform->addGroup($group); + $mform->setType('groupel2', PARAM_INT); + } +} + +// Used to test if debugging is called when a named group contains elements without type. +class formslib_settype_debugging_namedgroup extends moodleform { + public function definition() { + $mform = $this->_form; + $group = array( + $mform->createElement('text', 'groupel1', 'groupel1'), + $mform->createElement('text', 'groupel2', 'groupel2') + ); + $mform->addGroup($group, 'namedgroup'); + $mform->setType('namedgroup[groupel2]', PARAM_INT); + } +} + +// Used to test if debugging is called when has a funky name. +class formslib_settype_debugging_funky_name extends moodleform { + public function definition() { + $mform = $this->_form; + $mform->addElement('text', 'blah[foo][bar][0]', 'test', 'test'); + $mform->addElement('text', 'blah[foo][bar][1]', 'test', 'test'); + $mform->setType('blah[foo][bar][0]', PARAM_INT); + } +} + +// Used to test if debugging is not called with type inheritance. +class formslib_settype_debugging_type_inheritance extends moodleform { + public function definition() { + $mform = $this->_form; + $mform->addElement('text', 'blah[foo][bar][0]', 'test1', 'test'); + $mform->addElement('text', 'blah[bar][foo][1]', 'test2', 'test'); + $mform->addElement('text', 'blah[any][other][2]', 'test3', 'test'); + $mform->setType('blah[foo][bar]', PARAM_INT); + $mform->setType('blah[bar]', PARAM_FLOAT); + $mform->setType('blah', PARAM_TEXT); + } +} From a85f745d5088a301edbaca29e58a7e26169842d2 Mon Sep 17 00:00:00 2001 From: Frederic Massart Date: Thu, 9 May 2013 12:20:15 +0800 Subject: [PATCH 4/5] MDL-38885 form: Unit tests for data cleaning --- lib/tests/formslib_test.php | 193 +++++++++++++++++++++++++++++++++++- 1 file changed, 192 insertions(+), 1 deletion(-) diff --git a/lib/tests/formslib_test.php b/lib/tests/formslib_test.php index 0ad6a4b5fba..4093610572a 100644 --- a/lib/tests/formslib_test.php +++ b/lib/tests/formslib_test.php @@ -268,6 +268,129 @@ class formslib_testcase extends advanced_testcase { $this->expectOutputRegex('/]*name="blah\[any\]\[other\]\[2\]"[^>]*type="text/'); $mform->display(); } + + public function test_type_cleaning() { + $expectedtypes = array( + 'simpleel' => PARAM_INT, + 'groupel1' => PARAM_INT, + 'groupel2' => PARAM_FLOAT, + 'groupel3' => PARAM_INT, + 'namedgroup' => array( + 'sndgroupel1' => PARAM_INT, + 'sndgroupel2' => PARAM_FLOAT, + 'sndgroupel3' => PARAM_INT + ), + 'namedgroupinherit' => array( + 'thdgroupel1' => PARAM_INT, + 'thdgroupel2' => PARAM_INT + ), + 'repeatedel' => array( + 0 => PARAM_INT, + 1 => PARAM_INT + ), + 'repeatedelinherit' => array( + 0 => PARAM_INT, + 1 => PARAM_INT + ), + 'squaretest' => array( + 0 => PARAM_INT + ), + 'nested' => array( + 0 => array( + 'bob' => array( + 123 => PARAM_INT, + 'foo' => PARAM_FLOAT + ), + 'xyz' => PARAM_RAW + ), + 1 => PARAM_INT + ) + ); + $valuessubmitted = array( + 'simpleel' => '11.01', + 'groupel1' => '11.01', + 'groupel2' => '11.01', + 'groupel3' => '11.01', + 'namedgroup' => array( + 'sndgroupel1' => '11.01', + 'sndgroupel2' => '11.01', + 'sndgroupel3' => '11.01' + ), + 'namedgroupinherit' => array( + 'thdgroupel1' => '11.01', + 'thdgroupel2' => '11.01' + ), + 'repeatedel' => array( + 0 => '11.01', + 1 => '11.01' + ), + 'repeatedelinherit' => array( + 0 => '11.01', + 1 => '11.01' + ), + 'squaretest' => array( + 0 => '11.01' + ), + 'nested' => array( + 0 => array( + 'bob' => array( + 123 => '11.01', + 'foo' => '11.01' + ), + 'xyz' => '11.01' + ), + 1 => '11.01' + ) + ); + $expectedvalues = array( + 'simpleel' => 11, + 'groupel1' => 11, + 'groupel2' => 11.01, + 'groupel3' => 11, + 'namedgroup' => array( + 'sndgroupel1' => 11, + 'sndgroupel2' => 11.01, + 'sndgroupel3' => 11 + ), + 'namedgroupinherit' => array( + 'thdgroupel1' => 11, + 'thdgroupel2' => 11 + ), + 'repeatedel' => array( + 0 => 11, + 1 => 11 + ), + 'repeatable' => 2, + 'repeatedelinherit' => array( + 0 => 11, + 1 => 11 + ), + 'repeatableinherit' => 2, + 'squaretest' => array( + 0 => 11 + ), + 'nested' => array( + 0 => array( + 'bob' => array( + 123 => 11, + 'foo' => 11.01 + ), + 'xyz' => '11.01' + ), + 1 => 11 + ) + ); + + $mform = new formslib_clean_value(); + $mform->get_form()->updateSubmission($valuessubmitted, null); + foreach ($expectedtypes as $elementname => $type) { + $expected = $mform->get_form()->getCleanType($elementname, $valuessubmitted[$elementname]); + $this->assertEquals($expected, $type, "Failed validating clean type of '$elementname'"); + } + + $data = $mform->get_data(); + $this->assertEquals($data, (object) $expectedvalues); + } } @@ -391,7 +514,7 @@ class formslib_settype_debugging_funky_name extends moodleform { } } -// Used to test if debugging is not called with type inheritance. +// Used to test that debugging is not called with type inheritance. class formslib_settype_debugging_type_inheritance extends moodleform { public function definition() { $mform = $this->_form; @@ -403,3 +526,71 @@ class formslib_settype_debugging_type_inheritance extends moodleform { $mform->setType('blah', PARAM_TEXT); } } + +class formslib_clean_value extends moodleform { + public function get_form() { + return $this->_form; + } + public function definition() { + $mform = $this->_form; + + // Add a simple int. + $mform->addElement('text', 'simpleel', 'simpleel'); + $mform->setType('simpleel', PARAM_INT); + + // Add a non-named group. + $group = array( + $mform->createElement('text', 'groupel1', 'groupel1'), + $mform->createElement('text', 'groupel2', 'groupel2'), + $mform->createElement('text', 'groupel3', 'groupel3') + ); + $mform->setType('groupel1', PARAM_INT); + $mform->setType('groupel2', PARAM_FLOAT); + $mform->setType('groupel3', PARAM_INT); + $mform->addGroup($group); + + // Add a named group. + $group = array( + $mform->createElement('text', 'sndgroupel1', 'sndgroupel1'), + $mform->createElement('text', 'sndgroupel2', 'sndgroupel2'), + $mform->createElement('text', 'sndgroupel3', 'sndgroupel3') + ); + $mform->addGroup($group, 'namedgroup'); + $mform->setType('namedgroup[sndgroupel1]', PARAM_INT); + $mform->setType('namedgroup[sndgroupel2]', PARAM_FLOAT); + $mform->setType('namedgroup[sndgroupel3]', PARAM_INT); + + // Add a named group, with inheritance. + $group = array( + $mform->createElement('text', 'thdgroupel1', 'thdgroupel1'), + $mform->createElement('text', 'thdgroupel2', 'thdgroupel2') + ); + $mform->addGroup($group, 'namedgroupinherit'); + $mform->setType('namedgroupinherit', PARAM_INT); + + // Add a repetition. + $repeat = $mform->createElement('text', 'repeatedel', 'repeatedel'); + $this->repeat_elements(array($repeat), 2, array('repeatedel' => array('type' => PARAM_INT)), 'repeatable', 'add', 0); + + // Add a repetition, with inheritance. + $repeat = $mform->createElement('text', 'repeatedelinherit', 'repeatedelinherit'); + $this->repeat_elements(array($repeat), 2, array(), 'repeatableinherit', 'add', 0); + $mform->setType('repeatedelinherit', PARAM_INT); + + // Add an arbitrary named element. + $mform->addElement('text', 'squaretest[0]', 'squaretest[0]'); + $mform->setType('squaretest[0]', PARAM_INT); + + // Add an arbitrary nested array named element. + $mform->addElement('text', 'nested[0][bob][123]', 'nested[0][bob][123]'); + $mform->setType('nested[0][bob][123]', PARAM_INT); + + // Add inheritance test cases. + $mform->setType('nested', PARAM_INT); + $mform->setType('nested[0]', PARAM_RAW); + $mform->setType('nested[0][bob]', PARAM_FLOAT); + $mform->addElement('text', 'nested[1]', 'nested[1]'); + $mform->addElement('text', 'nested[0][xyz]', 'nested[0][xyz]'); + $mform->addElement('text', 'nested[0][bob][foo]', 'nested[0][bob][foo]'); + } +} From d5909fd1447bc6f05dbf37d7c9eb72b79004e24a Mon Sep 17 00:00:00 2001 From: Frederic Massart Date: Fri, 10 May 2013 08:34:59 +0800 Subject: [PATCH 5/5] MDL-38885 form: Fix comments and stricter unit test --- lib/formslib.php | 5 +++-- lib/tests/formslib_test.php | 12 ++++++------ 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/formslib.php b/lib/formslib.php index 7df034d9b58..127c199b71e 100644 --- a/lib/formslib.php +++ b/lib/formslib.php @@ -1711,10 +1711,11 @@ class MoodleQuickForm extends HTML_QuickForm_DHTMLRulesTableless { * Now if you call this method passing 'foo', along with the submitted values of 'foo': * array(0 => '1.23', 1 => '10'), you will get an array telling you that the key 0 is a * FLOAT and 1 is an INT. If you had passed 'foo[1]', along with its value '10', you would - * get the default value returned. + * get the default clean type returned (param $default). * * @param string $elementname name of the element. * @param mixed $value value that should be cleaned. + * @param int $default default constant value to be returned (PARAM_...) * @return string|array constant value or array of constant values (PARAM_...) */ public function getCleanType($elementname, $value, $default = PARAM_RAW) { @@ -1759,7 +1760,7 @@ class MoodleQuickForm extends HTML_QuickForm_DHTMLRulesTableless { } else if (!is_array($type) && is_array($value)) { $value = clean_param_array($value, $type, true); } else { - throw new coding_exception('Unexpected type or value received in MoodleQuickForm::cleanValue()'); + throw new coding_exception('Unexpected type or value received in MoodleQuickForm::getCleanedValue()'); } return $value; } diff --git a/lib/tests/formslib_test.php b/lib/tests/formslib_test.php index 4093610572a..9edae3642f7 100644 --- a/lib/tests/formslib_test.php +++ b/lib/tests/formslib_test.php @@ -356,16 +356,16 @@ class formslib_testcase extends advanced_testcase { 'thdgroupel1' => 11, 'thdgroupel2' => 11 ), + 'repeatable' => 2, 'repeatedel' => array( 0 => 11, 1 => 11 ), - 'repeatable' => 2, + 'repeatableinherit' => 2, 'repeatedelinherit' => array( 0 => 11, 1 => 11 ), - 'repeatableinherit' => 2, 'squaretest' => array( 0 => 11 ), @@ -383,13 +383,13 @@ class formslib_testcase extends advanced_testcase { $mform = new formslib_clean_value(); $mform->get_form()->updateSubmission($valuessubmitted, null); - foreach ($expectedtypes as $elementname => $type) { - $expected = $mform->get_form()->getCleanType($elementname, $valuessubmitted[$elementname]); - $this->assertEquals($expected, $type, "Failed validating clean type of '$elementname'"); + foreach ($expectedtypes as $elementname => $expected) { + $actual = $mform->get_form()->getCleanType($elementname, $valuessubmitted[$elementname]); + $this->assertSame($expected, $actual, "Failed validating clean type of '$elementname'"); } $data = $mform->get_data(); - $this->assertEquals($data, (object) $expectedvalues); + $this->assertSame($expectedvalues, (array) $data); } }