[Bug] TinyMCEConfig breaks when using option extended_valid_elements on a custom Config.
#11141
Comments
|
Would appreciate any insight from maintainers here on what the intended behaviour is here. Have I been exploiting a bug that I took for a feature and which 4.13.30 fixed, or did 4.13.30 introduce a bug? |
|
If anyone else is seeing this bug, we were able to fix the particular issue with $valid_elements = TinyMCEConfig::get('cms')->getOption('valid_elements');
$valid_elements .= ',small';
TinyMCEConfig::get('CustomConfig')
->setOptions(['extended_valid_elements' => $valid_elements]);The above fix however assumes that only the valid element list is being wiped out. If we assume that any number of config options could be being affected, we need to programatically clone and pass along all of them. See below. $default_options = json_decode(TinyMCEConfig::get('cms')->getAttributes()['data-config'], true);
$createConfig = function ($name) use ($default_options) {
$config = TinyMCEConfig::get($name);
foreach ($default_options as $key => $value) {
if ($key === 'external_plugins') {
$config->enablePlugins($value);
continue;
}
if ($key === 'toolbar') {
foreach ($value as $line => $buttons) {
$config->setButtonsForLine($line + 1, $buttons);
}
continue;
}
$config->setOption($key, $value);
}
return $config;
};
$createConfig('CustomConfig')
->setOptions(['extended_valid_elements' => 'small']); |
|
Expected behaviour is that the configuration set for a given If To check:
|
|
Looks like this is acting as expected. use SilverStripe\Forms\HTMLEditor\TinyMCEConfig;
$config = TinyMCEConfig::get('CustomConfig');
$config->setOptions(['extended_valid_elements' => 'small']);
var_dump([
'valid_elements' => $config->getOption('valid_elements'),
'extended_valid_elements' => $config->getOption('extended_valid_elements'),
]);This outputs In otherwords, previously your config wasn't being validated correctly - but now it is. Any elements that your config do not say are valid.... well, they're not valid. In hindsight if we'd known this would be a problem we would have held off releasing this change until a minor release in CMS 5, but now that it's out there reverting it would just cause problems for people who have already updated and are relying on the now-correct behaviour. |
|
@GuySartorelli this output does not explain the TinyMCE behaviour. Stripping of tags only occurs when extended_valid_elements is set. If you do the following: use SilverStripe\Forms\HTMLEditor\TinyMCEConfig;
$config = TinyMCEConfig::get('CustomConfig');
var_dump([
'valid_elements' => $config->getOption('valid_elements'),
'extended_valid_elements' => $config->getOption('extended_valid_elements'),
]);This outputs The above is true in both 4.13.29 and 4.13.30-42. And in all versions you are able to save If the validator is intended to remove any elements not in the list then it should not be possible to save any such markup after 4.13.30. It is however very much possible until you set It would seem very odd to me if this was the intended behaviour. |
Ahh okay thank you, I must have skipped over that part of the original description, that's good context to have. I'll take a look at that side of it. |
|
Probably also worth noting that a new TinyMCEConfig, w/ no modifications applied will load on the frontend w/ a number of buttons in the toolbar to generate various HTML tags. Headings, lists, tables, etc. If the idea is to wipe the |
|
My current approach (and I passed this by @blueo who implemented the fix you linked to in the original issue description) is to explicitly set some default This retains the current behaviour for new configs that don't have any explicit So it keeps the good part of the current behaviour, and removes the unexpected element. Hopefully. |
|
Or if you'd like to amend yours to be along those lines, that works too 🥳 |
|
Yes, I think there should be a default set of I guess my question would then be what are the default valid elements? Willing to work on a PR but I'm not sure how such defaults have been, or should be arrived at. There is a list defined in silverstripe/admin/_config.php for the The 'cms' config defines quite a few allowed attributes, not just the element type. How was this list arrived at? Presumably we want to align the default valid elements list with the default set of buttons and plugins? Is there any way we can derive the default elements from these other options or otherwise ensure that a change in one is reflected by a change in the other? $config = TinyMCEConfig::get('Custom');
print_r([
'buttons' => $config->getButtons(),
'plugins' => $config->getPlugins()
]);Output Front-end |
|
Most of those questions are about past decisions, and I can't answer any of those since I don't have that context. The one I can answer (sort of) is what the default valid elements should be. And that's simply "whatever is currently allowed when you try to sanitise a new config which hasn't had its valid elements defined" We shouldn't change what buttons or plugins are available. The goal here is to just remove the footgun that setting There is definitely scope to clarify things in docs, though, to your question about that. |
|
Looks like the set of valid elements that TinyMCE uses by default (which is what's applied in that scenario since the PHP sanitiser is effectively skipped) was nice and clearly defined in TinyMCE 3, but in 4+ they don't hard code it. |
|
Attempting to generate the string we need then. Object.keys(tinymce.get()[0].parser.schema.elements).reduce(function (previous, key) { return (previous ? previous + "," : '') + key + '[' + Object.keys(tinymce.get()[0].parser.schema.elements[key].attributes).join('|') + ']'; }, "");Output |
|
Applying the above string w/ Unmodified version includes this for a.attributes.xml "xml": {
"forcedValue": "lang"
}But in the one where we've passed in the string via PHP we get "xml:lang": {},Unmodified has an attributesForced value w/ elements.a "attributesForced": [
{
"name": "xml",
"value": "lang"
}
]Configured w/ php there's no attributes forced but there is "removeEmpty": trueNot sure if these make a big enough difference to be concerned about, nor do I know how to pass in a string that would make the results identical. |
|
just a thought here - the CMS config is arguably what most people will expect as the 'default' - so whilst tinyMCE might have its own defaults perhaps it would be more consistent to stay with the CMS settings (perhaps without the funny extended valid elements)? The generated default above appears very permissive and maybe we should err on the side of 'sensible defaults' for a CMS product? |
|
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. |
How do you decide which defaults are "sensible"? What do we do when we change to a less permissive default set and it breaks someone's site because they're relying on the permissive set that we currently inherit from TinyMCE? That's definitely something we can look at doing in a major release where our opinion of what is sensible is allowed to be a breaking change, but for now we just want to not break things when |
|
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 Thoughts on that? |
Hmm yeah I think it was essentially defaulting to the cms config for all sanitisation as it would get set here: https://github.com/silverstripe/silverstripe-admin/blob/2/code/LeftAndMain.php#L688 |
That's a good point. |
|
All this discussion makes it pretty clear this issue needs a bit more thought put into how we choose to solve it - I've put it into our internal refinement column so we can discuss the options internally |
|
There's an underlying issue here of why setting |
it doesn't. It's just the difference between "something is set and I need to validate" vs "nothing is set so there's nothing to validate". At some point in the past someone made the decision that "nothing set" means "validate nothing" instead of "allow nothing". silverstripe-framework/src/Forms/HTMLEditor/HTMLEditorSanitiser.php Lines 290 to 292 in 528344d That just wasn't happening until now because the wrong config was being used for sanitisation, so there was something set (it just wasn't what the developer had intentionally set) |
|
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 |
|
FYI we have discussed this internally and added some acceptance criteria to the card. The decision was ultimately:
|
|
Linked PRs have been merged and will be released as part of the Silverstripe 5.3 release |
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_elementsare 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.
Add a custom field using this config in Page.php
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_elementsoption 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.
e5eb98c#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
silverstripe/installer(with any code examples you've provided)Acceptance criteria for minor release
valid_elementsis defined forTinyMCEConfigin the settings propertyvalid_elementsdefined for the "cms" config (hardcoded, not dynamic)TinyMCEConfiginstances, a combination of allvalid_elementsandextended_valid_elementsdeclarations are used for sanitisation, and are always respected even if one or both of these settings are emptyTinyMCEConfigand updateextended_valid_elementsif necessary.PRs
The text was updated successfully, but these errors were encountered: