NEW: Add extension hook for field-specific validation

[kinglozzer]

See also:

• NEW: Add extension hook for field-specific validation silverstripe-asset-admin#1305
• NEW: Add extension hooks for field-specific validation silverstripe-tagfield#234
• NEW: Add extension hook for field-specific validation silverstripe-subsites#510

Rationale:

As far as I’m aware, these are currently the only ways you can perform custom validation on form fields:

1. Subclass a form field and override validate()
2. Write a custom Validator and use that in place of RequiredFields
3. Perform validation in your form handler method(s)

These approaches all “work”, but they’re far from optimal. Adding a hook like this allows for cleaner project code, plus makes it easier to distribute validation functionality in modules.

Example:

This is a (slightly daft) example to illustrate the basic functionality this would offer:

// EmailFieldValidationExtension.php
class EmailFieldValidationExtension extends Extension
{
    protected $shouldRejectPlusAddresses = false;

    public function setRejectPlusAddresses($reject)
    {
        $this->shouldRejectPlusAddresses = $reject;
        return $this->owner;
    }

    public function updateValidationResult(&$result, $validator)
    {
        if ($this->shouldRejectPlusAddresses && strpos($this->owner->Value(), '+') !== false) {
            $result = false;
            $validator->validationError(
                $this->owner->getName(),
                'Sorry, we’re evil and don’t allow + aliases.'
            );
        }
    }
}

// SomeControllerWithAForm.php
$fields = FieldList::create(
    EmailField::create('Email', 'Where we should send spam to')
        ->setRejectPlusAddresses(true)
)

[kinglozzer]

This is broken out into a separate method to save a bunch of duplication across FormField subclasses like:

$result = true;
$this->extend('updateValidationResult', $result, $validator);
return $result;

For some fields (like ConfirmedPasswordField) this would end up being copy-pasted many times in one method, so it felt better to break it out like this.

The only other alternative I could spot would be to refactor all the validate() methods to take this approach:

$result = true;
if (failedValidation()) {
    $result = false;
}
$this->extend('updateValidationResult', $result, $validator);
return $result;

That works for simple fields (I essentially did that for TextField for example) but for ConfirmedPasswordField it means a big refactor and removing a bunch of early-returns, which just felt like a lot of work for no real gain in readability.

[GuySartorelli]

This is a great solution - makes it really easy for third-party formfields to use this functionality.

[GuySartorelli]

This looks good... though I do have a small concern that it will be expected for this to "just work" for form fields, but then we'll implement a new field and forget to do this, or people will expect this to work for 3rd party fields which haven't done this, which leads to an inconsistent DX

[michalkleiner]

Even though it was touching a lot of files I was also sort of ok with the change, thinking about the same thing as Guy, i.e. how can we make sure this works most of the times across all custom fields and 3rd party modules and custom validators etc.
I guess we can only do so much. Updating the docs around form field validation suggesting adding the hook to custom fields. Perhaps something like controllers check if custom init method makes sure parent init is called via onBeforeInit or something along those lines?

[kinglozzer]

Yeah I understand that, it’s kinda like using beforeUpdateCMSFields() in that it should be used by all core code/modules but there’s nothing to enforce/encourage that beside the docs. I’m not sure we can really do it programatically without:

A) abusing the deprecation API;
B) causing BC issues by triggering warnings in a minor release, or;
C) delaying this until 5.0 and then triggering warnings

Open to suggestions 🙂

[kinglozzer]

Have changed the target branch to 5.0 with a view to implementing option C above, but I don’t think there’s a way to do it that really makes sense.

As field implementations don’t necessarily call parent::validate(), any check for “has called extendValidationResult” would have to live outside of FormField, which means adding it to specific validator classes like RequiredFields, so we’re just moving the problem instead of really fixing it because a custom Validator may not perform these changes. It would also get quite messy for composite fields (e.g. MoneyField in core) that call validate() on their child fields, as validate() would have to perform this validation as well as the top-level validator.

I think it makes more sense to just leave this. If a developer doesn’t call ->extendValidationResult(), it’s not the end of the world - it’s a fairly simple addition to add to a module, and this is a new feature so there’s no BC risk.

[michalkleiner]

The reasoning makes sense to me, it's a ✅ from me but another pair of eyes would be good. There should be a corresponding docs PR linked, too, describing the addition and how to use it.

[kinglozzer]

Docs PR: silverstripe/developer-docs#144

[kinglozzer]

Bump

[michalkleiner]

I've approved this already but would prefer also @GuySartorelli's or @maxime-rainville's once over so we could get this to 5. It would need a separate entry into the changelogs both for the beta and the unreleased stable.

[GuySartorelli]

Sorry, I keep meaning to come back to this.

On the balance I'm okay with this - but can you please include appropriate unit tests and update documentation (both the changelog and if we have validation docs there too) to tell people that this is a new best practice?

I'll retarget this to 5.0 so it can be included in the stable 5.0.0 release.

[kinglozzer]

Thanks @GuySartorelli, I’ve added a new test with a few assertions to cover this and have updated the docs pr to add a note to the 5.0.0 changelog, linking through to the validation docs which contain more detail

[michalkleiner]

Thanks @kinglozzer for taking the time and effort to get this polished and ready to be merged in, we do appreciate it! 👍

[GuySartorelli]

This is a great solution - makes it really easy for third-party formfields to use this functionality.

[GuySartorelli]

Looks great, thank you!
Looks like there's just one failure here to sort out.

[kinglozzer]

Weird, that doesn’t happen locally so perhaps another module being loaded for CI runs has a formfield I missed (proving that this test is necessary 😅). Annoyingly I can’t spot a way to add the current class being tested to any error messages that PHPUnit outputs, so I’ve hacked in some temporary debugging output

[GuySartorelli]

Slightly more robust and permanent debugging suggestion

[GuySartorelli]

Looks like the failure was from a test formfield.... may make sense to add a if (is_a($formFieldClass, TestOnly::class, true)) {continue;} in there (maybe just add to the existing condition which continues) - we don't care if formfields that were created for testing other things fail this test.

[kinglozzer]

This API isn’t covered by phpunit’s BC promise, but it hasn’t changed from 8.x to 10.x: https://github.com/sebastianbergmann/phpunit/blob/10.0.9/src/Framework/MockObject/Rule/InvokedCount.php. I’ve included the comment for info so if it causes failures in future, this block can just be removed and the test will still run fine, just with less debugging info on error.

[kinglozzer]

It should be okay now - the remaining failure for UploadField is because this depends on silverstripe/silverstripe-asset-admin#1305. At least it proves the extra debugging code works as expected 😛

[GuySartorelli]

Nice. I've just rebased silverstripe/silverstripe-asset-admin#1305 and once that's green I'll merge it and then this rerun CI here (just in case there's some other PR further down the line that also needs to be updated).

We also run framework tests against kitchen sink - have you had a look at whether the new test passes with all of kitchen sink installed? I'm guessing there are a few fields there that will need updating as well.

[kinglozzer]

We also run framework tests against kitchen sink - have you had a look at whether the new test passes with all of kitchen sink installed? I'm guessing there are a few fields there that will need updating as well.

No it doesn’t pass, and after looking at it I’m beginning to wonder though if it’s a good idea to iterate over all subclasses of FormField like this.

Form fields for other modules might have their own argument requirements, even if they don’t need any changes following this PR they still have to be added to that switch statement. It feels wrong to be including a bunch of classes from other modules framework tests like this.

An example is Symbiote\AdvancedWorkflow\FormFields\WorkflowField::__construct() - even though it doesn’t have overload validation, it still needs a case statement added for it because its arguments are different, and those arguments include a WorkflowDefinition typed argument which means even more “external” code in core tests

[GuySartorelli]

Yeah that does seem like a bit of an anti-pattern. But we will need a test like that somewhere in kitchen sink. We don't really have a repository that exists to hold unit tests like that.... I guess we could add that to frameworktest and configure kitchen sink to include unit tests from frameworktest...
@emteknetnz this seems like the sort of thing you'd probably have thoughts about how to solve. Do you have any thoughts on how to handle this?

[emteknetnz]

Use class_exists() within module for a class that exists in a non-framework module? We already do it quite a bit

https://github.com/silverstripe/silverstripe-framework/blob/5/tests/php/Security/MemberTest.php#L1785

    public function testMapInCMSGroupsNoLeftAndMain()
    {
        if (class_exists(LeftAndMain::class)) {
            $this->markTestSkipped('LeftAndMain must not exist for this test.');
        }
        $result = Member::mapInCMSGroups();
        $this->assertInstanceOf(Map::class, $result);

        $this->assertEmpty($result, 'Without LeftAndMain, no groups are CMS groups.');
    }

We install silverstripe/installer for every github actions CI run, so the classes you're after will exist a lot of the time?

Is that a valid way to solve this?

[GuySartorelli]

The problem isn't that classes might not exist - the problem is that this test loops through all formfields, and when we have kitchen sink installed that means the framework unit test needs to include information about how to instantiate all of the kitchen sink formfields that have their own constructors.

It may be that there's only a few and it doesn't make the test particularly messy. It may be that it makes the test quite a bit messier than it currently is. But mostly it means framework's unit test ends up having cases for a bunch of very-not-core classes.

I think it's probably okay, given that it's a unit test so it won't be affecting anyone's project. But it's also not the ideal scenario.

[emteknetnz]

While it's messy i think it's much cleaner than introducing an entirely new paradigm of putting "shared" unit tests in separate module (frameworktest)

[GuySartorelli]

I agree with that on the balance. And I also think this test is necessary.
@kinglozzer could you please add the necessary changes for this to pass in kitchen sink?

[kinglozzer]

I’ve pushed a commit which covers everything in the kitchensink, I’ve raised a couple of new PRs for modules that did need this to be added (silverstripe/silverstripe-tagfield#234, silverstripe/silverstripe-subsites#510).

[GuySartorelli]

Thank you!
I've merged the asset-admin PR and rerun the failed CI jobs in this PR.
Merge on green - the other PRs will follow.