HTMLEditorField doesn't use field assigned configuration for sanitisation

[blueo]

Affected Version

tested on 4.12 and 5

Description

Saving an HTMLEditorField always uses the 'active' HTMLEditorConfig rather than the config defined on the field when sanitising the content for saving into the database.

I would have expected the field to sanitise its content using the config set on the field.

It means it is not possible to have mor than one kind of server side validation. This probably goes un-noticed as the client will prevent adding invalid content in the JS and the default valid elements is fairly permissive. However, it is possibly to bypass the client restrictions (eg with the source button)

Steps to Reproduce

This test will reproduce the issue:

expand code sample

<?php

namespace App\Tests\Forms;

use SilverStripe\Dev\SapphireTest;
use SilverStripe\Forms\HTMLEditor\HTMLEditorConfig;
use SilverStripe\Forms\HTMLEditor\HTMLEditorField;
use SilverStripe\ORM\DataObject;

class HTMLEditorFieldTest extends SapphireTest
{

    public function testHTMLEditorFieldSave(): void
    {
        $defaultValidElements = [
            '@[id|class|style|title|data*]',
            'a[id|rel|dir|tabindex|accesskey|type|name|href|target|title|class]',
            '-strong/-b[class]',
            '-em/-i[class]',
            '-ol[class]',
            '#p[id|dir|class|align|style]',
            '-li[class]',
            'br',
            '-span[class|align|style]',
            '-ul[class]',
            '-h3[id|dir|class|align|style]',
            '-h2[id|dir|class|align|style]',
            'hr[class]',
        ];

        $cmsConfig = HTMLEditorConfig::get('cms');
        $cmsConfig->setOption('valid_elements', implode(',', $defaultValidElements));

        HTMLEditorConfig::set_active_identifier('cms');
        $field = HTMLEditorField::create('Standard');

        $htmlValue = '<p>standard text</p><table><th><tr><td>Header</td></tr></th><tbody>';
        $expectedHtmlString = '<p>standard text</p>Header';
        $field->setValue($htmlValue);

        $dummyObject = new class extends DataObject {
        };

        $field->saveInto($dummyObject);

        $this->assertEquals($expectedHtmlString, $dummyObject->Standard);

        $tableValidElements = [
            'table',
            'thead',
            'tbody',
            'tr',
            'th',
            'td',
        ];
        
        $tableConfig = HTMLEditorConfig::get('table');
        $tableConfig->setOption('valid_elements', implode(',', $tableValidElements));
        // HTMLEditorConfig::set_active_identifier('table');
        $tableField = HTMLEditorField::create('Standard');
        $tableField->setEditorConfig($tableConfig);

        $htmlValue = '<p>standard text</p><table><th><tr><td>Header</td></tr><tbody></tbody></table>';
        $expectedHtmlString = '<p>standard text</p><table class="content-section__table">'
            .'<th></th><tr><td>Header</td></tr><tbody></tbody></table>';
        $tableField->setValue($htmlValue);

        $dummyObject2 = new class extends DataObject {
        };

        $tableField->saveInto($dummyObject2);

        $this->assertEquals($expectedHtmlString, $dummyObject2->Standard);
    }
}

if you uncomment HTMLEditorConfig::set_active_identifier('table'); then the test will pass.

Setting the active identifier works unless you have multiple different configurations within the same form - as there can only be one active config at time.

Save into could instead use the config attached to the field and fall back to the active one?

PRs

• Use field editorconfig when sanitising content #10977

[GuySartorelli]

Thanks for reporting this.

Save into could instead use the config attached to the field and fall back to the active one?

Yup that sounds like the correct course of action. Would you be willing to raise a PR to implement this fix?

[blueo]

Thanks for reporting this.

Save into could instead use the config attached to the field and fall back to the active one?

Yup that sounds like the correct course of action. Would you be willing to raise a PR to implement this fix?

PR attached now :)

[GuySartorelli]

PR was merged. This will be automatically tagged by GitHub Actions.

[GuySartorelli]

This was reverted - see #11141 for details