silverstripe-framework: [Bug] TinyMCEConfig breaks when using option `extended_valid_elements` on a custom Config.

Module version(s) affected

4.13.30-4.13.42

Description

Upon upgrading a site from 4.13.29 to 4.13.41 a bug was introduced in our TinyMCE editor. Nearly all formatting is being stripped on save. It appears as though only html elements listed in extended_valid_elements are being allowed, as if the list is being used to replace the list of valid elements rather than amend it.

How to reproduce

Define an extended valid elements value for a custom TinyMCE config in _config.php.

TinyMCEConfig::get('CustomConfig')
    ->setOptions(['extended_valid_elements' => 'small']);

Add a custom field using this config in Page.php

        private static $db = [
            'RichTextField' => 'HTMLText'
        ];

        public function getCMSFields()
        {
            $fields = parent::getCMSFields();
            $fields->addFieldToTab('Root.Main', HTMLEditorField::create('RichTextField', 'RichTextField', $this->RichTextField, 'CustomConfig'));
            return $fields;
        }

Open a TinyMCE editor in the CMS and enter paragraph breaks, links or any other formatting. On save, all such formatting is stripped.

If the extended_valid_elements option is removed, or set to an empty string, the issue does not occur.

If you set the value to 'h3', you will be able to save h3 elements, but nothing else.

Possible Solution

This bug does not appear in 4.13.29, but it appears in all versions after 4.13.30

The change in 4.13.30 appears to cause custom configs to be built from an empty config, instead of bootstrapping the defaults as it did previously.

https://github.com/silverstripe/silverstripe-framework/commit/e5eb98cc3491785cbae17bb53be0be05fd5a6f42#diff-816b9855682fea334353bdfeb9b733211672205ec14351283df7dc1289f89037L148

Reverting this change would fix my issue, but I’m not clear what problem it was trying to solve. In any case this was a breaking change in config behaviour.

Additional Context

No response

Validations

Check that there isn’t already an issue that reports the same bug
Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you’ve provided)

Acceptance criteria for minor release

this bugfix is reimplemented
sanitisation for HTML content is not skipped even when no valid elements are declared (see this code)
A default valid_elements is defined for TinyMCEConfig in the settings property
- The default value is equal to the valid_elements defined for the “cms” config (hardcoded, not dynamic)
For all TinyMCEConfig instances, a combination of all valid_elements and extended_valid_elements declarations are used for sanitisation, and are always respected even if one or both of these settings are empty
This change is highlighted in the changelog, with a call to action to check custom TinyMCEConfig and update extended_valid_elements if necessary.

PRs

About this issue

Original URL
State: closed
Created 5 months ago
Comments: 26 (17 by maintainers)

Most upvoted comments

So we definitely need to rip that condition out to get the valid elements respected properly. But we also have to add some default valid elements so we’re not just stripping everyone’s HTML out of their not-quite-configured-enough WYSIWYGs

GuySartorelli on Feb 21, 2024

Although… actually. Now I’m thinking (which is always dangerous)… in 4.13.29 it wouldn’t have been the full TinyMCE set would it? It would have been the “current config” set I think. So maybe we can just take the set that’d defined on cms and call it a default?

Thoughts on that?

GuySartorelli on Feb 21, 2024

That’s the direction I was going with some of my questions but if the goal here is to restore the functionality that broke in v4.13.30 and not disrupt anyone else’s configuration which may for some reason depend on the permissive settings the TinyMCE defaults make sense. Restoring what broke is a different task than revisiting the expectations, I suppose.

davejtoews on Feb 21, 2024