NEW Add getters and setters for public properties in ConfirmPasswordField, add tests

Some of the validation parts of ConfirmPasswordField are previously untested, this adds tests

src/Forms/ConfirmedPasswordField.php

@@ -140,12 +140,12 @@ class ConfirmedPasswordField extends FormField
         $title = isset($title) ? $title : _t('SilverStripe\\Security\\Member.PASSWORD', 'Password');
 
         // naming with underscores to prevent values from actually being saved somewhere
-        $this->children = new FieldList(
+        $this->children = FieldList::create(
-            $this->passwordField = new PasswordField(
+            $this->passwordField = PasswordField::create(
                 "{$name}[_Password]",
                 $title
             ),
-            $this->confirmPasswordfield = new PasswordField(
+            $this->confirmPasswordfield = PasswordField::create(
                 "{$name}[_ConfirmPassword]",
                 (isset($titleConfirmField)) ? $titleConfirmField : _t('SilverStripe\\Security\\Member.CONFIRMPASSWORD', 'Confirm Password')
             )
@@ -153,11 +153,11 @@ class ConfirmedPasswordField extends FormField
 
         // has to be called in constructor because Field() isn't triggered upon saving the instance
         if ($showOnClick) {
-            $this->children->push($this->hiddenField = new HiddenField("{$name}[_PasswordFieldVisible]"));
+            $this->getChildren()->push($this->hiddenField = HiddenField::create("{$name}[_PasswordFieldVisible]"));
         }
 
         // disable auto complete
-        foreach ($this->children as $child) {
+        foreach ($this->getChildren() as $child) {
             /** @var FormField $child */
             $child->setAttribute('autocomplete', 'off');
         }
@@ -176,8 +176,8 @@ class ConfirmedPasswordField extends FormField
 
     public function setTitle($title)
     {
-        parent::setTitle($title);
+        $this->getPasswordField()->setTitle($title);
-        $this->passwordField->setTitle($title);
+        return parent::setTitle($title);
     }
 
     /**
@@ -189,7 +189,7 @@ class ConfirmedPasswordField extends FormField
     {
         // Build inner content
         $fieldContent = '';
-        foreach ($this->children as $field) {
+        foreach ($this->getChildren() as $field) {
             /** @var FormField $field */
             $field->setDisabled($this->isDisabled());
             $field->setReadonly($this->isReadonly());
@@ -207,8 +207,8 @@ class ConfirmedPasswordField extends FormField
             return $fieldContent;
         }
 
-        if ($this->showOnClickTitle) {
+        if ($this->getShowOnClickTitle()) {
-            $title = $this->showOnClickTitle;
+            $title = $this->getShowOnClickTitle();
         } else {
             $title = _t(
                 __CLASS__ . '.SHOWONCLICKTITLE',
@@ -286,11 +286,11 @@ class ConfirmedPasswordField extends FormField
     /**
      * @param string $title
      *
-     * @return ConfirmedPasswordField
+     * @return $this
      */
     public function setRightTitle($title)
     {
-        foreach ($this->children as $field) {
+        foreach ($this->getChildren() as $field) {
             /** @var FormField $field */
             $field->setRightTitle($title);
         }
@@ -310,8 +310,8 @@ class ConfirmedPasswordField extends FormField
     public function setChildrenTitles($titles)
     {
         $expectedChildren = $this->getRequireExistingPassword() ? 3 : 2;
-        if (is_array($titles) && count($titles) == $expectedChildren) {
+        if (is_array($titles) && count($titles) === $expectedChildren) {
-            foreach ($this->children as $field) {
+            foreach ($this->getChildren() as $field) {
                 if (isset($titles[0])) {
                     /** @var FormField $field */
                     $field->setTitle($titles[0]);
@@ -350,7 +350,7 @@ class ConfirmedPasswordField extends FormField
                 : null;
 
             if ($this->showOnClick && isset($value['_PasswordFieldVisible'])) {
-                $this->children->fieldByName($this->getName() . '[_PasswordFieldVisible]')
+                $this->getChildren()->fieldByName($this->getName() . '[_PasswordFieldVisible]')
                     ->setValue($value['_PasswordFieldVisible']);
             }
         } else {
@@ -362,10 +362,10 @@ class ConfirmedPasswordField extends FormField
 
         //looking up field by name is expensive, so lets check it needs to change
         if ($oldValue != $this->value) {
-            $this->children->fieldByName($this->getName() . '[_Password]')
+            $this->getChildren()->fieldByName($this->getName() . '[_Password]')
                 ->setValue($this->value);
 
-            $this->children->fieldByName($this->getName() . '[_ConfirmPassword]')
+            $this->getChildren()->fieldByName($this->getName() . '[_ConfirmPassword]')
                 ->setValue($this->confirmValue);
         }
 
@@ -380,8 +380,8 @@ class ConfirmedPasswordField extends FormField
      */
     public function setName($name)
     {
-        $this->passwordField->setName($name . '[_Password]');
+        $this->getPasswordField()->setName($name . '[_Password]');
-        $this->confirmPasswordfield->setName($name . '[_ConfirmPassword]');
+        $this->getConfirmPasswordField()->setName($name . '[_ConfirmPassword]');
         if ($this->hiddenField) {
             $this->hiddenField->setName($name . '[_PasswordFieldVisible]');
         }
@@ -417,12 +417,12 @@ class ConfirmedPasswordField extends FormField
             return true;
         }
 
-        $this->passwordField->setValue($this->value);
+        $this->getPasswordField()->setValue($this->value);
-        $this->confirmPasswordfield->setValue($this->confirmValue);
+        $this->getConfirmPasswordField()->setValue($this->confirmValue);
-        $value = $this->passwordField->Value();
+        $value = $this->getPasswordField()->Value();
 
         // both password-fields should be the same
-        if ($value != $this->confirmPasswordfield->Value()) {
+        if ($value != $this->getConfirmPasswordField()->Value()) {
             $validator->validationError(
                 $name,
                 _t('SilverStripe\\Forms\\Form.VALIDATIONPASSWORDSDONTMATCH', "Passwords don't match"),
@@ -434,7 +434,7 @@ class ConfirmedPasswordField extends FormField
 
         if (!$this->canBeEmpty) {
             // both password-fields shouldn't be empty
-            if (!$value || !$this->confirmPasswordfield->Value()) {
+            if (!$value || !$this->getConfirmPasswordField()->Value()) {
                 $validator->validationError(
                     $name,
                     _t('SilverStripe\\Forms\\Form.VALIDATIONPASSWORDSNOTEMPTY', "Passwords can't be empty"),
@@ -446,29 +446,31 @@ class ConfirmedPasswordField extends FormField
         }
 
         // lengths
-        if (($this->minLength || $this->maxLength)) {
+        $minLength = $this->getMinLength();
+        $maxLength = $this->getMaxLength();
+        if ($minLength || $maxLength) {
             $errorMsg = null;
             $limit = null;
-            if ($this->minLength && $this->maxLength) {
+            if ($minLength && $maxLength) {
-                $limit = "{{$this->minLength},{$this->maxLength}}";
+                $limit = "{{$minLength},{$maxLength}}";
                 $errorMsg = _t(
-                    'SilverStripe\\Forms\\ConfirmedPasswordField.BETWEEN',
+                    __CLASS__ . '.BETWEEN',
                     'Passwords must be {min} to {max} characters long.',
-                    array('min' => $this->minLength, 'max' => $this->maxLength)
+                    ['min' => $minLength, 'max' => $maxLength]
                 );
-            } elseif ($this->minLength) {
+            } elseif ($minLength) {
-                $limit = "{{$this->minLength}}.*";
+                $limit = "{{$minLength}}.*";
                 $errorMsg = _t(
-                    'SilverStripe\\Forms\\ConfirmedPasswordField.ATLEAST',
+                    __CLASS__ . '.ATLEAST',
                     'Passwords must be at least {min} characters long.',
-                    array('min' => $this->minLength)
+                    ['min' => $minLength]
                 );
-            } elseif ($this->maxLength) {
+            } elseif ($maxLength) {
-                $limit = "{0,{$this->maxLength}}";
+                $limit = "{0,{$maxLength}}";
                 $errorMsg = _t(
-                    'SilverStripe\\Forms\\ConfirmedPasswordField.MAXIMUM',
+                    __CLASS__ . '.MAXIMUM',
                     'Passwords must be at most {max} characters long.',
-                    array('max' => $this->maxLength)
+                    ['max' => $maxLength]
                 );
             }
             $limitRegex = '/^.' . $limit . '$/';
@@ -478,16 +480,18 @@ class ConfirmedPasswordField extends FormField
                     $errorMsg,
                     "validation"
                 );
+
+                return false;
             }
         }
 
-        if ($this->requireStrongPassword) {
+        if ($this->getRequireStrongPassword()) {
             if (!preg_match('/^(([a-zA-Z]+\d+)|(\d+[a-zA-Z]+))[a-zA-Z0-9]*$/', $value)) {
                 $validator->validationError(
                     $name,
                     _t(
                         'SilverStripe\\Forms\\Form.VALIDATIONSTRONGPASSWORD',
-                        "Passwords must have at least one digit and one alphanumeric character"
+                        'Passwords must have at least one digit and one alphanumeric character'
                     ),
                     "validation"
                 );
@@ -502,8 +506,8 @@ class ConfirmedPasswordField extends FormField
                 $validator->validationError(
                     $name,
                     _t(
-                        'SilverStripe\\Forms\\ConfirmedPasswordField.CURRENT_PASSWORD_MISSING',
+                        __CLASS__ . '.CURRENT_PASSWORD_MISSING',
-                        "You must enter your current password."
+                        'You must enter your current password.'
                     ),
                     "validation"
                 );
@@ -516,7 +520,7 @@ class ConfirmedPasswordField extends FormField
                 $validator->validationError(
                     $name,
                     _t(
-                        'SilverStripe\\Forms\\ConfirmedPasswordField.LOGGED_IN_ERROR',
+                        __CLASS__ . '.LOGGED_IN_ERROR',
                         "You must be logged in to change your password."
                     ),
                     "validation"
@@ -532,7 +536,7 @@ class ConfirmedPasswordField extends FormField
                     $validator->validationError(
                         $name,
                         _t(
-                            'SilverStripe\\Forms\\ConfirmedPasswordField.CURRENT_PASSWORD_ERROR',
+                            __CLASS__ . '.CURRENT_PASSWORD_ERROR',
                             "The current password you have entered is not correct."
                         ),
                         "validation"
@@ -608,10 +612,84 @@ class ConfirmedPasswordField extends FormField
         $currentName = "{$name}[_CurrentPassword]";
         if ($show) {
             $confirmField = PasswordField::create($currentName, _t('SilverStripe\\Security\\Member.CURRENT_PASSWORD', 'Current Password'));
-            $this->children->unshift($confirmField);
+            $this->getChildren()->unshift($confirmField);
         } else {
-            $this->children->removeByName($currentName, true);
+            $this->getChildren()->removeByName($currentName, true);
         }
         return $this;
     }
+
+    /**
+     * @return PasswordField
+     */
+    public function getPasswordField()
+    {
+        return $this->passwordField;
+    }
+
+    /**
+     * @return PasswordField
+     */
+    public function getConfirmPasswordField()
+    {
+        return $this->confirmPasswordfield;
+    }
+
+    /**
+     * Set the minimum length required for passwords
+     *
+     * @param int $minLength
+     * @return $this
+     */
+    public function setMinLength($minLength)
+    {
+        $this->minLength = (int) $minLength;
+        return $this;
+    }
+
+    /**
+     * @return int
+     */
+    public function getMinLength()
+    {
+        return $this->minLength;
+    }
+
+    /**
+     * Set the maximum length required for passwords
+     *
+     * @param int $maxLength
+     * @return $this
+     */
+    public function setMaxLength($maxLength)
+    {
+        $this->maxLength = (int) $maxLength;
+        return $this;
+    }
+
+    /**
+     * @return int
+     */
+    public function getMaxLength()
+    {
+        return $this->maxLength;
+    }
+
+    /**
+     * @param bool $requireStrongPassword
+     * @return $this
+     */
+    public function setRequireStrongPassword($requireStrongPassword)
+    {
+        $this->requireStrongPassword = (bool) $requireStrongPassword;
+        return $this;
+    }
+
+    /**
+     * @return bool
+     */
+    public function getRequireStrongPassword()
+    {
+        return $this->requireStrongPassword;
+    }
 }

tests/php/Forms/ConfirmedPasswordFieldTest.php

@@ -83,33 +83,40 @@ class ConfirmedPasswordFieldTest extends SapphireTest
         $field = new ConfirmedPasswordField(
             'Test',
             'Testing',
-            array(
+            [
-            "_Password" => "abc123",
+                '_Password' => 'abc123',
-            "_ConfirmPassword" => "abc123"
+                '_ConfirmPassword' => 'abc123',
-            )
+            ]
         );
         $validator = new RequiredFields();
-        /** @skipUpgrade */
-        new Form(Controller::curr(), 'Form', new FieldList($field), new FieldList(), $validator);
         $this->assertTrue(
             $field->validate($validator),
-            "Validates when both passwords are the same"
+            'Validates when both passwords are the same'
         );
-        $field->setName("TestNew"); //try changing name of field
+        $field->setName('TestNew'); //try changing name of field
         $this->assertTrue(
             $field->validate($validator),
-            "Validates when field name is changed"
+            'Validates when field name is changed'
         );
         //non-matching password should make the field invalid
-        $field->setValue(
+        $field->setValue([
-            array(
+            '_Password' => 'abc123',
-            "_Password" => "abc123",
+            '_ConfirmPassword' => '123abc',
-            "_ConfirmPassword" => "123abc"
+        ]);
-            )
-        );
         $this->assertFalse(
             $field->validate($validator),
-            "Does not validate when passwords differ"
+            'Does not validate when passwords differ'
+        );
+
+        // Empty passwords should make the field invalid
+        $field->setCanBeEmpty(false);
+        $field->setValue([
+            '_Password' => '',
+            '_ConfirmPassword' => '',
+        ]);
+        $this->assertFalse(
+            $field->validate($validator),
+            'Empty passwords should not be allowed when canBeEmpty is false'
         );
     }
 
@@ -123,17 +130,121 @@ class ConfirmedPasswordFieldTest extends SapphireTest
             new FieldList()
         );
 
-        $form->loadDataFrom(
+        $form->loadDataFrom([
-            array(
+            'Password' => [
-            'Password' => array(
                 '_Password' => '123',
                 '_ConfirmPassword' => '999',
-            )
+            ],
-            )
+        ]);
-        );
 
         $this->assertEquals('123', $field->children->first()->Value());
         $this->assertEquals('999', $field->children->last()->Value());
         $this->assertNotEquals($field->children->first()->Value(), $field->children->last()->Value());
     }
+
+    /**
+     * @param int|null $minLength
+     * @param int|null $maxLength
+     * @param bool $expectValid
+     * @param string $expectedMessage
+     * @dataProvider lengthValidationProvider
+     */
+    public function testLengthValidation($minLength, $maxLength, $expectValid, $expectedMessage = '')
+    {
+        $field = new ConfirmedPasswordField('Test', 'Testing', [
+            '_Password' => 'abc123',
+            '_ConfirmPassword' => 'abc123',
+        ]);
+        $field->setMinLength($minLength)->setMaxLength($maxLength);
+
+        $validator = new RequiredFields();
+        $result = $field->validate($validator);
+
+        $this->assertSame($expectValid, $result, 'Validate method should return its result');
+        $this->assertSame($expectValid, $validator->getResult()->isValid());
+        if ($expectedMessage) {
+            $this->assertContains($expectedMessage, $validator->getResult()->serialize());
+        }
+    }
+
+    /**
+     * @return array[]
+     */
+    public function lengthValidationProvider()
+    {
+        return [
+            'valid: within min and max' => [3, 8, true],
+            'invalid: lower than min with max' => [8, 12, false, 'Passwords must be 8 to 12 characters long'],
+            'valid: greater than min' => [3, null, true],
+            'invalid: lower than min' => [8, null, false, 'Passwords must be at least 8 characters long'],
+            'valid: less than max' => [null, 8, true],
+            'invalid: greater than max' => [null, 4, false, 'Passwords must be at most 4 characters long'],
+
+        ];
+    }
+
+    public function testStrengthValidation()
+    {
+        $field = new ConfirmedPasswordField('Test', 'Testing', [
+            '_Password' => 'abc',
+            '_ConfirmPassword' => 'abc',
+        ]);
+        $field->setRequireStrongPassword(true);
+
+        $validator = new RequiredFields();
+        $result = $field->validate($validator);
+
+        $this->assertFalse($result, 'Validate method should return its result');
+        $this->assertFalse($validator->getResult()->isValid());
+        $this->assertContains(
+            'Passwords must have at least one digit and one alphanumeric character',
+            $validator->getResult()->serialize()
+        );
+    }
+
+    public function testTitle()
+    {
+        $this->assertNull(ConfirmedPasswordField::create('Test')->Title(), 'Should not have it\'s own title');
+    }
+
+    public function testSetTitlePropagatesToPasswordField()
+    {
+        /** @var ConfirmedPasswordField $field */
+        $field = ConfirmedPasswordField::create('Test')
+            ->setTitle('My password');
+
+        $this->assertSame('My password', $field->getPasswordField()->Title());
+    }
+
+    public function testSetRightTitlePropagatesToChildren()
+    {
+        /** @var ConfirmedPasswordField $field */
+        $field = ConfirmedPasswordField::create('Test');
+
+        $this->assertCount(2, $field->getChildren());
+        foreach ($field->getChildren() as $child) {
+            $this->assertEmpty($child->RightTitle());
+        }
+
+        $field->setRightTitle('Please confirm');
+        foreach ($field->getChildren() as $child) {
+            $this->assertSame('Please confirm', $child->RightTitle());
+        }
+    }
+
+    public function testSetChildrenTitles()
+    {
+        /** @var ConfirmedPasswordField $field */
+        $field = ConfirmedPasswordField::create('Test');
+        $field->setRequireExistingPassword(true);
+        $field->setChildrenTitles([
+            'Current Password',
+            'Password',
+            'Confirm Password',
+        ]);
+
+        $this->assertSame('Current Password', $field->getChildren()->shift()->Title());
+        $this->assertSame('Password', $field->getChildren()->shift()->Title());
+        $this->assertSame('Confirm Password', $field->getChildren()->shift()->Title());
+    }
 }