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.

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

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Aug 7, 2023

I've also created #10908 for some follow-up breaking changes that should be made in CMS 6.

Issue

@GuySartorelli GuySartorelli marked this pull request as draft August 7, 2023 23:55
@GuySartorelli GuySartorelli force-pushed the pulls/4.13/new-fields-validator branch 3 times, most recently from 6e71d2c to 3bfe760 Compare August 8, 2023 00:44
@GuySartorelli GuySartorelli force-pushed the pulls/4.13/new-fields-validator branch from 3bfe760 to 5a52484 Compare August 8, 2023 01:02
@GuySartorelli GuySartorelli marked this pull request as ready for review August 8, 2023 01:07
Copy link
Member Author

@GuySartorelli GuySartorelli Aug 8, 2023

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.

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.

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

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!

Copy link
Contributor

@sabina-talipova sabina-talipova left a comment

Choose a reason for hiding this comment

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

Looks good. Test locally. Tests passed.

@sabina-talipova sabina-talipova merged commit 597d97b into silverstripe:4.13 Aug 9, 2023
@sabina-talipova sabina-talipova deleted the pulls/4.13/new-fields-validator branch August 9, 2023 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants