Fix iframe removal in TinyMCE

[sprankhub]

Description (*)

The TinyMCE WYSIWYG editor removes iframes when the editor is toggled.

Related Pull Requests

n/a

Fixed Issues (if relevant)

n/a

Manual testing scenarios (*)

1. Create a new CMS page.
2. Disable the WYSIWYG editor.
3. Insert <iframe width="560" height="315" src="https://www.youtube.com/embed/JvBWJ6Lm9MU" frameborder="0" allow="accelerometer; autoplay; encrypted-media; gyroscope; picture-in-picture" allowfullscreen></iframe> in the content of the CMS page.
4. Enable the WYSIWYG editor.
5. Save the CMS page.
6. Open the CMS page in the frontend.

Expected behaviour: The iframe is displayed.
Actual behaviour: The iframe is not displayed at all.

Questions or comments

See https://magento.stackexchange.com/a/312285/142.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)

[sprankhub]

The failed labeler task seems to be unrelated to my changes.

[tmotyl]

How does the non minified change looks like?
How did you minify it? Or did you just provide updated tinymce version?

[sprankhub]

There is no non-minified change @tmotyl. This PR fixes an error Magento introduced in the minified file.They use JSON in the minified file, although JSON has been changed to h in the minified version. See also the linked StackExchange answer https://magento.stackexchange.com/a/312285/142.

[tmotyl]

My findings:
The non minified version is here:
/js/tiny_mce/plugins/media/editor_plugin_src.js
Both minified version and non-minified got an update in 1.9.4.3
The change in non-minified version looks like:

			try {
				data = JSON.parse(data);
			} catch (e) {
				return;
			}

before 1.9.4.3 this file was identical to tinymce v 3.5.11 https://raw.githubusercontent.com/tinymce/tinymce/3.5.11/jscripts/tiny_mce/plugins/media/editor_plugin_src.js
I couldn't find official minified version of this file.

[sprankhub]

I know @tmotyl. Did you look at the StackExchange post? Did you compare the old minified version and my updated one? Again: JSON is used in the minified version although it is called h in the minified version.

[tmotyl]

@sprankhub I'm digging through it.
Its quite time consuming if the PR is missing context like that. Please note that when we merge stuff we need to make sure everything is ok. And doing manual changes to minified files is a risky thing and was used as an attack to some other products. So I would love to have a tool in openmage to generate these minified files out of the non-minified version, so anybody (even the CI tool) can verify that minified version shipped is correct.

But until we get there, lets manually check stuff.

[sprankhub]

If you actually had a look at the stuff I mentioned and referenced, it should not be that hard to get. You can simply format the file and format my changed file to see what I changed. Not a nice experience as a contributor currently TBH. Anyway, I will try to give you even more context:

The issue has been introduced in 1.9.4.3:

• Change of the minified file: OpenMage/magento-mirror@99b772f#diff-12926fb7afe700adb97673ce44b968c1
• Change of the non-minified file: OpenMage/magento-mirror@99b772f#diff-c8b7c4a0c088d5266c073fcc19679dac

I formatted the minified file and created a diff. It looks like this:

$ diff editor_plugin.js editor_plugin-new.js                                                                                                               14:22:04
241c241,245
<             G = h.parse(G);
---
>             try {
>                 G = JSON.parse(G);
>             } catch (e) {
>                 return;
>             }

As you can see, the idea was to wrap the call with a try-catch-block. However, instead of just doing this, the minified variable h has also been replaced with JSON. Since JSON is not available in the minified file, the call JSON.parse(G) fails. Unfortunately, it fails silently due to the try-catch-block.

[tmotyl]

@sprankhub please dont get me wrong. I appreciate the PR and work you put to get this fixed.
The important thing for me as a maintainer is to make sure we dont break sth with rushing some patch without a proper due dilligence.
This includes not only me understanding the change and problem it solves but also making sure the findings are written down here on github.
Stackoverflow is nice but it evolves over time and after some months the SO thread can look quite different then it looks now. So we can loose valuable info if we dont specify stuff here.

The real bottleneck in here (and with probably most OSS projects) is manpower on reviewing testing and merging patches.
In practice it boils down to the simple rule that the better the patch is explained (the less time reviewer needs to spend on it) the faster it gets merged.
This is why I asked questions and also wrote down my findings, os others can have easier job on reviewing it.

Thanks for your quick answers. Its all clear now, the change looks good by reading, but i would like to test it before I vote on it.

[sreichel]

The failed labeler task seems to be unrelated to my changes.

Its unrelated. Waiting for #1101

[tmotyl]

reviewed and tested

[colinmollenhour]

Thanks for the PR and the thorough description, @sprankhub!