From 7dc1a7a12b5e14d8ed1bfc93a6e3fac5dbb9aa4f Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Tue, 19 Dec 2023 17:38:51 +1300 Subject: [PATCH 1/2] FIX Correctly mark ConfirmedPasswordField children as required --- src/Forms/ConfirmedPasswordField.php | 30 ++++++- .../php/Forms/ConfirmedPasswordFieldTest.php | 79 +++++++++++++++++++ 2 files changed, 108 insertions(+), 1 deletion(-) diff --git a/src/Forms/ConfirmedPasswordField.php b/src/Forms/ConfirmedPasswordField.php index 479c89d59..1707ef6a5 100644 --- a/src/Forms/ConfirmedPasswordField.php +++ b/src/Forms/ConfirmedPasswordField.php @@ -4,6 +4,7 @@ namespace SilverStripe\Forms; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DataObjectInterface; +use SilverStripe\ORM\FieldType\DBField; use SilverStripe\Security\Authenticator; use SilverStripe\Security\Security; use SilverStripe\View\HTML; @@ -200,7 +201,7 @@ class ConfirmedPasswordField extends FormField } } - $fieldContent .= $field->FieldHolder(); + $fieldContent .= $field->FieldHolder(['AttributesHTML' => $this->getAttributesHTMLForChild($field)]); } if (!$this->showOnClick) { @@ -233,6 +234,19 @@ class ConfirmedPasswordField extends FormField ); } + public function Required() + { + return !$this->canBeEmpty || parent::Required(); + } + + public function setForm($form) + { + foreach ($this->getChildren() as $field) { + $field->setForm($form); + } + return parent::setForm($form); + } + /** * Returns the children of this field for use in templating. * @return FieldList @@ -694,4 +708,18 @@ class ConfirmedPasswordField extends FormField { return $this->requireStrongPassword; } + + /** + * Get the AttributesHTML for a child field. + * Includes extra information the child isn't aware of on its own, such as whether + * it's required due to this field as a whole being required. + */ + private function getAttributesHTMLForChild(FormField $child): DBField + { + $attributes = $child->getAttributesHTML(); + if (strpos($attributes, 'required="required"') === false && $this->Required()) { + $attributes .= ' required="required" aria-required="true"'; + } + return DBField::create_field('HTMLFragment', $attributes); + } } diff --git a/tests/php/Forms/ConfirmedPasswordFieldTest.php b/tests/php/Forms/ConfirmedPasswordFieldTest.php index 990402db0..5f900df3d 100644 --- a/tests/php/Forms/ConfirmedPasswordFieldTest.php +++ b/tests/php/Forms/ConfirmedPasswordFieldTest.php @@ -383,4 +383,83 @@ class ConfirmedPasswordFieldTest extends SapphireTest $field->setRequireExistingPassword(false); $this->assertCount(2, $field->getChildren(), 'Current password field should not be removed'); } + + public function provideRequired() + { + return [ + 'can be empty' => [true], + 'cannot be empty' => [false], + ]; + } + + /** + * @dataProvider provideRequired + */ + public function testRequired(bool $canBeEmpty) + { + $field = new ConfirmedPasswordField('Test'); + $field->setCanBeEmpty($canBeEmpty); + $this->assertSame(!$canBeEmpty, $field->Required()); + } + + public function provideChildFieldsAreRequired() + { + return [ + 'not required' => [ + 'canBeEmpty' => true, + 'required' => false, + 'childrenRequired' => false, + 'expectRequired' => false, + ], + 'required via validator' => [ + 'canBeEmpty' => true, + 'required' => true, + 'childrenRequired' => false, + 'expectRequired' => true, + ], + 'children required directly' => [ + 'canBeEmpty' => true, + 'required' => false, + 'childrenRequired' => true, + 'expectRequired' => true, + ], + 'required because cannot be empty' => [ + 'canBeEmpty' => false, + 'required' => false, + 'childrenRequired' => false, + 'expectRequired' => true, + ], + ]; + } + + /** + * @dataProvider provideChildFieldsAreRequired + */ + public function testChildFieldsAreRequired(bool $canBeEmpty, bool $required, bool $childrenRequired, bool $expectRequired) + { + $form = new Form(); + $field = new ConfirmedPasswordField('Test'); + $field->setForm($form); + $field->setCanBeEmpty($canBeEmpty); + $requiredFields = []; + if ($required) { + $requiredFields[] = 'Test'; + } + if ($childrenRequired) { + $requiredFields[] = 'Test[_Password]'; + $requiredFields[] = 'Test[_ConfirmPassword]'; + } + $form->setValidator(new RequiredFields($requiredFields)); + + $rendered = $field->Field(); + $fieldOneRegex = ']*?required="required"\s+aria-required="true"\s[^>]*\/>'; + $fieldTwoRegex = ']*?required="required"\s+aria-required="true"\s[^>]*\/>'; + $regex = '/' . $fieldOneRegex . '.*' . $fieldTwoRegex . '/isu'; + + if ($expectRequired) { + $this->assertMatchesRegularExpression($regex, $rendered); + } else { + $this->assertDoesNotMatchRegularExpression($regex, $rendered); + } + } } From b979ce5896113d76533f31c29af842f299c3faa8 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Fri, 19 Jan 2024 13:27:32 +1300 Subject: [PATCH 2/2] MNT Fix test for required password fields in kitchen sink (#11111) --- .../php/Forms/ConfirmedPasswordFieldTest.php | 52 +++++++++++-------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/tests/php/Forms/ConfirmedPasswordFieldTest.php b/tests/php/Forms/ConfirmedPasswordFieldTest.php index 5f900df3d..42aa33c35 100644 --- a/tests/php/Forms/ConfirmedPasswordFieldTest.php +++ b/tests/php/Forms/ConfirmedPasswordFieldTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\Forms\Tests; +use SilverStripe\Admin\LeftAndMain; use SilverStripe\Control\Controller; use SilverStripe\Dev\SapphireTest; use SilverStripe\Forms\ConfirmedPasswordField; @@ -11,6 +12,7 @@ use SilverStripe\Forms\ReadonlyField; use SilverStripe\Forms\RequiredFields; use SilverStripe\Security\Member; use SilverStripe\Security\PasswordValidator; +use SilverStripe\View\SSViewer; class ConfirmedPasswordFieldTest extends SapphireTest { @@ -437,29 +439,37 @@ class ConfirmedPasswordFieldTest extends SapphireTest */ public function testChildFieldsAreRequired(bool $canBeEmpty, bool $required, bool $childrenRequired, bool $expectRequired) { - $form = new Form(); - $field = new ConfirmedPasswordField('Test'); - $field->setForm($form); - $field->setCanBeEmpty($canBeEmpty); - $requiredFields = []; - if ($required) { - $requiredFields[] = 'Test'; - } - if ($childrenRequired) { - $requiredFields[] = 'Test[_Password]'; - $requiredFields[] = 'Test[_ConfirmPassword]'; - } - $form->setValidator(new RequiredFields($requiredFields)); + // CWP front-end templates break this logic - but there's no easy fix for that. + // For the most part we are interested in ensuring this works in the CMS with default templates. + $originalThemes = SSViewer::get_themes(); + SSViewer::set_themes(LeftAndMain::config()->uninherited('admin_themes')); + try { + $form = new Form(); + $field = new ConfirmedPasswordField('Test'); + $field->setForm($form); + $field->setCanBeEmpty($canBeEmpty); + $requiredFields = []; + if ($required) { + $requiredFields[] = 'Test'; + } + if ($childrenRequired) { + $requiredFields[] = 'Test[_Password]'; + $requiredFields[] = 'Test[_ConfirmPassword]'; + } + $form->setValidator(new RequiredFields($requiredFields)); - $rendered = $field->Field(); - $fieldOneRegex = ']*?required="required"\s+aria-required="true"\s[^>]*\/>'; - $fieldTwoRegex = ']*?required="required"\s+aria-required="true"\s[^>]*\/>'; - $regex = '/' . $fieldOneRegex . '.*' . $fieldTwoRegex . '/isu'; + $rendered = $field->Field(); + $fieldOneRegex = ']*?required="required"\s+aria-required="true"\s[^>]*\/>'; + $fieldTwoRegex = ']*?required="required"\s+aria-required="true"\s[^>]*\/>'; + $regex = '/' . $fieldOneRegex . '.*' . $fieldTwoRegex . '/isu'; - if ($expectRequired) { - $this->assertMatchesRegularExpression($regex, $rendered); - } else { - $this->assertDoesNotMatchRegularExpression($regex, $rendered); + if ($expectRequired) { + $this->assertMatchesRegularExpression($regex, $rendered); + } else { + $this->assertDoesNotMatchRegularExpression($regex, $rendered); + } + } finally { + SSViewer::set_themes($originalThemes); } } }