Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEW Add FieldsValidator to ensure fields get validated #10907

Merged
merged 1 commit into from
Aug 9, 2023
+136 −17
Merged

NEW Add FieldsValidator to ensure fields get validated #10907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/Forms/FieldsValidator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

namespace SilverStripe\Forms;

/**
* Validates the internal state of all fields in the form.
*/
class FieldsValidator extends Validator
{
public function php($data): bool
{
$valid = true;
$fields = $this->form->Fields();

foreach ($fields as $field) {
$valid = ($field->validate($this) && $valid);
michalkleiner marked this conversation as resolved.
Show resolved Hide resolved
}

return $valid;
}

public function canBeCached(): bool
{
return true;
}
}
5 changes: 3 additions & 2 deletions src/ORM/DataObject.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use SilverStripe\Forms\FormField;
use SilverStripe\Forms\FormScaffolder;
use SilverStripe\Forms\CompositeValidator;
use SilverStripe\Forms\FieldsValidator;
use SilverStripe\Forms\HiddenField;
use SilverStripe\i18n\i18n;
use SilverStripe\i18n\i18nEntityProvider;
Expand Down Expand Up @@ -2600,7 +2601,7 @@ public function getCMSActions()
*/
public function getCMSCompositeValidator(): CompositeValidator
{
$compositeValidator = CompositeValidator::create();
$compositeValidator = CompositeValidator::create([FieldsValidator::create()]);

// Support for the old method during the deprecation period
if ($this->hasMethod('getCMSValidator')) {
Expand Down Expand Up @@ -3735,7 +3736,7 @@ private function getDatabaseBackedField(string $fieldPath): ?string
return null;
}
$fieldParts[] = $nextPart;

if ($component instanceof Relation || $component instanceof DataList) {
if ($component->hasMethod($nextPart)) {
// If the next part is a method, we don't have a database-backed field.
Expand Down
36 changes: 24 additions & 12 deletions tests/behat/src/CmsUiContext.php
View file
Open in desktop
Copy link
Member Author

@GuySartorelli GuySartorelli Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to this file should be fine, since behat contexts aren't intended to be subclassed. This is effectively the same as a unit test fixture for some custom functionality we want to use in tests.

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use Behat\Behat\Hook\Scope\AfterStepScope;
use Behat\Mink\Element\Element;
use Behat\Mink\Element\NodeElement;
use Behat\Mink\Exception\ElementNotFoundException;
use Behat\Mink\Selector\Xpath\Escaper;
use Behat\Mink\Session;
use PHPUnit\Framework\Assert;
Expand Down Expand Up @@ -86,26 +87,37 @@ public function iShouldSeeANotice($notice)
}

/**
* @Then /^I should see a "(.+)" (\w+) toast$/
* @Then /^I should (not |)see a "(.+)" (\w+) toast$/
*/
public function iShouldSeeAToast($notice, $type)
public function iShouldSeeAToast($not, $notice, $type)
{
$this->getMainContext()->assertElementContains('.toast--' . $type, $notice);
if ($not) {
try {
// If there is a toast of that type, ensure it doesn't contain the notice.
$this->getMainContext()->assertElementNotContains('.toast--' . $type, $notice);
} catch (ElementNotFoundException $e) {
// no-op - if the element doesn't exist at all, then that passes the test.
}
} else {
$this->getMainContext()->assertElementContains('.toast--' . $type, $notice);
}
}

/**
* @Then /^I should see a "(.+)" (\w+) toast with these actions: (.+)$/
* @Then /^I should (not |)see a "(.+)" (\w+) toast with these actions: (.+)$/
*/
public function iShouldSeeAToastWithAction($notice, $type, $actions)
public function iShouldSeeAToastWithAction($not, $notice, $type, $actions)
{
$this->iShouldSeeAToast($notice, $type);
$this->iShouldSeeAToast($not, $notice, $type);

$actions = explode(',', $actions ?? '');
foreach ($actions as $order => $action) {
$this->getMainContext()->assertElementContains(
sprintf('.toast--%s .toast__action:nth-child(%s)', $type, $order+1),
trim($action ?? '')
);
if (!$not) {
$actions = explode(',', $actions ?? '');
foreach ($actions as $order => $action) {
$this->getMainContext()->assertElementContains(
sprintf('.toast--%s .toast__action:nth-child(%s)', $type, $order+1),
trim($action ?? '')
);
}
}
}

Expand Down
79 changes: 79 additions & 0 deletions tests/php/Forms/FieldsValidatorTest.php
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add additional test case for another fields if possible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can, but can you please explain why? We're not testing if all form fields have implemented good validation logic - we're just testing if this new validator type correctly validates all fields in the form. The type of the field isn't really relevant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I add this comment as my suggestion / question in other PR.
But my small concern and question, will this solution work if user decided somehow to override parent method or add another validator.

Copy link
Member Author

@GuySartorelli GuySartorelli Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the developer implements it like this, it will work:

public function getCMSCompositeValidator(): CompositeValidator
{
    $validator = parent::getCMSCompositeValidator();
    $validator->addValidator(MyValidator::create());
    return $validator;
}

If they implement it like this, then they are explicitly choosing to ignore all validators previously set, and we shouldn't in that case force a validator on them:

public function getCMSCompositeValidator(): CompositeValidator
{
    $validator = CompositeValidator::create();
    $validator->addValidator(MyValidator::create());
    return $validator;
}

The latter scenario is the equivalent of this code before the composite validator was introduced:

public function getCMSValidator()
{
    return MyValidator::create();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Sounds good!

Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
<?php

namespace SilverStripe\Forms\Tests;

use SilverStripe\Dev\SapphireTest;
use SilverStripe\Forms\EmailField;
use SilverStripe\Forms\FieldList;
use SilverStripe\Forms\FieldsValidator;
use SilverStripe\Forms\Form;

class FieldsValidatorTest extends SapphireTest
{
protected $usesDatabase = false;

public function provideValidation()
{
return [
'missing values arent invalid' => [
'values' => [],
'isValid' => true,
],
'empty values arent invalid' => [
'values' => [
'EmailField1' => '',
'EmailField2' => null,
],
'isValid' => true,
],
'any invalid is invalid' => [
'values' => [
'EmailField1' => '[email protected]',
'EmailField2' => 'not email',
],
'isValid' => false,
],
'all invalid is invalid' => [
'values' => [
'EmailField1' => 'not email',
'EmailField2' => 'not email',
],
'isValid' => false,
],
'all valid is valid' => [
'values' => [
'EmailField1' => '[email protected]',
'EmailField2' => '[email protected]',
],
'isValid' => true,
],
];
}

/**
* @dataProvider provideValidation
*/
public function testValidation(array $values, bool $isValid)
{
$fieldList = new FieldList([
$field1 = new EmailField('EmailField1'),
$field2 = new EmailField('EmailField2'),
]);
if (array_key_exists('EmailField1', $values)) {
$field1->setValue($values['EmailField1']);
}
if (array_key_exists('EmailField2', $values)) {
$field2->setValue($values['EmailField2']);
}
$form = new Form(null, 'testForm', $fieldList, new FieldList([/* no actions */]), new FieldsValidator());

$result = $form->validationResult();
$this->assertSame($isValid, $result->isValid());
$messages = $result->getMessages();
if ($isValid) {
$this->assertEmpty($messages);
} else {
$this->assertNotEmpty($messages);
}
}
}
7 changes: 4 additions & 3 deletions tests/php/Security/GroupTest.php
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a new default validator in the composite validator now, so we have to change the logic in this test in order to reliably get the correct one.

Original file line number Diff line number Diff line change
Expand Up @@ -290,10 +290,11 @@ public function testGroupTitleValidation()

$newGroup = new Group();

$validators = $newGroup->getCMSCompositeValidator()->getValidators();
$validators = $newGroup->getCMSCompositeValidator()->getValidatorsByType(RequiredFields::class);
$this->assertCount(1, $validators);
$this->assertInstanceOf(RequiredFields::class, $validators[0]);
$this->assertTrue(in_array('Title', $validators[0]->getRequired() ?? []));
$validator = array_shift($validators);
$this->assertInstanceOf(RequiredFields::class, $validator);
$this->assertTrue(in_array('Title', $validator->getRequired() ?? []));

$newGroup->Title = $group1->Title;
$result = $newGroup->validate();
Expand Down