API: Form Field ID attribute should follow HTML specification

Fixes: http://open.silverstripe.org/ticket/4431.

Changes Form and Form Field classes to make use of Convert::raw2htmlid() which follows http://www.w3.org/TR/REC-html40/types.html#type-cdata.

Introduces a FormTemplateHelper class to assist in these sort of updates in the future.

docs/en/changelogs/rc/3.2.0.md

@@ -14,6 +14,7 @@ Otherwise, you'll need to include the module yourself
  * API: Removed URL routing by controller name
  * Security: The multiple authenticator login page should now be styled manually - i.e. without the default jQuery UI layout. A new template, Security_MultiAuthenticatorLogin.ss is available.
  * Security: This controller's templates can be customised by overriding the `getTemplate` function.
+ * API: Form and FormField ID attributes rewritten.
 
 ## Details
 
@@ -63,3 +64,78 @@ you can reinstate the old behaviour through a director rule:
 	Director:
   		rules:
     		'$Controller//$Action/$ID/$OtherID':  '*'
+
+### API: Default Form and FormField ID attributes rewritten.
+
+Previously the automatic generation of ID attributes throughout the Form API
+could generate invalid ID values such as Password[ConfirmedPassword] as well
+as duplicate ID values between forms on the same page. For example, if you
+created a field called `Email` on more than one form on the page, the resulting
+HTML would have multiple instances of `#Email`. ID should be a unique
+identifier for a single element within the document.
+
+This rewrite has several angles, each of which is described below. If you rely
+on ID values in your CSS files, Javascript code or application unit tests *you
+will need to update your code*.
+
+#### Conversion of invalid form ID values
+
+ID attributes on Form and Form Fields will now follow the
+[HTML specification](http://www.w3.org/TR/REC-html40/types.html#type-cdata).
+Generating ID attributes is now handled by the new `FormTemplateHelper` class.
+
+Please test each of your existing site forms to ensure that they work
+correctly in particular, javascript and css styles which rely on specific ID
+values.
+
+#### Invalid ID attributes stripped
+
+ID attributes will now be run through `Convert::raw2htmlid`. Invalid characters
+are replaced with a single underscore character. Duplicate, leading and trailing
+underscores are removed. Custom ID attributes (set through `setHTMLID`) will not
+be altered.
+
+	Before:
+	<form id="MyForm[Form]"
+		<div id="MyForm[Form][ID]">
+
+	Now:
+	<form id="MyForm_Form">
+		<div id="MyForm_Form_ID">
+
+#### Namespaced FormField ID's
+
+Form Field ID values will now be namespaced with the parent form ID.
+
+	Before:
+	<div id="Email">
+
+	Now:
+	<div id="MyForm_Email">
+
+#### FormField wrapper containers suffixed with `_Holder`
+
+Previously both the container div and FormField tag shared the same ID in
+certain cases. Now, the wrapper div in the default `FormField` template will be
+suffixed with `_Holder`.
+
+	Before:
+	<div id="Email">
+		<input id="Email" />
+
+	After:
+	<div id="MyForm_Email_Holder"
+		<input id="MyForm_Email" />
+
+#### Reverting to the old specification
+
+If upgrading existing forms is not feasible, developers can opt out of the new
+specifications by using the `FormTemplateHelper_Pre32` class rules instead of
+the default ones.
+
+	:::yaml
+	# mysite/config/_config.yml
+
+	Injector:
+		FormTemplateHelper:
+			class: FormTemplateHelper_Pre32

forms/Form.php

@@ -149,6 +149,21 @@ class Form extends RequestHandler {
 	 */
 	protected $attributes = array();
 
+	/**
+	 * @var FormTemplateHelper
+	 */
+	private $templateHelper = null;
+
+	/**
+	 * @ignore
+	 */
+	private $htmlID = null;
+
+	/**
+	 * @ignore
+	 */
+	private $formActionPath = false;
+
 	/**
 	 * Create a new form, with the given fields an action buttons.
 	 * 
@@ -642,13 +657,14 @@ class Form extends RequestHandler {
 
 	public function getAttributes() {
 		$attrs = array(
-			'id' => $this->FormName(),
+			'id' => $this->getTemplateHelper()->generateFormID($this),
 			'action' => $this->FormAction(),
 			'method' => $this->FormMethod(),
 			'enctype' => $this->getEncType(),
 			'target' => $this->target,
 			'class' => $this->extraClass(),
 		);
+
 		if($this->validator && $this->validator->getErrors()) {
 			if(!isset($attrs['class'])) $attrs['class'] = '';
 			$attrs['class'] .= ' validationerror';
@@ -671,6 +687,7 @@ class Form extends RequestHandler {
 
 		if(!$attrs || is_string($attrs)) $attrs = $this->getAttributes();
 
+
 		// Figure out if we can cache this form
 		// - forms with validation shouldn't be cached, cos their error messages won't be shown
 		// - forms with security tokens shouldn't be cached because security tokens expire
@@ -706,13 +723,43 @@ class Form extends RequestHandler {
 	}
 
 	/**
-	* Set the target of this form to any value - useful for opening the form contents in a new window or refreshing
+	 * Set the {@link FormTemplateHelper}
-	* another frame
+	 *
-	* 
+	 * @param string|FormTemplateHelper
-	* @param target The value of the target
+	 */
-	*/
+	public function setTemplateHelper($helper) {
+		$this->templateHelper = $helper;
+	}
+
+	/**
+	 * Return a {@link FormTemplateHelper} for this form. If one has not been
+	 * set, return the default helper.
+	 *
+	 * @return FormTemplateHelper
+	 */
+	public function getTemplateHelper() {
+		if($this->templateHelper) {
+			if(is_string($this->templateHelper)) {
+				return Injector::inst()->get($this->templateHelper);
+			}
+
+			return $this->templateHelper;
+		}
+
+		return Injector::inst()->get('FormTemplateHelper');
+	}
+
+	/**
+	 * Set the target of this form to any value - useful for opening the form
+	 * contents in a new window or refreshing another frame.
+	 *
+	 * @param target $target The value of the target
+	 *
+	 * @return FormField
+	 */
 	public function setTarget($target) {
 		$this->target = $target;
+
 		return $this;
 	}
 	
@@ -862,38 +909,50 @@ class Form extends RequestHandler {
 		}
 	}
 	
-	/** @ignore */
-	private $formActionPath = false;
-	
 	/**
 	 * Set the form action attribute to a custom URL.
 	 * 
-	 * Note: For "normal" forms, you shouldn't need to use this method.  It is recommended only for situations where
+	 * Note: For "normal" forms, you shouldn't need to use this method.  It is
-	 * you have two relatively distinct parts of the system trying to communicate via a form post.
+	 * recommended only for situations where you have two relatively distinct
+	 * parts of the system trying to communicate via a form post.
 	 */
 	public function setFormAction($path) {
 		$this->formActionPath = $path;
+
 		return $this;
 	}
 
 	/**
-	 * @ignore
+	 * Returns the name of the form.
-	 */
+	 *
-	private $htmlID = null;
+	 * @return string
-
-	/**
-	 * Returns the name of the form
 	 */
 	public function FormName() {
-		if($this->htmlID) return $this->htmlID;
+		if($this->htmlID) {
-		else return $this->class . '_' . str_replace(array('.', '/'), '', $this->name);
+			return $this->htmlID;
+		} else {
+			return $this->class . '_' . str_replace(array('.', '/'), '', $this->name);
+		}
 	}
 
 	/**
-	 * Set the HTML ID attribute of the form
+	 * Set the HTML ID attribute of the form.
+	 *
+	 * @param string $id
+	 *
+	 * @return FormField
 	 */
 	public function setHTMLID($id) {
 		$this->htmlID = $id;
+
+		return $this;
+	}
+
+	/**
+	 * @return string
+	 */
+	public function getHTMLID() {
+		return $this->htmlID;
 	}
 	
 	/**

forms/FormField.php

@@ -115,6 +115,7 @@ class FormField extends RequestHandler {
 		} else {
 			$label = $fieldName;
 		}
+
 		$label = preg_replace("/([a-z]+)([A-Z])/","$1 $2", $label);
 		
 		return $label;
@@ -177,17 +178,44 @@ class FormField extends RequestHandler {
 	
 	/**
 	 * Returns the HTML ID of the field - used in the template by label tags.
+	 *
 	 * The ID is generated as FormName_FieldName.  All Field functions should ensure
 	 * that this ID is included in the field.
+	 *
+	 * @return string
 	 */
 	public function ID() {
-		$name = preg_replace('/(^-)|(-$)/', '', preg_replace('/[^A-Za-z0-9_-]+/', '-', $this->name));
+		return $this->getTemplateHelper()->generateFieldID($this);
-		if($this->form) return $this->form->FormName() . '_' . $name;
-		else return $name;
 	}
 	
 	/**
-	 * Returns the field name - used by templates.
+	 * Returns the HTML ID for the form field holder element.
+	 *
+	 * @return string
+	 */
+	public function HolderID() {
+		return $this->getTemplateHelper()->generateFieldHolderID($this);
+	}
+
+	/**
+	 * Returns the current {@link FormTemplateHelper} on either the parent
+	 * Form or the global helper set through the {@link Injector} layout.
+	 *
+	 * To customize a single {@link FormField}, use {@link setTemplate} and
+	 * provide a custom template name.
+	 *
+	 * @return FormTemplateHelper
+	 */
+	public function getTemplateHelper() {
+		if($this->form) {
+			return $this->form->getTemplateHelper();
+		}
+
+		return Injector::inst()->get('FormTemplateHelper');
+	}
+
+	/**
+	 * Returns the raw field name.
 	 * 
 	 * @return string
 	 */
@@ -552,7 +580,6 @@ class FormField extends RequestHandler {
 	 * message has not been defined then just return blank. The default
 	 * error is defined on {@link Validator}.
 	 *
-	 * @todo Should the default error message be stored here instead
 	 * @return string
 	 */
 	public function getCustomValidationMessage() {

forms/FormTemplateHelper.php

@@ -0,0 +1,122 @@
+<?php
+
+/**
+ * A helper class for managing {@link Form} and {@link FormField} HTML template
+ * output.
+ *
+ * This primarily exists to maintain backwards compatibility between Form and
+ * FormField template changes since developers may rely on specific HTML output
+ * in their applications. Any core changes to templates (such as changing ID's)
+ * may have the potential to silently prevent websites from working.
+ *
+ * To provide a form with a custom FormTemplateHelper use the following snippet:
+ *
+ * <code>
+ * $form->setTemplateHelper('ClassName');
+ * </code>
+ *
+ * Globally, the FormTemplateHelper can be set via the {@link Injector} API.
+ *
+ * For backwards compatibility, with < 3.2 use the {@link FormTemplateHelper_Pre32}
+ * class which will preserve the old style form field attributes.
+ *
+ * <code>
+ *	Injector:
+ *	  FormTemplateHelper:
+ *	    class: FormTemplateHelper_Pre32
+ * </code>
+ *
+ * @package framework
+ * @subpackage forms
+ */
+class FormTemplateHelper {
+
+	/**
+	 * @param Form $form
+	 *
+	 * @return string
+	 */
+	public function generateFormID($form) {
+		if($id = $form->getHTMLID()) {
+			return Convert::raw2htmlid($id);
+		}
+
+		return Convert::raw2htmlid($form->FormName());
+	}
+
+	/**
+	 * @param FormField $field
+	 *
+	 * @return string
+	 */
+	public function generateFieldHolderID($field) {
+		return $this->generateFieldID($field) . '_Holder';
+	}
+
+	/**
+	 * Generate the field ID value
+	 *
+	 * @param FormField
+	 *
+	 * @return string
+	 */
+	public function generateFieldID($field) {
+		if($form = $field->getForm()) {
+			return sprintf("%s_%s",
+				$form->getHTMLID(),
+				Convert::raw2htmlid($field->getName())
+			);
+		}
+
+		return Convert::raw2htmlid($field->getName());
+	}
+
+}
+
+/**
+ * Note that this will cause duplicate and invalid ID attributes.
+ *
+ * @deprecated 4.0
+ *
+ * @package framework
+ * @subpackage forms
+ */
+class FormTemplateHelper_Pre32 extends FormTemplateHelper {
+
+	/**
+	 * @param Form
+	 *
+	 * @return string
+	 */
+	public function generateFormID($form) {
+		if($id = $form->getHTMLID()) {
+			return $id;
+		}
+
+		return $form->class . '_' . str_replace(array('.', '/'), '', $form->getName());
+	}
+
+	/**
+	 * @param FormField
+	 *
+	 * @return string
+	 */
+	public function generateFieldHolderID($field) {
+		return $field->getName();
+	}
+
+	/**
+	 * @param FormField
+	 *
+	 * @return string
+	 */
+	public function generateFieldID($field) {
+		$name = preg_replace('/(^-)|(-$)/', '', preg_replace('/[^A-Za-z0-9_-]+/', '-', $field->getName()));
+
+		if($form = $field->getForm()) {
+			return $form->FormName() . '_' . $name;
+		}
+
+		return $name;
+	}
+}

templates/forms/FormField_holder.ss

@@ -1,4 +1,4 @@
-<div id="$Name" class="field<% if extraClass %> $extraClass<% end_if %>">
+<div id="$HolderID" class="field<% if extraClass %> $extraClass<% end_if %>">
 	<% if Title %><label class="left" for="$ID">$Title</label><% end_if %>
 	<div class="middleColumn">
 		$Field

tests/forms/FieldListTest.php

@@ -127,23 +127,27 @@ class FieldListTest extends SapphireTest {
 		$this->assertEquals(0, $tab->Fields()->Count());
 	}
 
-	/**
-	 * Test removing a field from a set by it's name.
-	 */
 	public function testRemoveFieldByName() {
 		$fields = new FieldList();
-		
-		/* First of all, we add a field into our FieldList object */
 		$fields->push(new TextField('Name', 'Your name'));
 		
-		/* We have 1 field in our set now */
 		$this->assertEquals(1, $fields->Count());
-		
-		/* Then, we call up removeByName() to take it out again */
 		$fields->removeByName('Name');
-		
-		/* We have 0 fields in our set now, as we've just removed the one we added */
 		$this->assertEquals(0, $fields->Count());
+
+		$fields->push(new TextField('Name[Field]', 'Your name'));
+		$this->assertEquals(1, $fields->Count());
+		$fields->removeByName('Name[Field]');
+		$this->assertEquals(0, $fields->Count());
+	}
+
+	public function testDataFieldByName() {
+		$fields = new FieldList();
+		$fields->push($basic = new TextField('Name', 'Your name'));
+		$fields->push($brack = new TextField('Name[Field]', 'Your name'));
+
+		$this->assertEquals($basic, $fields->dataFieldByName('Name'));
+		$this->assertEquals($brack, $fields->dataFieldByName('Name[Field]'));
 	}
 	
 	/**
@@ -157,14 +161,19 @@ class FieldListTest extends SapphireTest {
 		/* A field gets added to the set */
 		$fields->addFieldToTab('Root', new TextField('Country'));
 
-		/* We have the same object as the one we pushed */
 		$this->assertSame($fields->dataFieldByName('Country'), $tab->fieldByName('Country'));
 		
-		/* The field called Country is replaced by the field called Email */
 		$fields->replaceField('Country', new EmailField('Email'));
-		
-		/* We have 1 field inside our tab */
 		$this->assertEquals(1, $tab->Fields()->Count());
+
+		$fields = new FieldList();
+		$fields->push(new TextField('Name', 'Your name'));
+		$brack = new TextField('Name[Field]', 'Your name');
+
+		$fields->replaceField('Name', $brack);
+		$this->assertEquals(1, $fields->Count());
+
+		$this->assertEquals('Name[Field]', $fields->first()->getName());
 	}
 	
 	public function testRenameField() {

tests/forms/FormTest.php

@@ -1,4 +1,5 @@
 <?php
+
 /**
  * @package framework
  * @subpackage tests
@@ -228,6 +229,8 @@ class FormTest extends FunctionalTest {
 	}
 	
 	public function testSessionValidationMessage() {
+		Config::inst()->update('Injector', 'FormTemplateHelper', array('class' => 'FormTemplateHelper_Pre32'));
+
 		$this->get('FormTest_Controller');
 		
 		$response = $this->post(
@@ -251,7 +254,6 @@ class FormTest extends FunctionalTest {
 			),
 			'Required fields show a notification on field when left blank'
 		);
-		
 	}
 	
 	public function testSessionSuccessMessage() {
@@ -433,6 +435,10 @@ class FormTest extends FunctionalTest {
 	
 }
 
+/**
+ * @package framework
+ * @subpackage tests
+ */
 class FormTest_Player extends DataObject implements TestOnly {
 	private static $db = array(
 		'Name' => 'Varchar',
@@ -454,6 +460,10 @@ class FormTest_Player extends DataObject implements TestOnly {
 	
 }
 
+/**
+ * @package framework
+ * @subpackage tests
+ */
 class FormTest_Team extends DataObject implements TestOnly {
 	private static $db = array(
 		'Name' => 'Varchar',
@@ -465,6 +475,10 @@ class FormTest_Team extends DataObject implements TestOnly {
 	);
 }
 
+/**
+ * @package framework
+ * @subpackage tests
+ */
 class FormTest_Controller extends Controller implements TestOnly {
 	private static $url_handlers = array(
 		'$Action//$ID/$OtherID' => "handleAction",
@@ -510,6 +524,10 @@ class FormTest_Controller extends Controller implements TestOnly {
 
 }
 
+/**
+ * @package framework
+ * @subpackage tests
+ */
 class FormTest_ControllerWithSecurityToken extends Controller implements TestOnly {
 	private static $url_handlers = array(
 		'$Action//$ID/$OtherID' => "handleAction",