API Use core validation for form submission

Fixes #350 and fixes #404
This change abandons validation via EditableFormField::validateField, as it bypassed too many core validation mechanisms (RequiredFields, etc).
In order to enforce consistency of editable field validation, display rules have been hard-disabled when a field is marked as required.
Since this removes functionality, I have incremented the major version number

code/extensions/UserFormValidator.php

@@ -22,7 +22,7 @@ class UserFormValidator extends RequiredFields {
 				// Page at top level, or after another page is ok
 				if(empty($stack) || (count($stack) === 1 && $stack[0] instanceof EditableFormStep)) {
 					$stack = array($field);
-					$conditionalStep = $field->DisplayRules()->count() > 0;
+					$conditionalStep = $field->EffectiveDisplayRules()->count() > 0;
 					continue;
 				}
 

code/forms/UserForm.php

@@ -27,6 +27,9 @@ class UserForm extends Form {
 		$actions->setForm($this);
 		$this->setValidator($this->getRequiredFields());
 
+		// This needs to be re-evaluated since fields have been assigned
+		$this->setupFormErrors();
+
 		// Number each page
 		$stepNumber = 1;
 		foreach($this->getSteps() as $step) {
@@ -46,6 +49,16 @@ class UserForm extends Form {
 		$this->extend('updateForm');
 	}
 
+	public function setupFormErrors()
+	{
+		// Suppress setupFormErrors if fields haven't been bootstrapped
+		if($this->fields && $this->fields->exists()) {
+			return parent::setupFormErrors();
+		}
+
+		return $this;
+	}
+
 	/**
 	 * Used for partial caching in the template.
 	 *
@@ -90,6 +103,7 @@ class UserForm extends Form {
 		}
 		$fields->clearEmptySteps();
 		$this->extend('updateFormFields', $fields);
+		$fields->setForm($this);
 		return $fields;
 	}
 
@@ -115,7 +129,7 @@ class UserForm extends Form {
 		}
 
 		$this->extend('updateFormActions', $actions);
-
+		$actions->setForm($this);
 		return $actions;
 	}
 
@@ -127,37 +141,16 @@ class UserForm extends Form {
 	public function getRequiredFields() {
 		// Generate required field validator
 		$requiredNames = $this
-			->controller
+			->getController()
 			->Fields()
 			->filter('Required', true)
 			->column('Name');
 		$required = new RequiredFields($requiredNames);
 		$this->extend('updateRequiredFields', $required);
+		$required->setForm($this);
 		return $required;
 	}
 
-	/**
-	 * Override validation so conditional fields can be validated correctly.
-	 *
-	 * @return boolean
-	 */
-	public function validate() {
-		$data = $this->getData();
-
-		Session::set("FormInfo.{$this->FormName()}.data", $data);
-		Session::clear("FormInfo.{$this->FormName()}.errors");
-
-		foreach ($this->controller->Fields() as $key => $field) {
-			$field->validateField($data, $this);
-		}
-
-		if(Session::get("FormInfo.{$this->FormName()}.errors")) {
-			return false;
-		}
-
-		return true;
-	}
-
 	/**
 	 * Override some we can add UserForm specific attributes to the form.
 	 *

code/model/UserDefinedForm.php

@@ -426,7 +426,7 @@ class UserDefinedForm_Controller extends Page_Controller {
 				}
 
 				// Check for field dependencies / default
-				foreach($field->DisplayRules() as $rule) {
+				foreach($field->EffectiveDisplayRules() as $rule) {
 
 					// Get the field which is effected
 					$formFieldWatch = EditableFormField::get()->byId($rule->ConditionFieldID);

code/model/editableformfields/EditableFormField.php

@@ -8,7 +8,12 @@ use SilverStripe\Forms\SegmentField;
  *
  * @package userforms
  *
- * @property string Name
+ * @property string $Name
+ * @property string $Title
+ * @property string $Default
+ * @property int $Sort
+ * @property bool $Required
+ * @property string $CustomErrorMessage
  * @method UserDefinedForm Parent() Parent page
  * @method DataList DisplayRules() List of EditableCustomRule objects
  */
@@ -203,14 +208,39 @@ class EditableFormField extends DataObject {
 
 		// Validation
 		$validationFields = $this->getFieldValidationOptions();
-		if($validationFields) {
+		if($validationFields && $validationFields->count()) {
-			$fields->addFieldsToTab(
+			$fields->addFieldsToTab('Root.Validation', $validationFields);
-				'Root.Validation',
+		}
-				$this->getFieldValidationOptions()
+
+		// Add display rule fields
+		$displayFields = $this->getDisplayRuleFields();
+		if($displayFields && $displayFields->count()) {
+			$fields->addFieldsToTab('Root.DisplayRules', $displayFields);
+		}
+
+		$this->extend('updateCMSFields', $fields);
+
+		return $fields;
+	}
+
+	/**
+	 * Return fields to display on the 'Display Rules' tab
+	 *
+	 * @return FieldList
+	 */
+	protected function getDisplayRuleFields() {
+		// Check display rules
+		if($this->Required) {
+			return new FieldList(
+				LabelField::create(_t(
+					'EditableFormField.DISPLAY_RULES_DISABLED',
+					'Display rules are not enabled for required fields. ' .
+					'Please uncheck "Is this field Required?" under "Validation" to re-enable.'
+				))->addExtraClass('message warning')
 			);
 		}
-		$allowedClasses = array_keys($this->getEditableFieldClasses(false));
 		$self = $this;
+		$allowedClasses = array_keys($this->getEditableFieldClasses(false));
 		$editableColumns = new GridFieldEditableColumns();
 		$editableColumns->setDisplayFields(array(
 			'Display' => '',
@@ -251,7 +281,7 @@ class EditableFormField extends DataObject {
 				new GridFieldDeleteAction()
 			);
 
-		$fields->addFieldsToTab('Root.DisplayRules', array(
+		return new FieldList(
 			CheckboxField::create('ShowOnLoad')
 				->setDescription(_t(
 					'EditableFormField.SHOWONLOAD',
@@ -263,16 +293,9 @@ class EditableFormField extends DataObject {
 				$this->DisplayRules(),
 				$customRulesConfig
 			)
-		));
+		);
-
-		$this->extend('updateCMSFields', $fields);
-
-		return $fields;
 	}
 
-	/**
-	 * @return void
-	 */
 	public function onBeforeWrite() {
 		parent::onBeforeWrite();
 
@@ -320,24 +343,24 @@ class EditableFormField extends DataObject {
 		return false;
 	}
 
-    /**
+	/**
-     * Return whether a user can delete this form field
+	 * Return whether a user can delete this form field
-     * based on whether they can edit the page
+	 * based on whether they can edit the page
-     *
+	 *
      * @param Member $member
-     * @return bool
+	 * @return bool
-     */
+	 */
 	public function canDelete($member = null) {
 		return $this->canEdit($member);
-	}
+		}
 
-    /**
+	/**
-     * Return whether a user can edit this form field
+	 * Return whether a user can edit this form field
-     * based on whether they can edit the page
+	 * based on whether they can edit the page
-     *
+	 *
      * @param Member $member
-     * @return bool
+	 * @return bool
-     */
+	 */
 	public function canEdit($member = null) {
         $parent = $this->Parent();
 		if($parent && $parent->exists()) {
@@ -346,7 +369,7 @@ class EditableFormField extends DataObject {
 
         // Fallback to secure admin permissions
 		return parent::canEdit($member);
-	}
+		}
 
     /**
      * Return whether a user can view this form field
@@ -678,7 +701,7 @@ class EditableFormField extends DataObject {
 	 */
 	protected function updateFormField($field) {
 		// set the error / formatting messages
-		$field->setCustomValidationMessage($this->getErrorMessage());
+		$field->setCustomValidationMessage($this->getErrorMessage()->RAW());
 
 		// set the right title on this field
 		if($this->RightTitle) {
@@ -741,34 +764,6 @@ class EditableFormField extends DataObject {
 		return DBField::create_field('Varchar', $errorMessage);
 	}
 
-	/**
-	 * Validate the field taking into account its custom rules.
-	 *
-	 * @param Array $data
-	 * @param UserForm $form
-	 *
-	 * @return boolean
-	 */
-	public function validateField($data, $form) {
-		if($this->Required && $this->DisplayRules()->Count() == 0) {
-			$formField = $this->getFormField();
-
-			if(isset($data[$this->Name])) {
-				$formField->setValue($data[$this->Name]);
-			}
-
-			if(
-				!isset($data[$this->Name]) ||
-				!$data[$this->Name] ||
-				!$formField->validate($form->getValidator())
-			) {
-				$form->addErrorMessage($this->Name, $this->getErrorMessage()->HTML(), 'error', false);
-			}
-		}
-
-		return true;
-	}
-
 	/**
 	 * Invoked by UserFormUpgradeService to migrate settings specific to this field from CustomSettings
 	 * to the field proper
@@ -874,4 +869,17 @@ class EditableFormField extends DataObject {
 		return EditableFormFieldValidator::create()
 			->setRecord($this);
 	}
+
+	/**
+	 * Determine effective display rules for this field.
+	 *
+	 * @return SS_List
+	 */
+	public function EffectiveDisplayRules() {
+		if($this->Required) {
+			return new ArrayList();
+		}
+		return $this->DisplayRules();
+	}
+
 }

composer.json

@@ -38,7 +38,7 @@
 	},
 	"extra": {
 		"branch-alias": {
-			"dev-master": "3.1.x-dev"
+			"dev-master": "4.0.x-dev"
 		}
 	}
 }

tests/EditableFormFieldTest.php

@@ -55,7 +55,8 @@ class EditableFormFieldTest extends FunctionalTest {
 
 		// form has 2 fields - a checkbox and a text field
 		// it has 1 rule - when ticked the checkbox hides the text field
-		$this->assertEquals($rules->Count(), 1);
+		$this->assertEquals(1, $rules->Count());
+		$this->assertEquals($rules, $checkbox->EffectiveDisplayRules());
 
 		$checkboxRule = $rules->First();
 		$checkboxRule->ConditionFieldID = $field->ID;
@@ -63,6 +64,10 @@ class EditableFormFieldTest extends FunctionalTest {
 		$this->assertEquals($checkboxRule->Display, 'Hide');
 		$this->assertEquals($checkboxRule->ConditionOption, 'HasValue');
 		$this->assertEquals($checkboxRule->FieldValue, '6');
+
+		// If field is required then all custom rules are disabled
+		$checkbox->Required = true;
+		$this->assertEquals(0, $checkbox->EffectiveDisplayRules()->count());
 	}
 
 	function testEditableDropdownField() {

tests/EditableLiteralFieldTest.php

@@ -7,16 +7,9 @@ class EditableLiteralFieldTest extends SapphireTest {
 
 	public function setUp() {
 		parent::setUp();
-		Config::nest();
 		HtmlEditorConfig::set_active('cms');
 	}
 
-
-	public function tearDown() {
-		Config::unnest();
-		parent::tearDown();
-	}
-
 	/**
 	 * Tests the sanitisation of HTML content
 	 */

tests/UserDefinedFormControllerTest.php

@@ -8,7 +8,7 @@ class UserDefinedFormControllerTest extends FunctionalTest {
 
 	static $fixture_file = 'UserDefinedFormTest.yml';
 
-	function testProcess() {
+	public function testProcess() {
 		$form = $this->setupFormFrontend();
 
 		$controller = new UserDefinedFormControllerTest_Controller($form);
@@ -60,7 +60,40 @@ class UserDefinedFormControllerTest extends FunctionalTest {
 		$this->assertStringEndsWith('finished#uff', $response->getHeader('Location'));
 	}
 
-	function testFinished() {
+	public function testValidation() {
+		$form = $this->setupFormFrontend('email-form');
+
+		// Post with no fields
+		$this->get($form->URLSegment);
+		$response = $this->submitForm('UserForm_Form', null, array());
+		$this->assertPartialMatchBySelector(
+			'.field .message',
+			array('This field is required')
+		);
+
+		// Post with all fields, but invalid email
+		$this->get($form->URLSegment);
+		$this->submitForm('UserForm_Form', null, array(
+			'required-email' => 'invalid',
+			'required-text' => 'bob'
+		));
+		$this->assertPartialMatchBySelector(
+			'.field .message',
+			array('Please enter an email address')
+		);
+
+		// Post with only required
+		$this->get($form->URLSegment);
+		$this->submitForm('UserForm_Form', null, array(
+			'required-text' => 'bob'
+		));
+		$this->assertPartialMatchBySelector(
+			'p',
+			array("Thanks, we've received your submission.")
+		);
+	}
+
+	public function testFinished() {
 		$form = $this->setupFormFrontend();
 
 		// set formProcessed and SecurityID to replicate the form being filled out
@@ -72,7 +105,7 @@ class UserDefinedFormControllerTest extends FunctionalTest {
 		$this->assertContains($form->OnCompleteMessage ,$response->getBody());
 	}
 
-	function testAppendingFinished() {
+	public function testAppendingFinished() {
 		$form = $this->setupFormFrontend();
 
 		// replicate finished being added to the end of the form URL without the form being filled out
@@ -84,7 +117,7 @@ class UserDefinedFormControllerTest extends FunctionalTest {
 		$this->assertNotContains($form->OnCompleteMessage ,$response->getBody());
 	}
 
-	function testForm() {
+	public function testForm() {
 		$form = $this->objFromFixture('UserDefinedForm', 'basic-form-page');
 
 		$controller = new UserDefinedFormControllerTest_Controller($form);
@@ -103,7 +136,7 @@ class UserDefinedFormControllerTest extends FunctionalTest {
 		$this->assertEquals(count($controller->Form()->getValidator()->getRequired()), 1);
 	}
 
-	function testGetFormFields() {
+	public function testGetFormFields() {
 		// generating the fieldset of fields
 		$form = $this->objFromFixture('UserDefinedForm', 'basic-form-page');
 
@@ -125,7 +158,7 @@ class UserDefinedFormControllerTest extends FunctionalTest {
 		$firstStep = $formSteps->first();
 		$firstField = $firstStep->getChildren()->first();
 
-		$this->assertEquals('Custom Error Message', $firstField->getCustomValidationMessage()->getValue());
+		$this->assertEquals('Custom Error Message', $firstField->getCustomValidationMessage());
 		$this->assertEquals($firstField->Title(), 'Required Text Field <span class=\'required-identifier\'>*</span>');
 
 		// test custom right title
@@ -146,7 +179,8 @@ class UserDefinedFormControllerTest extends FunctionalTest {
 		$this->assertFalse($controller->Form()->getFormFields()->exists());
 	}
 
-	function testGetFormActions() {
+	public function testGetFormActions()
+	{
 		// generating the fieldset of actions
 		$form = $this->objFromFixture('UserDefinedForm', 'basic-form-page');
 
@@ -155,6 +189,7 @@ class UserDefinedFormControllerTest extends FunctionalTest {
 
 		// by default will have 1 submit button which links to process
 		$expected = new FieldList(new FormAction('process', 'Submit'));
+		$expected->setForm($controller->Form());
 
 		$this->assertEquals($actions, $expected);
 
@@ -165,11 +200,12 @@ class UserDefinedFormControllerTest extends FunctionalTest {
 
 		$expected = new FieldList(new FormAction('process', 'Custom Button'));
 		$expected->push(new ResetFormAction("clearForm", "Clear"));
+		$expected->setForm($controller->Form());
 
 		$this->assertEquals($actions, $expected);
 	}
 
-	function testRenderingIntoFormTemplate() {
+	public function testRenderingIntoFormTemplate() {
 		$form = $this->setupFormFrontend();
 
 		$form->Content = 'This is some content without a form nested between it';
@@ -184,7 +220,7 @@ class UserDefinedFormControllerTest extends FunctionalTest {
 		$this->checkTemplateIsCorrect($parser);
 	}
 
-	function testRenderingIntoTemplateWithSubstringReplacement() {
+	public function testRenderingIntoTemplateWithSubstringReplacement() {
 		$form = $this->setupFormFrontend();
 
 		$controller = new UserDefinedFormControllerTest_Controller($form);
@@ -195,9 +231,14 @@ class UserDefinedFormControllerTest extends FunctionalTest {
 
 		$this->checkTemplateIsCorrect($parser);
 	}
-
+	/**
-	function setupFormFrontend() {
+	 * Publish a form for use on the frontend
-		$form = $this->objFromFixture('UserDefinedForm', 'basic-form-page');
+	 *
+	 * @param string $fixtureName
+	 * @return UserDefinedForm
+	 */
+	protected function setupFormFrontend($fixtureName = 'basic-form-page') {
+		$form = $this->objFromFixture('UserDefinedForm', $fixtureName);
 		$this->logInWithPermission('ADMIN');
 
 		$form->doPublish();
@@ -208,7 +249,7 @@ class UserDefinedFormControllerTest extends FunctionalTest {
 		return $form;
 	}
 
-	function checkTemplateIsCorrect($parser) {
+	public function checkTemplateIsCorrect($parser) {
 		$this->assertArrayHasKey(0, $parser->getBySelector('form#UserForm_Form'));
 
 		// check for the input
@@ -228,12 +269,12 @@ class UserDefinedFormControllerTest extends FunctionalTest {
 	}
 }
 
-class UserDefinedFormControllerTest_Controller extends UserDefinedForM_Controller implements TestOnly {
+class UserDefinedFormControllerTest_Controller extends UserDefinedForm_Controller implements TestOnly {
 
 	/**
 	 * Overloaded to avoid inconsistencies between 2.4.2 and 2.4.3 (disables all security tokens in unit tests by default)
 	 */
-	function Form() {
+	public function Form() {
 		$form = parent::Form();
 
 		if($form) $form->disableSecurityToken();

tests/UserDefinedFormTest.yml

@@ -100,7 +100,13 @@ EditableTextField:
     
   some-field:
     Name: SomeField
-    
+
+  another-required:
+    Name: required-text
+    Title: Required Text Field
+    Required: true
+    CustomErrorMessage: 'This field is required'
+
 EditableDropdown:
   basic-dropdown:
     Name: basic-dropdown
@@ -137,6 +143,11 @@ EditableEmailField:
     Name: email-field
     Title: Email
 
+  another-email-field:
+    Name: required-email
+    Title: Enter your email
+    CustomErrorMessage: 'That email is not valid'
+
 EditableRadioField:
   radio-field:
     Name: radio-option
@@ -251,3 +262,7 @@ UserDefinedForm:
   empty-page:
     Title: 'Page with empty step'
     Fields: =>EditableFormStep.form6step1, =>EditableTextField.field-1, =>EditableFormStep.form6step2, =>EditableTextField.field-2, =>EditableFormStep.form6step3
+
+  email-form:
+    Title: 'Page with email field'
+    Fields: =>EditableEmailField.another-email-field, =>EditableTextField.another-required