Upgraded TinyMCE to 6.7.0 #3220

fballiano · 2023-04-25T16:30:59Z

the WYSIWYG editor shipped with OM is TinyMCE 3.something, it is extremely old and outdated and has at least one known vulnerability.

example:

what works (everything):

loading/unloading the editor
localization
openmage-variables widget
openmage-widgets widget
file/image upload

note:

there's a small modification to prototypejs in this PR, because the default one is not compatible with the new tinymce

addison74 · 2023-04-25T17:25:07Z

I know that some parts of OM are old and not updated, but they work. However, if it can be replaced with the latest version in next branch, I will approve it after intensive testing.

Please note over 1k issues reported in TinyMCE repository. Here we didn't really face any reports related to this editor.

fballiano · 2023-04-25T17:26:45Z

@colinmollenhour knows why this PR is here, let's just say that outdates software may bring some side effects.

addison74 · 2023-04-27T21:18:57Z

Let's make a visual comparison between what exists now in OpenMage and the proposed version. Even if the current one looks old, by far it has all the intuitive features for an editor. I tried to create a list just pressing a button in version 6 and I didn't succeed. It is probably hidden somewhere but does not appear in the menus.

fballiano · 2023-04-27T21:20:28Z

It’s not worth testing it now, it’s extremely far from being usable

addison74 · 2023-04-27T21:21:53Z

The browser console must be followed. I get errors as soon as I open the editor. If I enter html tags when I press the hide/show button, the editor no longer retains the changes, errors appear in the console.

empiricompany · 2023-09-06T11:06:00Z

for me, it's perfect. I'm already using it with several clients, and they're really satisfied about it

colinmollenhour · 2023-09-06T15:44:50Z

A test suite would be great (and Cypress is an excellent choice) but to me that's a new feature so I think it should not become a blocker for this task. My only stipulations would be that if there is a test suite it must run automated via Github Actions and be easy to setup and run locally.

@pquerner I propose you create a new branch based on the one in this PR and add the Cypress test suite there and if it is done before this branch is ready to merge, great, they can both be merged together. However, if it is not ready in time, no problem, it can be merged later.

It appears TinyMCE doesn't really have their own test suite, but still I think just making sure the basic functionality (turn WYSIWYG off/on) and the OM add-ons (images, variables, widgets) work as expected would be sufficient. If there are changes between TinyMCE 3 and 6 for normal content they will probably not become apparent until it gets broader use and not every difference will indicate there is a regression as we may decide the new behavior is preferred anyway.

So is PHPStan the only blocker now for merging (edit: aside from approval reviews of course)?

fballiano · 2023-09-06T15:47:02Z

@colinmollenhour If I'm not mistaken it's a PHPStan bug with PHP 8.1, nothing to so with this PR, the whole next branch shows this error (regardless of this PR which is not merged yet)

colinmollenhour · 2023-09-06T22:19:39Z

Attempting to fix PHPStan with #3498

fballiano · 2023-09-10T20:57:31Z

all green checks again ;-)

fballiano · 2023-09-17T10:21:21Z

It is a bit frustrating not to get a single review for such a big effort mostly @empiricompany put into this.

we prefer keeping a >10 years old component with a published vulnerability instead of this.

empiricompany · 2023-09-17T10:45:24Z

It is a bit frustrating not to get a single review for such a big effort mostly @empiricompany put into this.

we prefer keeping a >10 years old component with a published vulnerability instead of this.

yes we should merge it ASAP this PR is perfect, i have dedicated a lot of time to it and also fixed all minor things ..
IMHO in general, we should speed up the development of OpenMage and move forward quickly; otherwise, we will miss the opportunity to become a independent and affidable e-commerce platform

colinmollenhour

Awesome work, guys! I've tested in Chrome just clicking around, adding variables, widgets and images and saving an reloading and switching WYSIWYG off and on.

I can't find any issues with it, works beautifully!

pquerner · 2023-09-19T06:51:27Z

I've started the cypress tests here btw: https://github.com/pquerner/openmage_cypress_tests
In case anyone wants to take a look.

I think I write a few tests for widget and magento-only functionality and that should be it.

Why its own repo?
I think such tests shouldnt be shipped with the normal openmage (test) code.
Also its very hard to make this automatic, since a instance must be up and running and configured to be testable - not sure how you would do that automatically - other than have a instance ready to go to (at all times).

fballiano · 2023-09-24T11:12:11Z

@kiatng @elidrissidev @addison74 @Flyingmana @tmotyl sorry to bother you but we would need to get some review on this PR.

notes:

there's an public vulnerability on the version of tinymce that's currently in OM
this PR is for main and I'll backport it as soon as it gets merged

Flyingmana · 2023-09-24T13:28:40Z

I will need maybe 2 days to fully review the changes.

An advise. If something is of importance for security, keep the amount of changed lines and files low
No codestyle changes.
If the framework/library changes its used variable name, link the previous one to it so you dont need to rename all.

