I tested this issue on simple TreeDropdownField. And yes, if input value is "0" TreeDropdownField passes validation.
My code example.

<?php

use CWP\CWP\PageTypes\BasePage;
use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\Forms\RequiredFields;
use SilverStripe\Forms\TreeDropdownField;

class Page extends BasePage
{

  private static $has_one = [
    'MyPage' => SiteTree::class,
  ];

  public function getCMSFields() 
  {
      $fields = parent::getCMSFields();

      $fields->addFieldToTab('Root.Main', TreeDropdownField::create('MyPage', 'MyPage', SiteTree::class));

      return $fields;
  }

  public function getCMSValidator()
  {
      return RequiredFields::create(
          'MyPage',
      );
  }
}

The original issue is about TreeDropdownField in react forms i.e. the form inside the popup modal used in silverstripe/linkfield. The code sample you've provided is for a TreeDropdown on the regular page edit form. I'm not sure it makes a difference or not to the validation if the TreeDropdownfield is in a react form or not. Based on the video I made when testing (linked above) it seems like it's not working the way it should with this PR?

I couldn't reproduce this solution with LinkField through the Extension, but I implemented this with another modal window. Please, see video.
Before:

After:

It looks like validation for form in LinkField doesn't work at all, since it getValidator returns null. So make it works I have to implement getValidator in LinkField. So to test TreeDropdownField in LinkField you need to do the following:

app/src/Page.php

<?php

namespace {

    use SilverStripe\Forms\RequiredFields;
    use SilverStripe\LinkField\Form\LinkField;
    use SilverStripe\LinkField\ORM\DBLink;
    use SilverStripe\LinkField\Models\Link;
    use SilverStripe\CMS\Model\SiteTree;
    use SilverStripe\Forms\CompositeValidator;

    class Page extends SiteTree
    {
        private static array $db = [
            'DbLink' => DBLink::class
        ];

        private static $has_one = [
            'HasOneLink' => Link::class,
        ];

        public function getCMSFields()
        {
            $fields = parent::getCMSFields();
            $fields->addFieldsToTab(
                'Root.Main',
                [
                    LinkField::create('HasOneLink'),
                    LinkField::create('DbLink'),
                ]
            );
            return $fields;
        }
        
        // This method validates only Page Form
        public function getCMSCompositeValidator(): CompositeValidator
        {
            $validator = parent::getCMSCompositeValidator();
    
            return $validator->addValidator(RequiredFields::create([
                'DbLink',
            ]));
        }

    }
}

app/src/Extensions/SiteTreeLinkExtension.php

<?php
namespace App\Extensions;

use SilverStripe\Forms\CompositeValidator;
use SilverStripe\Forms\RequiredFields;
use SilverStripe\ORM\DataExtension;

class SiteTreeLinkExtension extends DataExtension
{
  public function updateCMSCompositeValidator(CompositeValidator $compositeValidator): void
  {
      $compositeValidator->addValidator(RequiredFields::create([
          'PageID',
      ]));
  }
}

app/_config/extensions.yml

SilverStripe\LinkField\Models\SiteTreeLink:
  extensions:
    - App\Extensions\SiteTreeLinkExtension

And also you should implement getValidator in SilverStripe\LinkField\Form\FormFactory. Just add this method in FormFactory class.

vendor/silverstripe/linkfield/src/Form/FormFactory.php

   // This method validates Link Form
    protected function getValidator($controller, $name, $context)
    {
        if (!$context['LinkType'] instanceof DataObject) {
            return null;
        }

        $compositeValidator = $context['LinkType']->getCMSCompositeValidator();
        return $compositeValidator;
    }

You should see the following result.

I shouldn't need to add a method to a linkfield class (FormFactory.php) to test this issue :-)

Seems like there's an issue with linkfield PHP validation not working? Could you please raise a new issue on the silverstripe/linkfield repo - I did some brief testing and it looks the the legacy getCMSValidator() I've got in my sample (copied from the README.md on that module) doesn't actually make that has_one required.

I think the original issue is more about client side JS validation that's not quite working correctly? In my original test where I linked to a video it seems that this wasn't working correctly, and there hasn't been any code updates since then?

@emteknetnz, I don't quite understand your expectations regarding the behavior of this validation.
In the example you gave, validation is carried out only at the form on Page level and only checks that these field ('DbLink', 'HasOneLink') have some value, doesn't matter what kind. And this validation does not check the required fields of the nested form (LinkField). Specifically in this case, checking for the required fields cannot be carried out since the link type must first be selected. When the user adds a link, then validation occurs at the form on modal window level, but since no validator is installed in LinkField, nothing is checked.
In my first example, I showed using the TreeDropdownField on Page as an example that TreeDropdownField is not validated correctly and this fix solves this problem.
In the next two examples (here and here), I tested this solution on built-in forms in a modal window. In both cases, if "PageID" is a required field, then without this change TreeDropdownField will be validated only once because by default the value of this field is set to the empty string and does not pass this validation. The second time, when we clean the TreeDropDownfield value, we set the value to 0, which easily passes validation in the Validator.

I'm just confused. My core confusion is that I don't even know how to replicate the issue in the description. I don't know what I'm supposed to test. I've looked at the issue description multiple times and I cannot replicate the issue.

• In my example where I made a video is admittedly bad. I think. If nothing else it's using the DBLink which isn't really used in the real world.
• The first example you gave it's not using a modal
• The second example you gave a Treedropdownfield for "Insert external link" which doesn't make sense
• The third example you gave requires modifying a PHP file in the linkfield in order to test

