-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[5.0] Upgrade TinyMCE to version 6 #37633
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@laoneo check Digital-Peak#20 |
|
Oh boy, there are quite some changes needed, eg joomla-cms/plugins/editors/tinymce/src/PluginTraits/KnownButtons.php Lines 44 to 47 in b07d1e5
Need to be renamed to
and there are more... (will try to do some PR after dinner) |
|
So I have just to rename the variables in the mentioned file, is there no params upgrade neded during Joomla update? |
|
I think this is what you are looking for https://github.com/joomla/joomla-cms/blob/4.1-dev/build/build-modules-js/init/exemptions/tinymce.es6.js |
|
When I do there the changes with the new folders, they have no effect. |
|
@dgrammatiko can you help here regarding the build script to copy the models folder? |
|
Sure: After
add await mkdir(join(itemvendorPath, 'models'), { mode: 0o755 });After
add await copyAllFiles('models', 'tinymce', 'models'); |
|
Strange, I'm pretty sure that I tried this. Thanks, it worked... |
|
What would be the best way to migrate the options which got renamed? |
|
@dgrammatiko thanks. Putting this now as ready for testing. |
|
In my opinion the upgrade is a b/c break and couldn't go into 4.x, at least it would break one of my extension. |
|
Could you explain what breaks in your extension please |
|
In my case I only checked that I check the version major number ;-) which yeah not the best way to detect features but it's legacy code which was needed to detect tinymce 3/4 and JCE and now 5 and JCE. But I only do simple things (getcontent setcontent listen to focus/blur event). looking at https://www.tiny.cloud/docs/tinymce/6/migration-from-5x/ it could easily break some plugins doing more then me, having a tinycme plugins maybe. Beside that we have a strict b/c policy on javascript code, no b/c breaks,
|
|
My 2c is that its not a valid reason not to upgrade |
|
@HLeithner pretty sure this BC policy is only for our own scripts and not for external ones. Beside that a 3rd party extension should only interact with the public API from core and not directly with the editor implementation itself. If it does, then it should be prepared for this situation. |
our b/c promise is not a valid reason? what is then a valid reason? I would really like to have a more modern policy but we only have joomla/rfc#29 which is stalled because nobody seems to be interested. @laoneo actually I should be prepared for NO b/c break in a minor update, doesn't matter what. that's the reason for semver. I know we don't care to much about this because it's hard to fulfill the expectation. |
Not true - several people of commented and contributed. At the end of the day the rfc process is something only the production department can decide on so its the pd thats not interested. |
|
If you use a none public API (everything what doesn't go under https://github.com/joomla/joomla-cms/blob/4.2-dev/build/media_source/system/js/core.es6.js#L66-L101) then I hope your code will break. Because you are abusing the code base. This is not what backwards compatibility is about and has nothing to do with semver. You really should change your code to use the "official" editors API of core, because there we have a policy. And if the API doesn't have what you need, then make a pr. To finalize this, honestly I do not care if we wait for 5. |
|
@laoneo we are a CMS the end user doesn't care if we think something in this folder or that folder is important for b/c, we provide a packages as one download, and as long as we have a user base nothing should break except we explicitly say it (in which form doesn't matter, we do it in semantic versioning). That's not only important for users who blindly update the joomla installation. Also for extension developer who could expect that there code still works. You are right we don't promise 3rd party dependencies like guzzle, but tinymce is a first class dependency and our main editor which is more important then anything else in a CMS, if it breaks or changes the html output it saves then you break a user website (100k). So if it's ok for you I would like to wait for 5 to merge this. (I hopefully find a way for upgrading PR to PSR-12 soon) |
|
@HLeithner you might have a point about not upgrading to major version in a minor release, but not in this case:
|
|
I think, staying on 5 in 4 would be ok as 5.10 is not EOL https://www.tiny.cloud/docs/general-configuration-guide/system-requirements. We can postpone it to 5, no problem. Despite, I would merge it very early when 4.3 starts. I did this only to test if this pr would cause an issue as mentioned by Brian here. |
https://www.tiny.cloud/docs/general-configuration-guide/system-requirements/ 5.10.5 was released 5 days ago and the EOL is not defined yet. If we trust the tiny documentation.
What stupid things do you mean?
|
* Use the new option names * Update KnownButtons.php * Update ToolbarPresets.php
|
What is the situation now with b/c. As I understand it j5 will not have b/c breaks |
|
@brianteeman no, it doesn't break thing that have been deprecated in 4.x, also js is not part of the b/c promise there will be b/c breaks which will be minimal and should not effect most of the 3rd party components. |
|
I will merge this without out tests because I expect that this will be tested enough in the development lifecycle. |
|
thanks @laoneo |
|
It breaks things that are not deprecated. And why is js not part of the b/c promise. As a regular user I wouldn't care or even know the subtle difference between PHP and js |
|
/me confused as the policy says 6.1.2 JavaScript All JavaScript functions and classes that are not flagged as private. |
|
True I forget that the only thing that has a useful b/c promise is for what every reason the javascript stuff... any way I will do a Production motion for the update since the b/c breaks are not to big for the editor and 3rd party extensions should work with more then one editor. thanks for pointing this out. |
|
Honestly I doubt that this applies for dependencies and is only for core code. Otherwise it would not make sense as when we deprecate for a new major version of a library when it gets merged in three years the next one will very likely be out already. So we have to do the same game again or what? |
|
That's why I raised it as an issue. And tiny can be and is extended by extensions available on the JED so it's not exactly the same as other external libraries. |
Pull Request for Issue #37596.
Summary of Changes
Updates TinyMCE to version 6. This is an early draft, so it needs extensive testing. So please take it with caution.
What has changed can be found here https://www.tiny.cloud/docs/tinymce/6/migration-from-5x
As version 6 doesn't contain anymore the
hr,paste,printplugins. So I'm just onloading it, before merge this must be removed from the params.@dgrammatiko probably something you want to look at too
Testing Instructions
Play around with different settings in the TinyMCE editor plugin and test them by opening an article form.
Actual result BEFORE applying this Pull Request
All works.
Expected result AFTER applying this Pull Request
All should work and no console errors.