v4 improvement: Standardise getCMSValidator for DataObjects/Forms #9406

chrispenny · 2020-02-13T04:14:41Z

Standardise getCMSValidator for DataObjects/Forms

In response to the issue raised here:
#9407

Dependent PRs (to be merged after this)

DEPENDENCY: Replace getCMSValidator with new framework method getCompositeValidator silverstripe-admin#1013

Changes

`ValidatorList`

The ValidatorList deals with an array of validators. You can add additional validators (or edit existing validators) through extension points/etc.

This ValidatorList is itself a subclass of Validator, so all existing methods are available on it (generally this means that it collates info from its array of validators, and then returns a single expected value, EG: a single ValidationResult), and they return values as expected. It should, therefor, be compatible with all of our current implementations/usages of Validator.

New method `getValidatorList()` on `DataObject`

Wherever we previously called getCMSValidator() we should now instead call getValidatorList().

In order to provide BC for 4, this new method will check for the existence of getCMSValidator on the DataObject, and if it exists, it will add that validator to the ValidatorList.

Extra

Some related discussion here:

silverstripe/silverstripe-admin#1005

kinglozzer

Nice work! A few tweaks but I really like the approach here, it’s always bugged me that there’s no base getCMSValidator implementation and we rely on method_exists()...

src/Forms/Form.php

src/Forms/GridField/GridFieldDetailForm.php

src/Forms/Validator.php

kinglozzer · 2020-02-13T12:02:43Z

src/Forms/ValidatorList.php

+    /**
+     * @var ArrayList|Validator[]
+     */
+    private $validators;


Any particular reason why this is an ArrayList instead of just an array?

I don't have any strong feelings about ArrayList vs array. I think I just wondered if maybe(?) an ArrayList could be more useful to someone somewhere.

I would be happy to change it.

My preference is an array as ArrayList is intended for models and model-like data structures, which this isn’t. I’m struggling to think of a use case that would benefit from it being an ArrayList 😅 you can always manually wrap it in an ArrayList anyway

src/Forms/ValidatorList.php

src/ORM/DataObject.php

src/Forms/DefaultFormFactory.php

chrispenny · 2020-02-13T18:09:59Z

Thank you for the feedback, @kinglozzer ! I really appreciate it. I'll be working on implements all of your comments today :)

kinglozzer · 2020-02-19T12:32:26Z

src/Forms/DefaultFormFactory.php

        }

+        $validator = $context['Record']->getValidatorList();
+
        // Extend validator
        $this->invokeWithExtensions('updateFormValidator', $validator, $controller, $name, $context);


I think this will need to be amended slightly - currently $validator is a single validator, not a list. I think the safest way is probably just to do:

foreach ($validator->getValidators() as $validatorListItem) { $this->invokeWithExtensions('updateFormValidator', $validatorListItem, $controller, $name, $context); }

For BC concerns, you're right, we'll need to continue to send what was previously being sent.

I would be keen to add in a second extension point as well - one for updateValidatorList. I'm not entirely sure if I can reasonably throw a deprecation notice on the old one, but at the very least, I can add some comments around it.

kinglozzer · 2020-02-19T12:37:22Z

src/Forms/ValidatorList.php

+    /**
+     * @var ArrayList|Validator[]
+     */
+    private $validators;


My preference is an array as ArrayList is intended for models and model-like data structures, which this isn’t. I’m struggling to think of a use case that would benefit from it being an ArrayList 😅 you can always manually wrap it in an ArrayList anyway

chrispenny · 2020-02-19T20:20:46Z

Thanks again for the feedback, @kinglozzer !

I have implemented those change requests :)

chrispenny · 2020-05-25T23:43:57Z

Is there any additional feedback for me on this one?

kinglozzer

I’m happy with this, I’ll give it a few days in case any other committers have any feedback/changes and then merge

robbieaverill · 2020-05-26T17:26:48Z

cc @silverstripe/core-team

sminnee

A few comments, I think the API docs could use a bit of work. Imagine that someone knows nothing about this class except what you write in the API docs.

sminnee · 2020-05-26T22:27:25Z

src/Forms/ValidatorList.php

+use SilverStripe\ORM\ValidationResult;
+
+/**
+ * Class ValidatorList


Docblock please – we're introducing a public API that will be supported for years to come.

sminnee · 2020-05-26T22:30:09Z

src/Forms/ValidatorList.php

+ *
+ * @package SilverStripe\Forms
+ */
+class ValidatorList extends Validator


Not sure about the name ValidatorList. Does the order of the validators impact behaviour? If not, it's a set rather than a list. Aside from that my instinct would be to call this a CompositeValidator, as the name ValidatorList doesn't fully communicate that it is itself a Validator.

CompositeValidator it is! Thanks.

sminnee · 2020-05-26T22:33:40Z

tests/php/Forms/ValidatorTest/TestValidatorList.php

@@ -0,0 +1,25 @@
+<?php


Could you do this with reflection instead?

Mostly I was just following the paradigm already set by the Validator tests.

OK fair enough 👍

sminnee · 2020-05-26T22:35:22Z

src/Forms/ValidatorList.php

+     * @param int $key
+     * @return ValidatorList
+     */
+    public function removeValidatorByKey(int $key): ValidatorList


How confident are we that this is the right public API to add?

Happy to make it protected. If it's a feature that folks want in the future, they'll ask for it.

I agree - it feels a bit odd, whereas removeValidatorsByType makes a lot of sense, and matches the GridFieldConfig equivalent nicely

Maybe protected and put @internal in the docblock so people know we might change it in the future.

Sounds good. Thanks.

sminnee · 2020-05-27T22:08:24Z

src/ORM/DataObject.php

+    /**
+     * @return ValidatorList
+     */
+    public function getValidatorList(): ValidatorList


This needs a better Docblock too. Again, we're introducing an API that will be used by 1000s of different sites and we need to be able to make changes to how the core uses it without breaking those sites. No pressure! ;-)

Some things we need to clarify, some of which might prompt adjustments to our API...

Is the validator applied here intended to be only used in the CMS/admin context, or is it safe to use for front-end forms as well?

How do we define the difference between those two use-cases?

If just for the admin context, should it be called getCMSValidatorList()?

If it's intended to be used multiple contexts, do we need to assume that the validator will be identical in all contexts? If not, should there be a string $context argument that could be e.g. 'cms', 'website', but app developers could also use and handle their own contexts? How should an unknown context be treated?

How should people customise this in subclasses? Overriding the method will mean that people need to re-implement the extension code, or face having extensions applied before their subclass overrides – this mirrors getCMSFields / updateCMSFields, but has been a pain there. Perhaps there needs to be a pair of methods:

one is public final and is the one you call. If contains the invokeWithExtensions() call.

one is protected and is the one you override. By default it contains the getCMSValidator polyfill.

Related to this although I think CompositeValidator a great class name, I've got more mixed feelings about calling this accessor getCompositeValidator(). What do others think? cc @robbieaverill @unclecheese

Is the validator applied here intended to be only used in the CMS/admin context, or is it safe to use for front-end forms as well?

Are Validators in general intended for both? Because the CompositeValidator can be used in the same way as a regular Validator. It has the same public API, and returns the same (single) ValidationResult.

I can definitely update the docblock to reflect whatever the answer to the above is.

How should people customise this in subclasses?

It is intended to be used in the same way as getCMSFields. I feel like that's a paradigm that we're all familiar with. If we're wanting to change that paradigm, then I'm definitely happy to take that on as part of this change - so long as it's going to be mirrored in the future for other areas (like getCMSFields, and getCMSActions).

Validator is available to all Forms. getCMSValidator() was used as the companion of getCMSFields() and was intended to be applicable to administrative fields only.

Potentially we should call this getCMSCompositeValidator() if we want to maintain the same scope of applicability?

Regarding subclassing, retaining the same setup as getCMSFields() in spite of the same limitations seems okay.

Cool. getCMSCompositeValidator() sounds good to me, and matches the current convention.

Thanks :)

sminnee

LGTM now

chrispenny · 2020-05-28T02:46:27Z

Nice! Thank you for all of the feedback, @sminnee!

chrispenny · 2020-06-07T21:53:39Z

Linting fixed. Should (hopefully) be good to go. Please let me know if there is any additional feedback.

chrispenny · 2020-07-16T22:39:46Z

How's everyone feeling about this one?

sminnee · 2020-07-16T22:49:46Z

Loz and I have given it a 👍, @unclecheese or @robbieaverill do you want to merge?

robbieaverill · 2020-07-16T22:56:53Z

It looks cool. I've not been super involved with this PR, but LGTM.

robbieaverill · 2020-07-16T22:58:37Z

Nice work @chrispenny!

