Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

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.
  • Loading branch information...
commit cb0318b633c7eee9d647992a17a027710b5b0dda 1 parent 929c26c
@timhunt timhunt authored
View
18 lib/form/advcheckbox.php
@@ -87,24 +87,6 @@ function setHelpButton($helpbuttonargs, $function='helpbutton'){
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()
{
View
17 lib/form/checkbox.php
@@ -36,24 +36,7 @@ function setHelpButton($helpbuttonargs, $function='helpbutton'){
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
*
View
17 lib/form/radio.php
@@ -36,24 +36,7 @@ function setHelpButton($helpbuttonargs, $function='helpbutton'){
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.
*
View
17 lib/form/select.php
@@ -31,24 +31,7 @@ function toHtml(){
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
*
View
18 lib/form/selectgroups.php
@@ -548,24 +548,6 @@ function onQuickFormEvent($event, $arg, &$caller)
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
*
View
17 lib/form/selectwithlink.php
@@ -64,24 +64,7 @@ function toHtml(){
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
*
View
17 lib/form/text.php
@@ -32,24 +32,7 @@ function toHtml(){
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
*
View
17 lib/form/url.php
@@ -80,24 +80,7 @@ function toHtml(){
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
*
View
13 lib/formslib.php
@@ -2305,17 +2305,8 @@ function startGroup(&$group, $required, $error){
* @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
View
1  lib/pear/HTML/QuickForm/checkbox.php
@@ -64,7 +64,6 @@ function HTML_QuickForm_checkbox($elementName=null, $elementLabel=null, $text=''
$this->_text = $text;
$this->setType('checkbox');
$this->updateAttributes(array('value'=>1));
- $this->_generateId();
} //end constructor
// }}}
View
16 lib/pear/HTML/QuickForm/element.php
@@ -415,14 +415,16 @@ function accept(&$renderer, $required=false, $error=null)
* @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()
View
18 lib/pear/HTML/QuickForm/radio.php
@@ -66,10 +66,24 @@ function HTML_QuickForm_radio($elementName=null, $elementLabel=null, $text=null,
$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()
/**
View
103 lib/simpletest/testformslib.php
@@ -28,6 +28,9 @@
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 @@ public function test_require_rule() {
$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');
+ }
}
Please sign in to comment.
Something went wrong with that request. Please try again.