FIX Don't reload form session data using FormField::setSubmittedValue (#8056)

2024-10-22 14:05:37 +02:00 · 2018-06-15 17:57:17 +12:00 · 2018-06-15 17:57:17 +12:00 · 3fa2c056d7
commit 3fa2c056d7
parent 5f6eec6403
3 changed files with 237 additions and 5 deletions
--- 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);
--- 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(
+            '<input type="text" name="SomeDateField" value="15/06/2018"',
+            $body,
+            'Our reloaded form should contain a SomeDateField with the value "15/06/2018"'
+        );
+
+        $this->assertContains(
+            '<input type="text" name="SomeFrenchNumericField" value="9 876,5432" ',
+            $body,
+            'Our reloaded form should contain a SomeFrenchNumericField with the value "9 876,5432"'
+        );
+
+        $this->assertContains(
+            '<input type="text" name="SomeFrenchMoneyField[Currency]" value="NZD"',
+            $body,
+            'Our reloaded form should contain a SomeFrenchMoneyField[Currency] with the value "NZD"'
+        );
+
+        $this->assertContains(
+            '<input type="text" name="SomeFrenchMoneyField[Amount]" value="9 876,54" ',
+            $body,
+            'Our reloaded form should contain a SomeFrenchMoneyField[Amount] with the value "9 876,54"'
+        );
+
+        $this->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(
--- a/tests/php/Forms/FormTest/ControllerWithSpecialSubmittedValueFields.php
+++ b/tests/php/Forms/FormTest/ControllerWithSpecialSubmittedValueFields.php
@ -0,0 +1,103 @@
+<?php
+
+namespace SilverStripe\Forms\Tests\FormTest;
+
+use SilverStripe\Control\Controller;
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\Forms\CheckboxSetField;
+use SilverStripe\Forms\DateField;
+use SilverStripe\Forms\EmailField;
+use SilverStripe\Forms\FieldList;
+use SilverStripe\Forms\Form;
+use SilverStripe\Forms\FormAction;
+use SilverStripe\Forms\MoneyField;
+use SilverStripe\Forms\NumericField;
+use SilverStripe\Forms\RequiredFields;
+use SilverStripe\Forms\TextField;
+use SilverStripe\ORM\ValidationException;
+use SilverStripe\ORM\ValidationResult;
+use SilverStripe\View\SSViewer;
+
+/**
+ * @skipUpgrade
+ */
+class ControllerWithSpecialSubmittedValueFields extends Controller implements TestOnly
+{
+    public function __construct()
+    {
+        parent::__construct();
+        if (Controller::has_curr()) {
+            $this->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');
+    }
+}