From cb0318b633c7eee9d647992a17a027710b5b0dda Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Mon, 19 Dec 2011 18:22:28 +0000 Subject: [PATCH] MDL-30168 formslib: untangle automatic id generation. Previously, we had overridden the _generateId method in almost all subclasses; and then we mostly, but not always; ignored the value that was generated there, and instead generated new (nicer) values in MoodleQuickForm_Renderer::renderElement. Of course, that is not really a logical place to (re)generate ids. I have fixed the code so that the _generateId method now uses the 'nice id' algorithm from renderElement. This should make the whole code flow more logical. This make all our overriding of _generateId unnecessary. We do need a special _generateId for radio buttons, because you often have different radio buttons with the same name but different values. This change should only change the ids on radio, checkbox and advcheckbox elements. Previously, those were essentially random, so I don't think anyone could have been relying on the particular values. This commit also has new unit tests, first to test the basic _generateId algorithm, and then to create and render an example form (including some tricky things like repeat_elements) and chech the acutal ids in the generated HTML. --- lib/form/advcheckbox.php | 18 ----- lib/form/checkbox.php | 17 ----- lib/form/radio.php | 17 ----- lib/form/select.php | 17 ----- lib/form/selectgroups.php | 18 ----- lib/form/selectwithlink.php | 17 ----- lib/form/text.php | 17 ----- lib/form/url.php | 17 ----- lib/formslib.php | 13 +--- lib/pear/HTML/QuickForm/checkbox.php | 1 - lib/pear/HTML/QuickForm/element.php | 16 +++-- lib/pear/HTML/QuickForm/radio.php | 18 ++++- lib/simpletest/testformslib.php | 103 +++++++++++++++++++++++++++ 13 files changed, 130 insertions(+), 159 deletions(-) diff --git a/lib/form/advcheckbox.php b/lib/form/advcheckbox.php index 349fb8c02fd..4fe4650d6e1 100644 --- a/lib/form/advcheckbox.php +++ b/lib/form/advcheckbox.php @@ -87,24 +87,6 @@ class MoodleQuickForm_advcheckbox extends HTML_QuickForm_advcheckbox{ function getHelpButton(){ return $this->_helpbutton; } - /** - * Automatically generates and assigns an 'id' attribute for the element. - * - * Currently used to ensure that labels work on radio buttons and - * advcheckboxes. Per idea of Alexander Radivanovich. - * Overriden in moodleforms to remove qf_ prefix. - * - * @access private - * @return void - */ - function _generateId() - { - static $idx = 1; - - if (!$this->getAttribute('id')) { - $this->updateAttributes(array('id' => 'id_'.substr(md5(microtime() . $idx++), 0, 6))); - } - } // end func _generateId function toHtml() { diff --git a/lib/form/checkbox.php b/lib/form/checkbox.php index 237c796a149..bbccbc08600 100644 --- a/lib/form/checkbox.php +++ b/lib/form/checkbox.php @@ -36,24 +36,7 @@ class MoodleQuickForm_checkbox extends HTML_QuickForm_checkbox{ function getHelpButton(){ return $this->_helpbutton; } - /** - * Automatically generates and assigns an 'id' attribute for the element. - * - * Currently used to ensure that labels work on radio buttons and - * checkboxes. Per idea of Alexander Radivanovich. - * Overriden in moodleforms to remove qf_ prefix. - * - * @access private - * @return void - */ - function _generateId() - { - static $idx = 1; - if (!$this->getAttribute('id')) { - $this->updateAttributes(array('id' => 'id_'.substr(md5(microtime() . $idx++), 0, 6))); - } - } // end func _generateId /** * Called by HTML_QuickForm whenever form event is made on this element * diff --git a/lib/form/radio.php b/lib/form/radio.php index 3dc03a4a7dd..0287b0ff486 100644 --- a/lib/form/radio.php +++ b/lib/form/radio.php @@ -36,24 +36,7 @@ class MoodleQuickForm_radio extends HTML_QuickForm_radio{ function getHelpButton(){ return $this->_helpbutton; } - /** - * Automatically generates and assigns an 'id' attribute for the element. - * - * Currently used to ensure that labels work on radio buttons and - * checkboxes. Per idea of Alexander Radivanovich. - * Overriden in moodleforms to remove qf_ prefix. - * - * @access private - * @return void - */ - function _generateId() - { - static $idx = 1; - if (!$this->getAttribute('id')) { - $this->updateAttributes(array('id' => 'id_'.substr(md5(microtime() . $idx++), 0, 6))); - } - } // end func _generateId /** * Slightly different container template when frozen. * diff --git a/lib/form/select.php b/lib/form/select.php index f6bcfbd10e6..e76cd5718bf 100644 --- a/lib/form/select.php +++ b/lib/form/select.php @@ -31,24 +31,7 @@ class MoodleQuickForm_select extends HTML_QuickForm_select{ return parent::toHtml(); } } - /** - * Automatically generates and assigns an 'id' attribute for the element. - * - * Currently used to ensure that labels work on radio buttons and - * checkboxes. Per idea of Alexander Radivanovich. - * Overriden in moodleforms to remove qf_ prefix. - * - * @access private - * @return void - */ - function _generateId() - { - static $idx = 1; - if (!$this->getAttribute('id')) { - $this->updateAttributes(array('id' => 'id_'. substr(md5(microtime() . $idx++), 0, 6))); - } - } // end func _generateId /** * set html for help button * diff --git a/lib/form/selectgroups.php b/lib/form/selectgroups.php index 01ee9ed3336..92b5ea05450 100644 --- a/lib/form/selectgroups.php +++ b/lib/form/selectgroups.php @@ -548,24 +548,6 @@ class MoodleQuickForm_selectgroups extends HTML_QuickForm_element { function setHiddenLabel($hiddenLabel){ $this->_hiddenLabel = $hiddenLabel; } - /** - * Automatically generates and assigns an 'id' attribute for the element. - * - * Currently used to ensure that labels work on radio buttons and - * checkboxes. Per idea of Alexander Radivanovich. - * Overriden in moodleforms to remove qf_ prefix. - * - * @access private - * @return void - */ - function _generateId() - { - static $idx = 1; - - if (!$this->getAttribute('id')) { - $this->updateAttributes(array('id' => 'id_'. substr(md5(microtime() . $idx++), 0, 6))); - } - } // end func _generateId /** * set html for help button * diff --git a/lib/form/selectwithlink.php b/lib/form/selectwithlink.php index ccdd55a2599..637ce6a28cb 100644 --- a/lib/form/selectwithlink.php +++ b/lib/form/selectwithlink.php @@ -64,24 +64,7 @@ class MoodleQuickForm_selectwithlink extends HTML_QuickForm_select{ return $retval; } - /** - * Automatically generates and assigns an 'id' attribute for the element. - * - * Currently used to ensure that labels work on radio buttons and - * checkboxes. Per idea of Alexander Radivanovich. - * Overriden in moodleforms to remove qf_ prefix. - * - * @access private - * @return void - */ - function _generateId() - { - static $idx = 1; - if (!$this->getAttribute('id')) { - $this->updateAttributes(array('id' => 'id_'. substr(md5(microtime() . $idx++), 0, 6))); - } - } // end func _generateId /** * set html for help button * diff --git a/lib/form/text.php b/lib/form/text.php index 2e119b0abef..7d36b944e4b 100644 --- a/lib/form/text.php +++ b/lib/form/text.php @@ -32,24 +32,7 @@ class MoodleQuickForm_text extends HTML_QuickForm_text{ return parent::toHtml(); } } - /** - * Automatically generates and assigns an 'id' attribute for the element. - * - * Currently used to ensure that labels work on radio buttons and - * checkboxes. Per idea of Alexander Radivanovich. - * Overriden in moodleforms to remove qf_ prefix. - * - * @access private - * @return void - */ - function _generateId() - { - static $idx = 1; - if (!$this->getAttribute('id')) { - $this->updateAttributes(array('id' => 'id_'. substr(md5(microtime() . $idx++), 0, 6))); - } - } // end func _generateId /** * set html for help button * diff --git a/lib/form/url.php b/lib/form/url.php index 8822aa8ec46..dd38f92cfe7 100644 --- a/lib/form/url.php +++ b/lib/form/url.php @@ -80,24 +80,7 @@ EOD; return $str; } - /** - * Automatically generates and assigns an 'id' attribute for the element. - * - * Currently used to ensure that labels work on radio buttons and - * checkboxes. Per idea of Alexander Radivanovich. - * Overriden in moodleforms to remove qf_ prefix. - * - * @access private - * @return void - */ - function _generateId() - { - static $idx = 1; - if (!$this->getAttribute('id')) { - $this->updateAttributes(array('id' => 'id_'. substr(md5(microtime() . $idx++), 0, 6))); - } - } // end func _generateId /** * set html for help button * diff --git a/lib/formslib.php b/lib/formslib.php index ca83c7c6001..c69b6147750 100644 --- a/lib/formslib.php +++ b/lib/formslib.php @@ -2305,17 +2305,8 @@ class MoodleQuickForm_Renderer extends HTML_QuickForm_Renderer_Tableless{ * @param mixed $error */ function renderElement(&$element, $required, $error){ - //manipulate id of all elements before rendering - if (!is_null($element->getAttribute('id'))) { - $id = $element->getAttribute('id'); - } else { - $id = $element->getName(); - } - //strip qf_ prefix and replace '[' with '_' and strip ']' - $id = preg_replace(array('/^qf_|\]/', '/\[/'), array('', '_'), $id); - if (strpos($id, 'id_') !== 0){ - $element->updateAttributes(array('id'=>'id_'.$id)); - } + // Make sure the element has an id. + $element->_generateId(); //adding stuff to place holders in template //check if this is a group element first diff --git a/lib/pear/HTML/QuickForm/checkbox.php b/lib/pear/HTML/QuickForm/checkbox.php index 55c7574c57e..8b79c123fa3 100644 --- a/lib/pear/HTML/QuickForm/checkbox.php +++ b/lib/pear/HTML/QuickForm/checkbox.php @@ -64,7 +64,6 @@ class HTML_QuickForm_checkbox extends HTML_QuickForm_input $this->_text = $text; $this->setType('checkbox'); $this->updateAttributes(array('value'=>1)); - $this->_generateId(); } //end constructor // }}} diff --git a/lib/pear/HTML/QuickForm/element.php b/lib/pear/HTML/QuickForm/element.php index fccad5935cb..a0a600c45f5 100644 --- a/lib/pear/HTML/QuickForm/element.php +++ b/lib/pear/HTML/QuickForm/element.php @@ -415,14 +415,16 @@ class HTML_QuickForm_element extends HTML_Common * @access private * @return void */ - function _generateId() - { - static $idx = 1; - - if (!$this->getAttribute('id')) { - $this->updateAttributes(array('id' => 'qf_' . substr(md5(microtime() . $idx++), 0, 6))); + function _generateId() { + if ($this->getAttribute('id')) { + return; } - } // end func _generateId + + $id = $this->getName(); + $id = 'id_' . str_replace(array('qf_', '[', ']'), array('', '_', ''), $id); + $id = clean_param($id, PARAM_ALPHANUMEXT); + $this->updateAttributes(array('id' => $id)); + } // }}} // {{{ exportValue() diff --git a/lib/pear/HTML/QuickForm/radio.php b/lib/pear/HTML/QuickForm/radio.php index 963168b06ac..26320267668 100644 --- a/lib/pear/HTML/QuickForm/radio.php +++ b/lib/pear/HTML/QuickForm/radio.php @@ -66,10 +66,24 @@ class HTML_QuickForm_radio extends HTML_QuickForm_input $this->_persistantFreeze = true; $this->setType('radio'); $this->_text = $text; - $this->_generateId(); } //end constructor - + // }}} + + function _generateId() { + // Override the standard implementation, since you can have multiple + // check-boxes with the same name on a form. Therefore, add the + // (cleaned up) value to the id. + + if ($this->getAttribute('id')) { + return; + } + + parent::_generateId(); + $id = $this->getAttribute('id') . '_' . clean_param($this->getValue(), PARAM_ALPHANUMEXT); + $this->updateAttributes(array('id' => $id)); + } + // {{{ setChecked() /** diff --git a/lib/simpletest/testformslib.php b/lib/simpletest/testformslib.php index 03308956cf8..2d8d788e2cf 100644 --- a/lib/simpletest/testformslib.php +++ b/lib/simpletest/testformslib.php @@ -28,6 +28,9 @@ if (!defined('MOODLE_INTERNAL')) { die('Direct access to this script is forbidden.'); /// It must be included from a Moodle page } require_once($CFG->libdir . '/formslib.php'); +require_once($CFG->libdir . '/form/radio.php'); +require_once($CFG->libdir . '/form/select.php'); +require_once($CFG->libdir . '/form/text.php'); class formslib_test extends UnitTestCase { @@ -116,4 +119,104 @@ class formslib_test extends UnitTestCase { $CFG->strictformsrequired = $strictformsrequired; } + public function test_generate_id_select() { + $el = new MoodleQuickForm_select('choose_one', 'Choose one', + array(1 => 'One', '2' => 'Two')); + $el->_generateId(); + $this->assertEqual('id_choose_one', $el->getAttribute('id')); + } + + public function test_generate_id_like_repeat() { + $el = new MoodleQuickForm_text('text[7]', 'Type something'); + $el->_generateId(); + $this->assertEqual('id_text_7', $el->getAttribute('id')); + } + + public function test_can_manually_set_id() { + $el = new MoodleQuickForm_text('elementname', 'Type something', + array('id' => 'customelementid')); + $el->_generateId(); + $this->assertEqual('customelementid', $el->getAttribute('id')); + } + + public function test_generate_id_radio() { + $el = new MoodleQuickForm_radio('radio', 'Label', 'Choice label', 'choice_value'); + $el->_generateId(); + $this->assertEqual('id_radio_choice_value', $el->getAttribute('id')); + } + + public function test_radio_can_manually_set_id() { + $el = new MoodleQuickForm_radio('radio2', 'Label', 'Choice label', 'choice_value', + array('id' => 'customelementid2')); + $el->_generateId(); + $this->assertEqual('customelementid2', $el->getAttribute('id')); + } + + public function test_generate_id_radio_like_repeat() { + $el = new MoodleQuickForm_radio('repeatradio[2]', 'Label', 'Choice label', 'val'); + $el->_generateId(); + $this->assertEqual('id_repeatradio_2_val', $el->getAttribute('id')); + } + + public function test_rendering() { + $form = new formslib_test_form(); + ob_start(); + $form->display(); + $html = ob_get_clean(); + + $this->assert(new ContainsTagWithAttributes('select', array( + 'id' => 'id_choose_one', 'name' => 'choose_one')), $html); + + $this->assert(new ContainsTagWithAttributes('input', array( + 'type' => 'text', 'id' => 'id_text_0', 'name' => 'text[0]')), $html); + + $this->assert(new ContainsTagWithAttributes('input', array( + 'type' => 'text', 'id' => 'id_text_1', 'name' => 'text[1]')), $html); + + $this->assert(new ContainsTagWithAttributes('input', array( + 'type' => 'radio', 'id' => 'id_radio_choice_value', + 'name' => 'radio', 'value' => 'choice_value')), $html); + + $this->assert(new ContainsTagWithAttributes('input', array( + 'type' => 'radio', 'id' => 'customelementid2', 'name' => 'radio2')), $html); + + $this->assert(new ContainsTagWithAttributes('input', array( + 'type' => 'radio', 'id' => 'id_repeatradio_0_2', + 'name' => 'repeatradio[0]', 'value' => '2')), $html); + + $this->assert(new ContainsTagWithAttributes('input', array( + 'type' => 'radio', 'id' => 'id_repeatradio_2_1', + 'name' => 'repeatradio[2]', 'value' => '1')), $html); + + $this->assert(new ContainsTagWithAttributes('input', array( + 'type' => 'radio', 'id' => 'id_repeatradio_2_2', + 'name' => 'repeatradio[2]', 'value' => '2')), $html); + } +} + + +/** + * Test form to be used by {@link formslib_test::test_rendering()}. + */ +class formslib_test_form extends moodleform { + public function definition() { + $this->_form->addElement('select', 'choose_one', 'Choose one', + array(1 => 'One', '2' => 'Two')); + + $repeatels = array( + $this->_form->createElement('text', 'text', 'Type something') + ); + $this->repeat_elements($repeatels, 2, array(), 'numtexts', 'addtexts'); + + $this->_form->addElement('radio', 'radio', 'Label', 'Choice label', 'choice_value'); + + $this->_form->addElement('radio', 'radio2', 'Label', 'Choice label', 'choice_value', + array('id' => 'customelementid2')); + + $repeatels = array( + $this->_form->createElement('radio', 'repeatradio', 'Choose {no}', 'One', 1), + $this->_form->createElement('radio', 'repeatradio', 'Choose {no}', 'Two', 2), + ); + $this->repeat_elements($repeatels, 3, array(), 'numradios', 'addradios'); + } } \ No newline at end of file