Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Form validation exempts #2506

Merged
merged 1 commit into from

5 participants

@chillu
Owner

Followup from a 3.1 change by @kinglozzer which was limited to CMSForm rather than Form.
@halkyon @sminnee Do you think this is a good API to introduce on a Form level for 3.2?
See #2493 for context.

@chillu
Owner

The build is failing because of this here: silverstripe-labs/silverstripe-travis-support#1

@halkyon
Owner

I think this is a good idea, the multiform module already does something like this as well. e.g. https://github.com/silverstripe/silverstripe-multiform/blob/master/code/model/MultiForm.php#L76

Sounds like it makes sense for this API on the Form class instead.

@chillu
Owner

I've rebased this against master, anybody keen to have a look?

@kinglozzer
Collaborator

@chillu Travis build is failing, looks like a regression from my original PR #2493 (not sure why it wasn’t picked up earlier). Looks like it’s getting stuck here: https://github.com/chillu/silverstripe-framework/blob/af4b6c955a0230b0808d147623c4854514539975/forms/Form.php#L1537

Background:

The reasons for amending Form->buttonClicked() are in these two comments: silverstripe/silverstripe-cms#863 (comment) & silverstripe/silverstripe-cms#863 (comment). Essentially, Form->buttonClicked() wouldn’t include SiteTree’s actions, as it didn’t look inside CompositeField. The change was intended to flatten those fields.

Cause of failure:

The FileField test that’s failing doesn’t have any actions, so FieldList->dataFields() is returning null, hence the “Invalid argument supplied for foreach()”. FieldList->dataFields() calls FieldList->collateDataFields(), passing $sequentialSet as an argument by reference.

Because FieldList->sequentialSet is defined as null, and because there are no actions to be added to the list, it remains null and is returned as null.

Fix:

Untested, but if we were to change protected $sequentialSet; to protected $sequentialSet = array(); in FieldList (and the same for $sequentialSaveableSet, plus amend FieldList->flushFieldsCache()) that should fix it I think.

Thoughts?


edit: or you could just add that one line of PHP instead :persevere:

@simonwelsh simonwelsh added the master label
@simonwelsh simonwelsh added this to the 3.2 alpha 1 milestone
@halkyon
Owner

@chillu There's a bunch of whitespace being re-added in the commit. Could you clean that up please? :)

@dirx dirx referenced this pull request from a commit in klitsche/silverstripe-framework
@dirx dirx fixes Form buttonClick bug 8c22d24
@chillu chillu Form action validation excempts
Thanks to @kinglozzer for doing the majority of work on this. See #2493.
5d1c355
@chillu
Owner

Rebased against master, removed whitespace. @halkyon keen to merge?

@wilr wilr merged commit a4d4d02 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 26, 2014
  1. @chillu

    Form action validation excempts

    chillu authored
    Thanks to @kinglozzer for doing the majority of work on this. See #2493.
This page is out of date. Refresh to see the latest.
View
39 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.
@@ -50,25 +32,6 @@ protected function getValidationErrorResponse() {
}
/**
- * 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
* @return Form The current form
View
127 admin/tests/CMSFormTest.php
@@ -1,127 +0,0 @@
-<?php
-
-/**
- * @package framework
- * @subpackage tests
- */
-class CMSFormTest extends FunctionalTest {
-
- public function testValidationExemptActions() {
- $response = $this->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');
- }
-
-}
View
35 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',
@@ -582,6 +587,25 @@ public function unsetValidator(){
}
/**
+ * 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.
*/
public function transformTo(FormTransformation $format) {
@@ -1188,6 +1212,7 @@ public function getLegend() {
*
* 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 @@ public function getLegend() {
* @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 @@ public function setButtonClicked($funcName) {
* @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;
}
View
50 tests/forms/FormTest.php
@@ -228,6 +228,47 @@ public function testFormMethodOverride() {
);
}
+ 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 @@ public function Form() {
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 @@ public function doSubmit($data, $form, $request) {
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');
}
Something went wrong with that request. Please try again.