chrispenny · 2020-07-16T23:26:35Z

Wooo! Thank you for all the help, team!

chrispenny mentioned this pull request Feb 13, 2020

Add extension point/s for updateCMSValidator silverstripe/silverstripe-admin#1005

Open

kinglozzer requested changes Feb 13, 2020

View reviewed changes

chrispenny mentioned this pull request Feb 13, 2020

v4 improvement: Standardise getCMSValidator for DataObjects/Forms #9407

Closed

chrispenny force-pushed the feature/standardise-get-cms-validator branch from 4c2db7d to 15cebff Compare February 13, 2020 20:23

chrispenny changed the base branch from 4.5 to 4 February 13, 2020 20:23

kinglozzer added the feedback-required/core-team label Feb 14, 2020

chrispenny mentioned this pull request Feb 18, 2020

DEPENDENCY: Replace getCMSValidator with new framework method getCompositeValidator silverstripe/silverstripe-admin#1013

Merged

1 task

chrispenny marked this pull request as ready for review February 18, 2020 22:38

chrispenny changed the title ~~WIP/POC: Standardise getCMSValidator for DataObjects/Forms~~ v4 improvement: Standardise getCMSValidator for DataObjects/Forms Feb 18, 2020

kinglozzer requested changes Feb 19, 2020

View reviewed changes

kinglozzer approved these changes May 26, 2020

View reviewed changes

robbieaverill added the affects/v4 label May 26, 2020

sminnee reviewed May 26, 2020

View reviewed changes

sminnee reviewed May 27, 2020

View reviewed changes

cpenny added 8 commits May 28, 2020 11:18

Standardise getCMSValidator for DataObjects/Forms

d7dd93f

Implemented PR feedback. Added some initial test cov

b45a356

Add base updateValidatorList method to DataExtension

f977f97

Add deprecation notice for 4.6 and update docs

11e2005

Update DefaultFormFactory extension point. Use array for validators

a2b57f0

Update name to CompositeValidator. Add docblocks

bca4be7

Use ReflectionClass for CompositeValidator tests

2765b65

Add internal note for protected method

8ba6531

chrispenny force-pushed the feature/standardise-get-cms-validator branch from 97cb176 to 8ba6531 Compare May 27, 2020 23:18

Update getter name to getCMSCompositeValidator

d4165db

sminnee approved these changes May 28, 2020

View reviewed changes

Linting fix

f72491f

robbieaverill added change/minor and removed feedback-required/core-team labels Jul 16, 2020

robbieaverill approved these changes Jul 16, 2020

View reviewed changes

robbieaverill merged commit 84b4057 into silverstripe:4 Jul 16, 2020

chrispenny deleted the feature/standardise-get-cms-validator branch July 16, 2020 23:26

kinglozzer mentioned this pull request Feb 19, 2021

CompositeValidator skips validation logic #9868

Closed

kinglozzer mentioned this pull request Nov 9, 2022

SiteConfig doesn’t use getCMSCompositeValidator API silverstripe/silverstripe-siteconfig#131

Closed

v4 improvement: Standardise getCMSValidator for DataObjects/Forms #9406

v4 improvement: Standardise getCMSValidator for DataObjects/Forms #9406

Conversation

chrispenny commented Feb 13, 2020 • edited by robbieaverill Loading

Standardise getCMSValidator for DataObjects/Forms

Dependent PRs (to be merged after this)

Changes

ValidatorList

New method getValidatorList() on DataObject

Extra

kinglozzer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrispenny commented Feb 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrispenny commented Feb 19, 2020

chrispenny commented May 25, 2020

kinglozzer left a comment

Choose a reason for hiding this comment

robbieaverill commented May 26, 2020

sminnee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrispenny May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sminnee May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sminnee left a comment

Choose a reason for hiding this comment

chrispenny commented May 28, 2020

chrispenny commented Jun 7, 2020

chrispenny commented Jul 16, 2020

sminnee commented Jul 16, 2020

robbieaverill commented Jul 16, 2020

robbieaverill commented Jul 16, 2020

chrispenny commented Jul 16, 2020

chrispenny commented Feb 13, 2020 •

edited by robbieaverill

Loading

`ValidatorList`

New method `getValidatorList()` on `DataObject`

kinglozzer left a comment •

edited

Loading

chrispenny May 27, 2020 •

edited

Loading

sminnee May 27, 2020 •

edited

Loading