FIX Improve non-inline validation

emteknetnz commented 4 months ago

Issue https://github.com/silverstripe/silverstripe-elemental/issues/1155

Need to merge https://github.com/silverstripe/silverstripe-framework/pull/11213 first to get the has_one RequiredFields validation working correctly and https://github.com/silverstripe/silverstripe-frameworktest/pull/172 to get behat to pass.

There is a mutli-pr CI run linked on the issue

Note that this PR does not touch inline-saving

Scenarios:

DataObject validate() addFieldError()
DataObject validate() arrError()
UrlField validation
RequiredFields on default Title field
has_one relation to File and RequiredFields
has_one relation to SiteTree and RequiredFields

Non-inline editable block:

Already worked
Already worked
Already worked
Fixed in this PR - by default it's a composite field and validation works but doesn't show message on front-end
Already worked
Fixed in framework PR

Inline-editable block page save

Already worked - UX is bad not fixed in this PR - issue
Already worked - UX is bad not fixed in this PR - issue
Fixed in this PR
Fixed in this PR
Already worked
Fixed in framework PR

I used the following blocks to tests these scenarios

<?php

use DNADesign\Elemental\Models\BaseElement;
use SilverStripe\Forms\CompositeValidator;
use SilverStripe\Forms\RequiredFields;
use SilverStripe\Forms\UrlField;
use SilverStripe\Assets\Image;
use SilverStripe\CMS\Model\SiteTree;

class MyBlock extends BaseElement
{
    private static $db = [
        'MyField' => 'Varchar',
        'MyHTML' => 'HTMLText',
        'MyUrl' => 'Varchar',
    ];

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

    private static $table_name = 'MyBlock';

    private static $singular_name = 'My Block';

    private static $plural_name = 'My Blocks';

    private static $description = 'This is my block';

    private static $icon = 'font-icon-block-content';

    public function getType()
    {
        return 'My Block';
    }

    public function validate()
    {
        $result = parent::validate();
        if ($this->Title == 'x') {
            $result->addFieldError('Title', 'Title cannot be x', 'validation');
        }
        if ($this->MyField == 'x') {
            $result->addFieldError('MyField', 'MyField cannot be x', 'validation');
        }
        $c = trim(strip_tags($this->MyHTML ?? ''));
        if ($c == 'x') {
            $result->addFieldError('MyHTML', 'MyHTML cannot be x', 'validation');
        }
        if ($this->MyField == 'z' && $c == 'z') {
            $result->addError('This is a general error message');
        }
        return $result;
    }

    public function getCMSCompositeValidator(): CompositeValidator
    {
        return CompositeValidator::create([RequiredFields::create(['Title', 'MyImage', 'MyPageID'])]);
    }

    public function getCMSFields()
    {
        $fields = parent::getCMSFields();
        $fields->dataFieldByName('MyHTML')->setRows(5);
        $fields->removeByName('MyUrl');
        // URLfield contains its own validate() methods
        $fields->addFieldToTab('Root.Main', UrlField::create('MyUrl', 'My URL'));
        return $fields;
    }
}

<?php

class MyNonInlineBlock extends MyBlock
{
    private static $inline_editable = false;

    private static $table_name = 'MyNonInlineBlock';

    private static $singular_name = 'My non-inline Block';

    private static $plural_name = 'My non-inline Blocks';

    private static $description = 'This is my non-inline block';

    private static $icon = 'font-icon-block-content';

    public function getType()
    {
        return 'My non-inline Block';
    }
}

emteknetnz commented 4 months ago

If I save the page without expanding the block, none of the validation seems to happen. e.g. with your test blocks from the description, I add the block and immediately save the page - it should trigger validation errors for the missing required fields but it doesn't.

That would be hard to solve.

In the case of inline blocks, currently if they haven't been opened and the page is saved, then then because the child FormBuilder form hasn't been rendered then the element data will not be sent as part of the page form submission. However if the inline-element is opened then page save will submit element data with the rest of the page, and closing the element then saving the page will also send the element data. So to solve this we'd need to render the FormBuilder forms for ALL unopened elements on page save

In the case of non-inline blocks, I really don't know what can be done since their forms are never rendered on the page and and thus their data is never sent.

The fact this doesn't work doesn't actually matter given the AC is Saving/publishing a page will trigger validation on its child block if they are in a a dirty state. - the elements in question aren't dirty, they're just created in an invalid state.

We could split this off as a seperate card, though I honestly don't know how or even if we should attempt a fix here