U4-8724 - tinyMceConfig.config customConfig bool not correct deserialized

Created by Marcel van Helmont 12 Jul 2016, 18:12:26 Updated by Sebastiaan Janssen 13 Apr 2018, 12:33:47

I have set the following customConfig items to false convert_urls and remove_script_host.

What did you expect to happen? That tinymce does not convert the urls when creating a link to a absolute url.

What actually happened? The values are deserialized as string instead of bools in the javascript code so tinymce does not understand them and ignores them. When hardcoding the values tinymce works as expected.

Comments

Shannon Deminick 13 Sep 2016, 08:52:19

PR: https://github.com/umbraco/Umbraco-CMS/pull/1381


Andreas Emtinger 19 Mar 2017, 20:06:55

I need a solution for this to.

You can "hack" a fix: if (val.detectIsJson()) else { if(val === 'true') { tinyMceConfig.customConfig[i] = true; } else if (val === 'false') { tinyMceConfig.customConfig[i] = false; } }


Ronald Barendse 01 Feb 2018, 13:10:49

@Shandem Is it possible to include this fix together with issue U4-10191? They're both breaking changes, so makes sense to fix this in version 7.8!


Sebastiaan Janssen 16 Mar 2018, 15:36:44

Fixed in https://github.com/umbraco/Umbraco-CMS/commit/e3e425695f724cbbf1b25e2dfe97d90d8cdcd9f2


Ronald Barendse 28 Mar 2018, 22:11:59

@sebastiaan Why is the hack used as fix and not the correct PR of @Shandem?

If the CustomConfig property is changed to IDictionary<string, object>, all values are converted to the correct type, so this would also work for types other than booleans. That would also make merging with the baseline config a lot easier, especially if you use deep copy e.g. $.extend(true, baseLineConfigObj, tinyMceConfig.customConfig); (using jQuery).

Deep copies in AngularJS can be done using angular.merge, but the [documentation|https://docs.angularjs.org/api/ng/function/angular.merge] recommends to use the lodash method instead. I only found a reference to lodash in the gulpfile.js of Umbraco.Web.UI.Client, but although _.VERSION does return 1.7.0 in Umbraco 7.10, it doesn't seem to be included (at least not the _.merge function)... So using the [jQuery|https://api.jquery.com/jquery.extend/] $.extend should be the best option for deep copies!


Sebastiaan Janssen 04 Apr 2018, 09:38:00

@rgmbarendse We can't just change the IDictionary, it's a breaking change requiring everyone who uses this object to recompile.

Could be good to do this for v8 where a recompile is needed anyway. Care to submit a PR on the dev-v8 branch?


Ronald Barendse 10 Apr 2018, 15:21:15

@sebastiaan Is such a breaking change only allowed on v8 or could it be planned for the next minor version?

And why aren't all values parsed using JSON.parse? The current code checks if it's JSON (val.detectIsJson()), but that only checks whether it's an array or object. The values "true", "false", "1", etc. are also valid JSON tokens... That would make the JavaScript code a lot cleaner, because it could just parse the string values from the customConfig and merge any arrays with the baseLineConfigObj.

I'll see if I can create a PR for this!


Sebastiaan Janssen 13 Apr 2018, 12:33:47

@rgmbarendse I think it would be great to fix it for v8, before that version it's an unimportant change and we wouldn't want to introduce a breaking change for something trivial that basically works fine right now, even though the code obviously is not ideal.

Not sure why JSON.parse is not used, could it be a security issue? No idea :-)


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Very Easy

Category: Editor

Backwards Compatible: True

Fix Submitted: Pull request, Inline code

Affected versions: 7.4.3

Due in version: 7.10.0

Sprint:

Story Points: 1

Cycle: 7