So I don't know what I'm supposed to test here

The easiest way to make sure that the validation of a given field does not work is to simply add a TreeDropdownField to the Page or to any DataObject.
If you want to check it directly from LinkField, then you will have to add a method to validate this form, or in another way add validation for TreeDropdownField to LinkedField.
If you want to check the validation of this field in any other modal window, simply add validation for TreeDropdownField to this form. For the example in InternalLinkFormFactory, simply add PageID to RequiredFields in the getValidator method. Because I haven't found an easier way to do this.

Note that I'm not testing this with LinkField yet because that's a lot of extra complexity. Lets get the base case working first, then circle back to LinkField.

The original issue description also references vendor/silverstripe/admin/client/src/lib/Validator.js - if need be we can probably also add an explicit rule in that file for TreeDropdownField validation.

The original issue description also references vendor/silverstripe/admin/client/src/lib/Validator.js - if need be we can probably also add an explicit rule in that file for TreeDropdownField validation.

I don't think that validation should be so specified for each field. Also how explained here ThreeDropdownField by default has value = '', so I don't think why we should change default value to '0' when we clean this field., we should be constant.

A validation error is never produced for this, which makes it clear the validation logic is wrong. While this could be solved by changing the value TreeDropdownField uses to represent "empty", doing so would be a breaking change as I mentioned at the start of this comment. Instead, we should make this change to RequiredFields:

Also I don't think that this solution help us, since we need check field before we send it to the server. This is the main issue.
Also it doesn't work with nested forms.

I don't think that validation should be so specified for each field. Also how explained here ThreeDropdownField by default has value = '', so I don't think why we should change default value to '0' when we clean this field., we should be constant.

If we need to have it specified for a specific field type to avoid breaking changes, then we should do so. In this case I believe that is necessary. But lets start with the base case (i.e. just a TreeDropdownField directly on some DataObject without all this react formscaffolder stuff in the way, and then work our way up to the more complicated scenarios.

Also I don't think that this solution help us, since we need check field before we send it to the server. This is the main issue.
Also it doesn't work with nested forms.

Right now validation doesn't work for the base case. After we've solved that, I'll be happy to talk more about the LinkField scenario.

How about case where '0' will be valid value for TreeDropdownField? User can populate with ["Yes" => 1, "No" => 0].

"0" is already explicitly not a valid value for this form field, because the single empty value has been hardcoded to 0. Changing that would, as I've mentioned, be a breaking change for anyone who is already explicitly checking for that in their own custom validation. We cannot change that in a minor (let alone a patch) release.

I think we should additionally discuss TreedropdownField validation.

My 2c here would be not to have field type specific stuff in required fields validator, it shouldn't know about how each field validates itself.

This is less about the field validating itself, and more about checking if the field has a value set.
In the case of TreeDropdownField, its blank value is not an empty string or null - its empty value is "0". Without adding new API to FormField to get what each field's empty value is, we can't accomodate this without RequiredFields having some specific logic to handle TreeDropdownField - unless we change the empty value, which would be a breaking change as I've mentioned above.

I agree that having RequiredFields have a specific coupling to TreeDropdownField isn't ideal, but I think it's the only way to do this without introducing new largely redundant API or breaking changes.
Personally I think adding the coupling now and removing it in a future major release is probably the cleanest solution, but I'm open to reasoning against this.

This is less about the field validating itself, and more about checking if the field has a value set. In the case of TreeDropdownField, its blank value is not an empty string or null - its empty value is "0". Without adding new API to FormField to get what each field's empty value is, we can't accomodate this without RequiredFields having some specific logic to handle TreeDropdownField - unless we change the empty value, which would be a breaking change as I've mentioned above.

I agree that having RequiredFields have a specific coupling to TreeDropdownField isn't ideal, but I think it's the only way to do this without introducing new largely redundant API or breaking changes. Personally I think adding the coupling now and removing it in a future major release is probably the cleanest solution, but I'm open to reasoning against this.

Could we make RequiredFields forward-proofed and allow it to call ->getEmptyValue on the form field (with a hasMethod check) and introduce that API for all fields (either via that method or via a config) in a next minor? That way we can fix it here in a generic way.

Could be a bit unconventional approach, but won't be BC breaking.

That would be fine - though I don't know if we actually want to have a getEmptyValue() method? If we added that I feel like we'd just deprecate it before 6 anyway and update TreeDropdownField to have a more sensible empty value.

I think there might be a valid usecase for the empty value being a value that is not null or empty, e.g. with custom fields where people might already have custom validations.. but happy to not have that API and only have that extra handling within the RequiredFields without an actual public or documented API. It probably wouldn't be the first place. But saying that it nearly equals to having that tight coupling with one specific field type 🤷.

Yeah good point about other field types. I think at the end of the day I'd be happy with either approach.
Again I don't see that tight coupling as being a completely terrible thing in the event that it solves this bug and is cleaned up in the next major release. That's the main reason we have a major release cadence now after all, so that we can do some ugly things when we need to and then clean them up regularly.

RequiredFields is already tightly coupled to FileField - https://github.com/silverstripe/silverstripe-framework/blob/5/src/Forms/RequiredFields.php#L113 - so adding TreeDropdownField at this point isn't too horrible

Probably a more robust option here is to just work out what the Dataobject class is by going get_class($this->form->record); to get the DataObject class and then looking at what its config has_one's are seeing if the current $fieldName is suffixed with ID and maps to one of the has_one's. If so then say that 0 is the empty value, rather than the current strlen() test

Closing in favor of PRs on #1616