SPIKE Review option for handling cleared TreeTropDown validation

[maxime-rainville]

Parent issue

#1576

Description

It's not clear to us what the "No value" value for a TreeDropdownField should be.

• Should any "falsy" value be considered a none-value?
• Is Zero a valid ID value?
• Can this be fixed in a none breaking way?
• If we're allowed to break things, what would be the ideal approach?

Identify a few alternative/approach and seek agreement from beloved colleagues.

Timebox

Half-day

Note

• 0 is likely the current "no value" value because when you load data for a has_one when no has_one has been saved to the relation, its value will be 0.

PRs

• FIX Add extraEmptyValues to TreedropdownField silverstripe-framework#11044
• FIX Handle extra empty values #1621
• FIX Handle extra empty values #1627
• FIX Add extraEmptyValues to TreedropdownField silverstripe-framework#11068

[emteknetnz]

To summarise a big discussion on #1578:

1. The default submitted value of a TreedropdownField in a regular Page edit form is "0"
2. The default submitted value of a TreedropdownField for a new record in a FormBuilder form is null (I think, based on debugging the json blob that linkfield v3 SiteTreeLink passes to the parent Page which is then POSTed)
3. The default value of a TreedownField for a value that was cleared using the 'x' button is 0 i.e. SINGLE_EMPTY_VALUE from TreeDropdownField.js
4. The original issue from Bernie was specifically about the front-end only redux-form validation logic that's contained in Validator.js
5. Sabina's earlier PR recommended changing SINGLE_EMPTY_VALUE in TreeDropdownField.js from 0 to '', which does fix the frontend validation issue
6. Guy didn't like that approach because it's something of a breaking change because it means that the Treedropdown field would be submitting a blank string instead of "0". However as noted in 2. this can already happen.
7. Guy's feedback centered around RequiredFields.php which is server-side validation, there was discussion around whether it's OK to couple logic pecifically for TreeDropdownField in RequiredFields, the consensus was that it's OK
8. Linkfield 3.x-dev serverside validation is broken, however frontend validation works

I've investigated this pretty deeply. I've created a pair of PRs that I think provide a fairly robust solution

FormBuilder/redux-form is submitting null values because the default value of TreeDropdownfield inherits from FormField, which has a default value of null. Changing the default value of TreedropdownField to 0 makes this consistent with what happens if the TreedropField is in a page edit form, it means that it submits a "0" if left untouched. TreedropdownField only supports DataObjects that have the Hierarchy extension, and Hierarchy very much only works with relational data, rather than arbitary data, so it should have a default value of 0 which is the same as the default value of a RelationID. I count this a bugfix because the previous default value was incorrect, so we can release in a patch

Despite this I still think we need to treat both blank string and 0 as empty values for both the frontend and serverside validation. This is because the proposed fixes need to go in both framework and admin modules and you can install one module with the fix and one without.

I've fixed serverside validation with an update to RequiredFields.php adding in a condition for TreeDropdownField where it will treat both blank string and "0" as empty, so will fail validation

I've fixed frontend validation with an update to Validator.js so that it's able to support passing in "extraEmptyValues" that are considered alongside empty string. Empty string should fail required fields validation is all scenarios. I've updated the TreeDropdownField to pass in a "0" for this.

[emteknetnz]

You can test TreeDropdownfield on a page and a FormBuilder form with LinkField 3.x-dev with the following code, assuming you've also installed linkfield

<?php

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

class Page extends SiteTree
{
    private static $db = [];

    private static $has_one = [
        'TestValidationPage' => SiteTree::class,
        'MyLink' => Link::class,
    ];
    
    public function getCMSFields()
    {
        $fields = parent::getCMSFields();
    
        $fields->addFieldsToTab('Root.Main', [
            TreeDropdownField::create('TestValidationPageID', null, SiteTree::class),
            LinkField::create('MyLink'),
        ]);
    
        return $fields;
    }
    
    public function getCMSCompositeValidator(): CompositeValidator
    {
        $validator = parent::getCMSCompositeValidator();
    
        $validator->addValidator(RequiredFields::create([
            'TestValidationPageID',
        ]));
    
        return $validator;
    }
}

And add the following to the bottom on SiteTreeLink.php (from the linkfield module, not the cms module)

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

[maxime-rainville]

I only read the description. Didn't look at the PR. Seems to make broad sense.

My only question is around this.

TreedropdownField only supports DataObjects that have the Hierarchy extension, and Hierarchy very much only works with relational data, rather than arbitary data, so it should have a default value of 0 which is the same as the default value of a RelationID.

Presumably, if we want to select the parent of a Page, 0 means that the page is in the root of the site ... it does not have a parent in other words.

In this framework, I'm assuming there's no distinction between, "I have not selected a value" and "I have put the page in the root".

CMS 6

If we think there's some follow up CMS6 work needed here, please create a card and stick it in the CMS6 milestone.

[GuySartorelli]

Presumably, if we want to select the parent of a Page, 0 means that the page is in the root of the site ... it does not have a parent in other words.

If you wanted to have a TreeDropdownField for the ParentID field, and you wanted to use that field to make your record a root-level object, you would indeed select the empty value in the TreeDropdownField, signifying that there is no parent. This would set the field to the value 0.

In this framework, I'm assuming there's no distinction between, "I have not selected a value" and "I have put the page in the root".

I think what you really mean is there's no distinction between "I have not selected a value" and "I have intentionally selected to empty the value of this field". And if the field was already defaulting to the empty value, then yes that's correct.

Hierarchical records are in the root if they have no parent. This is by definition. I don't think it makes sense to change this, since having an item in the root while that item also has a parent would be contradictory.

[sabina-talipova]

PRs are merged. Solution will be automatically tagged.