addison74 · 2023-09-24T16:05:35Z

I've been testing it in production for about 2 weeks and I haven't identified any problems. I haven't approved it yet because I have about 5% left to be fully convinced. I also received quite useful feedback. There is room for other changes, but they can be made in time and in separate PRs. In general, these changes are TinyMCE features for which the API must be read in full.

fballiano · 2023-09-24T21:58:03Z

about workflow failing after #3536, I think #3542 would fix. anyway they don't fail because of this PR, it's the next branch failing, not the PR.

Flyingmana · 2023-09-30T20:48:33Z

These files still use the tinyMCE writing

app/design/adminhtml/default/default/template/widget/form/element.phtml
js/mage/adminhtml/browser.js
js/mage/adminhtml/wysiwyg/widget.js

Flyingmana · 2023-09-24T13:31:34Z

js/mage/adminhtml/variables.js

@@ -112,7 +112,7 @@ var Variables = {
    }
 };

-MagentovariablePlugin = {
+OpenmagevariablePlugin = {


was it necessary to mix a renaming with a security related update?

actually the naming changes were done to make it easier to find something like what you highlight in #3220 (comment) actually I'll check them out right now, thank you for noticing!!!

Flyingmana · 2023-09-30T20:42:57Z

app/code/core/Mage/Widget/controllers/Adminhtml/WidgetController.php

@@ -34,10 +34,11 @@ public function indexAction()
    {
        // save extra params for widgets insertion form
        $skipped = $this->getRequest()->getParam('skip_widgets');
-        $skipped = Mage::getSingleton('widget/widget_config')->decodeWidgetsFromQuery($skipped);
+        if (is_string($skipped)) {


for type safety you should put $skipped as an empty array for the else case.

in order not to drag this PR any longer I'd address it in a separate PR ASAP :-)

Flyingmana · 2023-09-30T20:53:51Z

js/mage/adminhtml/browser.js

-         } else {
-             return null;
-         }
+    getMediaBrowserCallback: function() {


Why did we change method names of our own class here? isnt this a BC Break?

well initially my idea was to have it in the next branch because, since all tinymce's variables/objects have different names between v3 and v6... everything is BC so having better names for our method didn't make any change.

later it was sad to have it into main which changes things a bit but at the end of the day, all of tinymce's scripts are still incompatible so... also if we don't change our naming... we don't get much compatibility anyway.

what do you think?

Flyingmana · 2023-09-30T21:07:17Z

js/mage/adminhtml/wysiwyg/tinymce/setup.js

+    },
+
+
+    getSettings: function (mode) {


We can do this later in a separate PR

Flyingmana · 2023-09-30T21:24:29Z

None of the things I commented I would count as blocking because of the security relevance, but maybe we should look again at them and if needed fix them later then

fballiano · 2023-10-01T16:25:45Z

is it ok if I merge and port to main now? then work on @Flyingmana's notes in a PR on the main branch?

addison74 · 2023-10-01T16:36:08Z

I looked over this PR this weekend too. There are still changes to be made, but they are not a priority because all are related to with UI/UX. The editor is functional and ready for production.

Could we extract the version we are replacing in a repository? If someone is not happy with this new version and wants the previous one, he can replace it on his own responsibility.

I agree with porting to the main branch and we continue the work from there. All future PRs will go to next too.

fballiano · 2023-10-01T16:45:05Z

@addison74 create something like an extension to return to tinymce v3 is complex because it's like 500 files, with phtml in many different places, I'm sorry but I really wouldn't do that, it's too much work and I can't dedicate that time. anybody could do it if somebody feels the need for that, although highly un-suggested because of the security implications.

fballiano · 2023-10-02T12:40:12Z

oki guys it's time, I'll merge and port to main so we can continue there, I feel that after ZF1F this is another major stepstone!

Co-authored-by: Tony <[email protected]> Co-authored-by: Tony <[email protected]>

colinmollenhour · 2023-10-03T14:54:05Z

Nice work, @fballiano! Also thanks for everyone else's testing and feedback!

First commit

3683b2d

fballiano added 2 commits April 25, 2023 17:58

First test converting the plugins

c8fe0b5

Coverted part of the "variable" plugin

c87b2cd

github-actions bot added the Component: Core Relates to Mage_Core label Apr 25, 2023

fballiano added 2 commits April 25, 2023 18:14

typo

138fda1

typo

a6ccefd

github-actions bot added the Component: Oauth Relates to Mage_Oauth label Apr 25, 2023

renames plugin javascript files

38e3527

something starts to work

51f3fd9

fballiano added 3 commits May 9, 2023 14:03

Merge branch 'next' into tinymce6

15c441e

upgraded to 6.4.2

938aff0

Merge branch 'tinymce6' of github.com:fballiano/openmage into tinymce6

56908d5

fballiano changed the title ~~Upgraded TinyMCE to 6.4.1~~ Upgraded TinyMCE to 6.4.2 May 9, 2023

fballiano added 3 commits May 9, 2023 16:00

Fixed showing of plugin buttons

828ef09

removed tinyMceEditors hash from prototypejs

d43ca5c

close window image was ugly

318ff9f

github-actions bot added the Template : base Relates to base template label May 9, 2023

file selector starts to work

437b4da

Merge branch 'next' into tinymce6

58d063b

Merge branch 'next' into tinymce6

d3554e3

colinmollenhour approved these changes Sep 18, 2023

View reviewed changes

Merge branch 'next' into tinymce6

1f2efb0

Flyingmana approved these changes Sep 30, 2023

View reviewed changes

Merge branch 'next' into tinymce6

288bd56

addison74 approved these changes Oct 1, 2023

View reviewed changes

fballiano merged commit 9c7e07e into OpenMage:next Oct 2, 2023

fballiano deleted the tinymce6 branch October 2, 2023 12:40

fballiano added a commit that referenced this pull request Oct 2, 2023

Upgraded TinyMCE to 6.7.0 (#3220)

51f5bb5

Co-authored-by: Tony <[email protected]> Co-authored-by: Tony <[email protected]>

fballiano mentioned this pull request Oct 2, 2023

Minor fixes for TinyMCE v6 #3570

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgraded TinyMCE to 6.7.0 #3220

Upgraded TinyMCE to 6.7.0 #3220

fballiano commented Apr 25, 2023 •

edited

Loading

addison74 commented Apr 25, 2023

fballiano commented Apr 25, 2023

addison74 commented Apr 27, 2023

fballiano commented Apr 27, 2023

addison74 commented Apr 27, 2023

empiricompany commented Sep 6, 2023

colinmollenhour commented Sep 6, 2023 •

edited

Loading

fballiano commented Sep 6, 2023

colinmollenhour commented Sep 6, 2023

fballiano commented Sep 10, 2023

fballiano commented Sep 17, 2023

empiricompany commented Sep 17, 2023

colinmollenhour left a comment

pquerner commented Sep 19, 2023 •

edited

Loading

fballiano commented Sep 24, 2023

Flyingmana commented Sep 24, 2023

addison74 commented Sep 24, 2023

fballiano commented Sep 24, 2023

Flyingmana commented Sep 30, 2023

Flyingmana Sep 24, 2023

fballiano Oct 1, 2023

Flyingmana Sep 30, 2023

fballiano Oct 1, 2023

Flyingmana Sep 30, 2023

fballiano Oct 1, 2023

Flyingmana Sep 30, 2023

Flyingmana commented Sep 30, 2023

fballiano commented Oct 1, 2023

addison74 commented Oct 1, 2023

fballiano commented Oct 1, 2023

fballiano commented Oct 2, 2023

colinmollenhour commented Oct 3, 2023

Upgraded TinyMCE to 6.7.0 #3220

Upgraded TinyMCE to 6.7.0 #3220

Conversation

fballiano commented Apr 25, 2023 • edited Loading

addison74 commented Apr 25, 2023

fballiano commented Apr 25, 2023

addison74 commented Apr 27, 2023

fballiano commented Apr 27, 2023

addison74 commented Apr 27, 2023

empiricompany commented Sep 6, 2023

colinmollenhour commented Sep 6, 2023 • edited Loading

fballiano commented Sep 6, 2023

colinmollenhour commented Sep 6, 2023

fballiano commented Sep 10, 2023

fballiano commented Sep 17, 2023

empiricompany commented Sep 17, 2023

colinmollenhour left a comment

Choose a reason for hiding this comment

pquerner commented Sep 19, 2023 • edited Loading

fballiano commented Sep 24, 2023

Flyingmana commented Sep 24, 2023

addison74 commented Sep 24, 2023

fballiano commented Sep 24, 2023

Flyingmana commented Sep 30, 2023

Flyingmana Sep 24, 2023

Choose a reason for hiding this comment

fballiano Oct 1, 2023

Choose a reason for hiding this comment

Flyingmana Sep 30, 2023

Choose a reason for hiding this comment

fballiano Oct 1, 2023

Choose a reason for hiding this comment

Flyingmana Sep 30, 2023

Choose a reason for hiding this comment

fballiano Oct 1, 2023

Choose a reason for hiding this comment

Flyingmana Sep 30, 2023

Choose a reason for hiding this comment

Flyingmana commented Sep 30, 2023

fballiano commented Oct 1, 2023

addison74 commented Oct 1, 2023

fballiano commented Oct 1, 2023

fballiano commented Oct 2, 2023

colinmollenhour commented Oct 3, 2023

fballiano commented Apr 25, 2023 •

edited

Loading

colinmollenhour commented Sep 6, 2023 •

edited

Loading

pquerner commented Sep 19, 2023 •

edited

Loading