From 5d1c355cad00e3bbbb35d6254a43e5351d7b559c Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Wed, 26 Nov 2014 22:49:37 +1300 Subject: [PATCH] Form action validation excempts Thanks to @kinglozzer for doing the majority of work on this. See #2493. --- admin/code/CMSForm.php | 39 +---------- admin/tests/CMSFormTest.php | 127 ------------------------------------ forms/Form.php | 35 +++++++++- tests/forms/FormTest.php | 50 +++++++++++++- 4 files changed, 84 insertions(+), 167 deletions(-) delete mode 100644 admin/tests/CMSFormTest.php diff --git a/admin/code/CMSForm.php b/admin/code/CMSForm.php index ee654df49..563a2d821 100644 --- a/admin/code/CMSForm.php +++ b/admin/code/CMSForm.php @@ -8,25 +8,7 @@ * @subpackage admin */ class CMSForm extends Form { - - /** - * @var array - */ - protected $validationExemptActions = array(); - - /** - * Always return true if the current form action is exempt from validation - * - * @return boolean - */ - public function validate() { - $buttonClicked = $this->buttonClicked(); - return ( - ($buttonClicked && in_array($buttonClicked->actionName(), $this->getValidationExemptActions())) - || parent::validate() - ); - } - + /** * Route validation error responses through response negotiator, * so they return the correct markup as expected by the requesting client. @@ -49,25 +31,6 @@ class CMSForm extends Form { } } - /** - * Set actions that are exempt from validation - * - * @param array - */ - public function setValidationExemptActions($actions) { - $this->validationExemptActions = $actions; - return $this; - } - - /** - * Get a list of actions that are exempt from validation - * - * @return array - */ - public function getValidationExemptActions() { - return $this->validationExemptActions; - } - /** * Sets the response negotiator * @param ResponseNegotiator $negotiator The response negotiator to use diff --git a/admin/tests/CMSFormTest.php b/admin/tests/CMSFormTest.php deleted file mode 100644 index 7095de33e..000000000 --- a/admin/tests/CMSFormTest.php +++ /dev/null @@ -1,127 +0,0 @@ -get('CMSFormTest_Controller'); - - $response = $this->submitForm( - 'CMSForm_Form', - 'action_doSubmit', - array( - 'Email' => 'test@test.com' - ) - ); - - // Firstly, assert that required fields still work when not using an exempt action - $this->assertPartialMatchBySelector( - '#CMSForm_Form_SomeRequiredField_Holder span.required', - array( - '"Some Required Field" is required' - ), - 'Required fields show a notification on field when left blank' - ); - - // Re-submit the form using validation-exempt button - $response = $this->submitForm( - 'CMSForm_Form', - 'action_doSubmitValidationExempt', - array( - 'Email' => 'test@test.com' - ) - ); - - // The required message should be empty if validation was skipped - $items = $this->cssParser()->getBySelector('#CMSForm_Form_SomeRequiredField_Holder span.required'); - $this->assertEmpty($items); - - // And the session message should show up is submitted successfully - $this->assertPartialMatchBySelector( - '#CMSForm_Form_error', - array( - 'Validation skipped' - ), - 'Form->sessionMessage() shows up after reloading the form' - ); - } - - public function testSetValidationExemptActions() { - $form = $this->getStubForm(); - - $form->setValidationExemptActions(array('exemptaction')); - $exemptActions = $form->getValidationExemptActions(); - $this->assertEquals('exemptaction', $exemptActions[0]); - } - - protected function getStubForm() { - $form = new CMSForm( - new CMSFormTest_Controller(), - 'CMSForm', - new FieldList(), - new FieldList() - ); - - return $form; - } - -} - -class CMSFormTest_Controller extends Controller implements TestOnly { - - private static $allowed_actions = array('Form'); - - private static $url_handlers = array( - '$Action//$ID/$OtherID' => "handleAction", - ); - - protected $template = 'BlankPage'; - - public function Link($action = null) { - return Controller::join_links('CMSFormTest_Controller', $this->request->latestParam('Action'), - $this->request->latestParam('ID'), $action); - } - - public function Form() { - $form = new CMSForm( - $this, - 'Form', - new FieldList( - new EmailField('Email'), - new TextField('SomeRequiredField'), - new CheckboxSetField('Boxes', null, array('1'=>'one','2'=>'two')) - ), - new FieldList( - new FormAction('doSubmit'), - new FormAction('doSubmitValidationExempt') - ), - new RequiredFields( - 'Email', - 'SomeRequiredField' - ) - ); - $form->setValidationExemptActions(array('doSubmitValidationExempt')); - $form->setResponseNegotiator('foo'); // We aren't testing AJAX responses, so just set anything - $form->disableSecurityToken(); // Disable CSRF protection for easier form submission handling - - return $form; - } - - public function doSubmit($data, $form, $request) { - $form->sessionMessage('Test save was successful', 'good'); - return $this->redirectBack(); - } - - public function doSubmitValidationExempt($data, $form, $request) { - $form->sessionMessage('Validation skipped', 'good'); - return $this->redirectBack(); - } - - public function getViewer($action = null) { - return new SSViewer('BlankPage'); - } - -} diff --git a/forms/Form.php b/forms/Form.php index 918f16bc1..22a2d02ac 100644 --- a/forms/Form.php +++ b/forms/Form.php @@ -152,6 +152,11 @@ class Form extends RequestHandler { */ protected $attributes = array(); + /** + * @var array + */ + protected $validationExemptActions = array(); + private static $allowed_actions = array( 'handleField', 'httpSubmission', @@ -581,6 +586,25 @@ class Form extends RequestHandler { return $this; } + /** + * Set actions that are exempt from validation + * + * @param array + */ + public function setValidationExemptActions($actions) { + $this->validationExemptActions = $actions; + return $this; + } + + /** + * Get a list of actions that are exempt from validation + * + * @return array + */ + public function getValidationExemptActions() { + return $this->validationExemptActions; + } + /** * Convert this form to another format. */ @@ -1188,6 +1212,7 @@ class Form extends RequestHandler { * * This includes form validation, if it fails, we redirect back * to the form with appropriate error messages. + * Always return true if the current form action is exempt from validation * * Triggered through {@link httpSubmission()}. * @@ -1197,6 +1222,11 @@ class Form extends RequestHandler { * @return boolean */ public function validate(){ + $buttonClicked = $this->buttonClicked(); + if($buttonClicked && in_array($buttonClicked->actionName(), $this->getValidationExemptActions())) { + return true; + } + if($this->validator){ $errors = $this->validator->validate(); @@ -1535,7 +1565,10 @@ class Form extends RequestHandler { * @return FormAction */ public function buttonClicked() { - foreach($this->actions->dataFields() as $action) { + $actions = $this->actions->dataFields(); + if(!$actions) return; + + foreach($actions as $action) { if($action->hasMethod('actionname') && $this->buttonClickedFunc == $action->actionName()) { return $action; } diff --git a/tests/forms/FormTest.php b/tests/forms/FormTest.php index f9d6566a8..2ac8b1b37 100644 --- a/tests/forms/FormTest.php +++ b/tests/forms/FormTest.php @@ -228,6 +228,47 @@ class FormTest extends FunctionalTest { ); } + public function testValidationExemptActions() { + $response = $this->get('FormTest_Controller'); + + $response = $this->submitForm( + 'Form_Form', + 'action_doSubmit', + array( + 'Email' => 'test@test.com' + ) + ); + + // Firstly, assert that required fields still work when not using an exempt action + $this->assertPartialMatchBySelector( + '#Form_Form_SomeRequiredField_Holder .required', + array('"Some Required Field" is required'), + 'Required fields show a notification on field when left blank' + ); + + // Re-submit the form using validation-exempt button + $response = $this->submitForm( + 'Form_Form', + 'action_doSubmitValidationExempt', + array( + 'Email' => 'test@test.com' + ) + ); + + // The required message should be empty if validation was skipped + $items = $this->cssParser()->getBySelector('#Form_Form_SomeRequiredField_Holder .required'); + $this->assertEmpty($items); + + // And the session message should show up is submitted successfully + $this->assertPartialMatchBySelector( + '#Form_Form_error', + array( + 'Validation skipped' + ), + 'Form->sessionMessage() shows up after reloading the form' + ); + } + public function testSessionValidationMessage() { $this->get('FormTest_Controller'); @@ -634,13 +675,15 @@ class FormTest_Controller extends Controller implements TestOnly { new CheckboxSetField('Boxes', null, array('1'=>'one','2'=>'two')) ), new FieldList( - new FormAction('doSubmit') + new FormAction('doSubmit'), + new FormAction('doSubmitValidationExempt') ), new RequiredFields( 'Email', 'SomeRequiredField' ) ); + $form->setValidationExemptActions(array('doSubmitValidationExempt')); $form->disableSecurityToken(); // Disable CSRF protection for easier form submission handling return $form; @@ -651,6 +694,11 @@ class FormTest_Controller extends Controller implements TestOnly { return $this->redirectBack(); } + public function doSubmitValidationExempt($data, $form, $request) { + $form->sessionMessage('Validation skipped', 'good'); + return $this->redirectBack(); + } + public function getViewer($action = null) { return new SSViewer('BlankPage'); }