From 3fa2c056d71a3286ce83017d7cafc180cee076c2 Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Fri, 15 Jun 2018 17:57:17 +1200 Subject: [PATCH 1/3] FIX Don't reload form session data using FormField::setSubmittedValue (#8056) --- src/Forms/Form.php | 16 ++- tests/php/Forms/FormTest.php | 123 ++++++++++++++++++ ...trollerWithSpecialSubmittedValueFields.php | 103 +++++++++++++++ 3 files changed, 237 insertions(+), 5 deletions(-) create mode 100644 tests/php/Forms/FormTest/ControllerWithSpecialSubmittedValueFields.php diff --git a/src/Forms/Form.php b/src/Forms/Form.php index 0619f78f5..fe3fcbd92 100644 --- a/src/Forms/Form.php +++ b/src/Forms/Form.php @@ -346,7 +346,7 @@ class Form extends ViewableData implements HasRequestHandler // load data in from previous submission upon error $data = $this->getSessionData(); if (isset($data)) { - $this->loadDataFrom($data); + $this->loadDataFrom($data, self::MERGE_AS_INTERNAL_VALUE); } return $this; } @@ -1317,9 +1317,10 @@ class Form extends ViewableData implements HasRequestHandler return $result; } - const MERGE_DEFAULT = 0; - const MERGE_CLEAR_MISSING = 1; - const MERGE_IGNORE_FALSEISH = 2; + const MERGE_DEFAULT = 0b0000; + const MERGE_CLEAR_MISSING = 0b0001; + const MERGE_IGNORE_FALSEISH = 0b0010; + const MERGE_AS_INTERNAL_VALUE = 0b0100; /** * Load data from the given DataObject or array. @@ -1340,6 +1341,7 @@ class Form extends ViewableData implements HasRequestHandler * {@link saveInto()}. * * @uses FieldList->dataFields() + * @uses FormField->setSubmittedValue() * @uses FormField->setValue() * * @param array|DataObject $data @@ -1359,6 +1361,10 @@ class Form extends ViewableData implements HasRequestHandler * Passing IGNORE_FALSEISH means that any false-ish value in {@link $data} won't replace * a field's value. * + * Passing MERGE_AS_INTERNAL_VALUE forces the data to be parsed using the internal representation of the matching + * form field. This is helpful if you are loading an array of values retrieved from `Form::getData()` and you + * do not want them parsed as submitted data. + * * For backwards compatibility reasons, this parameter can also be set to === true, which is the same as passing * CLEAR_MISSING * @@ -1468,7 +1474,7 @@ class Form extends ViewableData implements HasRequestHandler // pass original data as well so composite fields can act on the additional information if ($setValue) { - if ($submitted) { + if ($submitted && ($mergeStrategy & self::MERGE_AS_INTERNAL_VALUE) != self::MERGE_AS_INTERNAL_VALUE) { $field->setSubmittedValue($val, $data); } else { $field->setValue($val, $data); diff --git a/tests/php/Forms/FormTest.php b/tests/php/Forms/FormTest.php index ae7e1d7bd..39bc95cf1 100644 --- a/tests/php/Forms/FormTest.php +++ b/tests/php/Forms/FormTest.php @@ -8,6 +8,7 @@ use SilverStripe\Control\Session; use SilverStripe\Dev\CSSContentParser; use SilverStripe\Dev\FunctionalTest; use SilverStripe\Forms\DateField; +use SilverStripe\Forms\DatetimeField; use SilverStripe\Forms\FieldList; use SilverStripe\Forms\FileField; use SilverStripe\Forms\Form; @@ -17,12 +18,15 @@ use SilverStripe\Forms\LookupField; use SilverStripe\Forms\NumericField; use SilverStripe\Forms\PasswordField; use SilverStripe\Forms\Tests\FormTest\ControllerWithSecurityToken; +use SilverStripe\Forms\Tests\FormTest\ControllerWithSpecialSubmittedValueFields; use SilverStripe\Forms\Tests\FormTest\ControllerWithStrictPostCheck; use SilverStripe\Forms\Tests\FormTest\Player; use SilverStripe\Forms\Tests\FormTest\Team; use SilverStripe\Forms\Tests\FormTest\TestController; +use SilverStripe\Forms\Tests\ValidatorTest\TestValidator; use SilverStripe\Forms\TextareaField; use SilverStripe\Forms\TextField; +use SilverStripe\Forms\TimeField; use SilverStripe\ORM\ValidationResult; use SilverStripe\Security\NullSecurityToken; use SilverStripe\Security\RandomGenerator; @@ -46,6 +50,7 @@ class FormTest extends FunctionalTest TestController::class, ControllerWithSecurityToken::class, ControllerWithStrictPostCheck::class, + ControllerWithSpecialSubmittedValueFields::class ]; protected static $disable_themes = true; @@ -261,6 +266,65 @@ class FormTest extends FunctionalTest ); } + public function testLoadDataFromWithForceSetValueFlag() + { + // Get our data formatted in internal value and in submitted value + // We're using very esoteric date and time format + $dataInSubmittedValue = [ + 'SomeDateTimeField' => 'Fri, Jun 15, \'18 17:28:05', + 'SomeTimeField' => '05 o\'clock PM 28 05' + ]; + $dataInInternalValue = [ + 'SomeDateTimeField' => '2018-06-15 17:28:05', + 'SomeTimeField' => '17:28:05' + ]; + + // Test loading our data with the MERGE_FORCE_SET_VALUE + $form = new Form( + Controller::curr(), + 'Form', + new FieldList( + $dateField = DatetimeField::create('SomeDateTimeField') + ->setHTML5(false) + ->setDatetimeFormat("EEE, MMM d, ''yy HH:mm:ss"), + $timeField = TimeField::create('SomeTimeField') + ->setHTML5(false) + ->setTimeFormat("hh 'o''clock' a mm ss") // Swatch Internet Time format + ), + new FieldList() + ); + + $form->loadDataFrom($dataInInternalValue, Form::MERGE_AS_INTERNAL_VALUE); + + $this->assertEquals( + $dataInInternalValue, + $form->getData() + ); + + + // Test loading our data without the MERGE_FORCE_SET_VALUE + $form = new Form( + Controller::curr(), + 'Form', + new FieldList( + $dateField = DatetimeField::create('SomeDateTimeField') + ->setHTML5(false) + ->setDatetimeFormat("EEE, MMM d, ''yy HH:mm:ss"), + $timeField = TimeField::create('SomeTimeField') + ->setHTML5(false) + ->setTimeFormat("hh 'o''clock' a mm ss") // Swatch Internet Time format + ), + new FieldList() + ); + + $form->loadDataFrom($dataInSubmittedValue); + + $this->assertEquals( + $dataInInternalValue, + $form->getData() + ); + } + public function testLookupFieldDisabledSaving() { $object = new Team(); @@ -969,6 +1033,65 @@ class FormTest extends FunctionalTest } } + /** + * This test confirms that when a form validation fails, the submitted value are stored in the session and are + * reloaded correctly once the form is re-rendered. This indirectly test `Form::restoreFormState`, + * `Form::setSessionData`, `Form::getSessionData` and `Form::clearFormState`. + */ + public function testRestoreFromState() + { + // Use a specially crafted controlled for this request. The associated form contains fields that override the + // `setSubmittedValue` and require an internal format that differs from the submitted format. + $this->get('FormTest_ControllerWithSpecialSubmittedValueFields')->getBody(); + + // Posting our form. This should fail and redirect us to the form page and preload our submit value + $response = $this->post( + 'FormTest_ControllerWithSpecialSubmittedValueFields/Form', + array( + 'SomeDateField' => '15/06/2018', + 'SomeFrenchNumericField' => '9 876,5432', + 'SomeFrenchMoneyField' => [ + 'Amount' => '9 876,54', + 'Currency' => 'NZD' + ] + // Validation will fail because we leave out SomeRequiredField + ), + [] + ); + + // Test our reloaded form field + $body = $response->getBody(); + $this->assertContains( + 'assertContains( + 'assertContains( + 'assertContains( + 'assertEmpty( + $this->mainSession->session()->get('FormInfo.Form_Form'), + 'Our form was reloaded successfully. That should have cleared our session.' + ); + + } + protected function getStubForm() { return new Form( diff --git a/tests/php/Forms/FormTest/ControllerWithSpecialSubmittedValueFields.php b/tests/php/Forms/FormTest/ControllerWithSpecialSubmittedValueFields.php new file mode 100644 index 000000000..c373b124a --- /dev/null +++ b/tests/php/Forms/FormTest/ControllerWithSpecialSubmittedValueFields.php @@ -0,0 +1,103 @@ +setRequest(Controller::curr()->getRequest()); + } + } + + 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( + 'FormTest_ControllerWithSpecialSubmittedValueFields', + $this->getRequest()->latestParam('Action'), + $this->getRequest()->latestParam('ID'), + $action + ); + } + + public function Form() + { + $form = new Form( + $this, + 'Form', + new FieldList( + new TextField('SomeRequiredField'), + DateField::create('SomeDateField') + ->setHTML5(false) + ->setDateFormat('dd/MM/yyyy') + ->setValue('2000-01-01'), + NumericField::create('SomeFrenchNumericField') + ->setHTML5(false) + ->setLocale('fr_FR') + ->setScale(4) + ->setValue(12345.6789), + MoneyField::create('SomeFrenchMoneyField') + ->setValue('100.5 EUR') + ->setLocale('fr_FR') + ), + new FieldList( + FormAction::create('doSubmit') + ), + new RequiredFields( + 'SomeRequiredField' + ) + ); + $form->setValidationExemptActions(array('doSubmitValidationExempt')); + $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 doTriggerException($data, $form, $request) + { + $result = new ValidationResult(); + $result->addFieldError('Email', 'Error on Email field'); + $result->addError('Error at top of form'); + throw new ValidationException($result); + } + + public function getViewer($action = null) + { + return new SSViewer('BlankPage'); + } +} From 7fda52b2cdc55e8a7f635a8b50a5b211011eba00 Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Mon, 18 Jun 2018 11:51:52 +1200 Subject: [PATCH 2/3] Add a MERGE_AS_SUBMITTED_VALUE flag for Form::loadDataFrom --- src/Forms/Form.php | 22 ++++++++--- tests/php/Forms/FormTest.php | 72 +++++++++++++++++++----------------- 2 files changed, 55 insertions(+), 39 deletions(-) diff --git a/src/Forms/Form.php b/src/Forms/Form.php index fe3fcbd92..4ce24d70f 100644 --- a/src/Forms/Form.php +++ b/src/Forms/Form.php @@ -1317,10 +1317,11 @@ class Form extends ViewableData implements HasRequestHandler return $result; } - const MERGE_DEFAULT = 0b0000; - const MERGE_CLEAR_MISSING = 0b0001; - const MERGE_IGNORE_FALSEISH = 0b0010; - const MERGE_AS_INTERNAL_VALUE = 0b0100; + const MERGE_DEFAULT = 0b0000; + const MERGE_CLEAR_MISSING = 0b0001; + const MERGE_IGNORE_FALSEISH = 0b0010; + const MERGE_AS_INTERNAL_VALUE = 0b0100; + const MERGE_AS_SUBMITTED_VALUE = 0b1000; /** * Load data from the given DataObject or array. @@ -1363,7 +1364,8 @@ class Form extends ViewableData implements HasRequestHandler * * Passing MERGE_AS_INTERNAL_VALUE forces the data to be parsed using the internal representation of the matching * form field. This is helpful if you are loading an array of values retrieved from `Form::getData()` and you - * do not want them parsed as submitted data. + * do not want them parsed as submitted data. MERGE_AS_SUBMITTED_VALUE does the opposite and forces the data to be + * parsed as it would be submitted from a form. * * For backwards compatibility reasons, this parameter can also be set to === true, which is the same as passing * CLEAR_MISSING @@ -1395,6 +1397,14 @@ class Form extends ViewableData implements HasRequestHandler $submitted = false; } + // Using the `MERGE_AS_INTERNAL_VALUE` or `MERGE_AS_SUBMITTED_VALUE` flags users can explicitly specify which + // `setValue` method to use. + if (($mergeStrategy & self::MERGE_AS_INTERNAL_VALUE) == self::MERGE_AS_INTERNAL_VALUE) { + $submitted = false; + } else if (($mergeStrategy & self::MERGE_AS_SUBMITTED_VALUE) == self::MERGE_AS_SUBMITTED_VALUE) { + $submitted = true; + } + // dont include fields without data $dataFields = $this->Fields()->dataFields(); if (!$dataFields) { @@ -1474,7 +1484,7 @@ class Form extends ViewableData implements HasRequestHandler // pass original data as well so composite fields can act on the additional information if ($setValue) { - if ($submitted && ($mergeStrategy & self::MERGE_AS_INTERNAL_VALUE) != self::MERGE_AS_INTERNAL_VALUE) { + if ($submitted) { $field->setSubmittedValue($val, $data); } else { $field->setValue($val, $data); diff --git a/tests/php/Forms/FormTest.php b/tests/php/Forms/FormTest.php index 39bc95cf1..8b9a2948b 100644 --- a/tests/php/Forms/FormTest.php +++ b/tests/php/Forms/FormTest.php @@ -31,6 +31,7 @@ use SilverStripe\ORM\ValidationResult; use SilverStripe\Security\NullSecurityToken; use SilverStripe\Security\RandomGenerator; use SilverStripe\Security\SecurityToken; +use SilverStripe\View\ArrayData; use SilverStripe\View\SSViewer; /** @@ -279,21 +280,8 @@ class FormTest extends FunctionalTest 'SomeTimeField' => '17:28:05' ]; - // Test loading our data with the MERGE_FORCE_SET_VALUE - $form = new Form( - Controller::curr(), - 'Form', - new FieldList( - $dateField = DatetimeField::create('SomeDateTimeField') - ->setHTML5(false) - ->setDatetimeFormat("EEE, MMM d, ''yy HH:mm:ss"), - $timeField = TimeField::create('SomeTimeField') - ->setHTML5(false) - ->setTimeFormat("hh 'o''clock' a mm ss") // Swatch Internet Time format - ), - new FieldList() - ); - + // Test loading our data with the MERGE_AS_INTERNAL_VALUE + $form = $this->getStubFormWithWeirdValueFormat(); $form->loadDataFrom($dataInInternalValue, Form::MERGE_AS_INTERNAL_VALUE); $this->assertEquals( @@ -301,22 +289,16 @@ class FormTest extends FunctionalTest $form->getData() ); - - // Test loading our data without the MERGE_FORCE_SET_VALUE - $form = new Form( - Controller::curr(), - 'Form', - new FieldList( - $dateField = DatetimeField::create('SomeDateTimeField') - ->setHTML5(false) - ->setDatetimeFormat("EEE, MMM d, ''yy HH:mm:ss"), - $timeField = TimeField::create('SomeTimeField') - ->setHTML5(false) - ->setTimeFormat("hh 'o''clock' a mm ss") // Swatch Internet Time format - ), - new FieldList() + // Test loading our data with the MERGE_AS_SUBMITTED_VALUE and an data passed as an object + $form = $this->getStubFormWithWeirdValueFormat(); + $form->loadDataFrom(ArrayData::create($dataInSubmittedValue), Form::MERGE_AS_SUBMITTED_VALUE); + $this->assertEquals( + $dataInInternalValue, + $form->getData() ); + // Test loading our data without the MERGE_AS_INTERNAL_VALUE and without MERGE_AS_SUBMITTED_VALUE + $form = $this->getStubFormWithWeirdValueFormat(); $form->loadDataFrom($dataInSubmittedValue); $this->assertEquals( @@ -339,10 +321,10 @@ class FormTest extends FunctionalTest $form->loadDataFrom( array( 'Players' => array( - 14, - 18, - 22 - ), + 14, + 18, + 22 + ), ) ); $form->saveInto($object); @@ -1101,4 +1083,28 @@ class FormTest extends FunctionalTest new FieldList() ); } + + /** + * Some fields handle submitted values differently from their internal values. This forms contains 2 such fields + * * a SomeDateTimeField that expect a date such as `Fri, Jun 15, '18 17:28:05`, + * * a SomeTimeField that expects it's time as `05 o'clock PM 28 05` + * + * @return Form + */ + protected function getStubFormWithWeirdValueFormat() + { + return new Form( + Controller::curr(), + 'Form', + new FieldList( + $dateField = DatetimeField::create('SomeDateTimeField') + ->setHTML5(false) + ->setDatetimeFormat("EEE, MMM d, ''yy HH:mm:ss"), + $timeField = TimeField::create('SomeTimeField') + ->setHTML5(false) + ->setTimeFormat("hh 'o''clock' a mm ss") // Swatch Internet Time format + ), + new FieldList() + ); + } } From c77042aa8b5ba70f4c9dc6924d2ccccf3d5588a5 Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Tue, 19 Jun 2018 09:11:16 +1200 Subject: [PATCH 3/3] Fix linting. --- src/Forms/Form.php | 2 +- tests/php/Forms/FormTest.php | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Forms/Form.php b/src/Forms/Form.php index 4ce24d70f..3f5f95771 100644 --- a/src/Forms/Form.php +++ b/src/Forms/Form.php @@ -1401,7 +1401,7 @@ class Form extends ViewableData implements HasRequestHandler // `setValue` method to use. if (($mergeStrategy & self::MERGE_AS_INTERNAL_VALUE) == self::MERGE_AS_INTERNAL_VALUE) { $submitted = false; - } else if (($mergeStrategy & self::MERGE_AS_SUBMITTED_VALUE) == self::MERGE_AS_SUBMITTED_VALUE) { + } elseif (($mergeStrategy & self::MERGE_AS_SUBMITTED_VALUE) == self::MERGE_AS_SUBMITTED_VALUE) { $submitted = true; } diff --git a/tests/php/Forms/FormTest.php b/tests/php/Forms/FormTest.php index 8b9a2948b..3322eb1b5 100644 --- a/tests/php/Forms/FormTest.php +++ b/tests/php/Forms/FormTest.php @@ -1071,7 +1071,6 @@ class FormTest extends FunctionalTest $this->mainSession->session()->get('FormInfo.Form_Form'), 'Our form was reloaded successfully. That should have cleared our session.' ); - } protected function getStubForm()