Skip to content
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

[5.0] Tinymce v6 #40622

Closed
brianteeman opened this issue May 18, 2023 · 38 comments
Closed

[5.0] Tinymce v6 #40622

brianteeman opened this issue May 18, 2023 · 38 comments

Comments

@brianteeman
Copy link
Contributor

Joomla 5 is using tinymce v6 and there are a few breaking changes that need to be reviewed and addressed.

https://www.tiny.cloud/docs/tinymce/6/6.0-release-notes-core-changes/

For example the dropdown formatselect is now blocks so the presets need to be updated accordingly. Also need to consider what happens with existing saved toolbars as without action they will be missing the dropdown on upgrade.

@dgrammatiko
Copy link
Contributor

dgrammatiko commented May 18, 2023

You're right. Also it seems the code committed into 5.0 with #37633 is missing parts of Digital-Peak#21

This issue deserves a Release Blocker tag!

@HLeithner
Copy link
Member

Would you be so nice and create a pr @dgrammatiko

@dgrammatiko
Copy link
Contributor

@HLeithner probably @richard67 is the right person, as the breaking parts either have to be patched in the update sql files or in a fn in the admin script.php

@HLeithner
Copy link
Member

I mean the missing part from the pr you mentioned?

@dgrammatiko
Copy link
Contributor

With #40626 I tried to patch both the missing parts and introduce some runtime fixes for toolbar/menus.

@HLeithner
Copy link
Member

thanks, is more needed for conversation between j4 and j5?

@dgrammatiko
Copy link
Contributor

Yes, according to https://www.tiny.cloud/docs/tinymce/6/migration-from-5x/#formatting-user-interface-name-changes there are more runtime replacements needed. Actually someone should revisit their docs and check what's missing

@dgrammatiko
Copy link
Contributor

FWIW the runtime changes (the str_replace) is not ideal. The changes should be applied to the DB, so @richard67 maybe something for you?

@richard67
Copy link
Member

FWIW the runtime changes (the str_replace) is not ideal. The changes should be applied to the DB, so @richard67 maybe something for you?

@dgrammatiko I would do that only if it is absolutely necessary. Last time we have done such stuff was not good, see the fix with #40535 . The better way is to use the right defaults when reading parameters which have not been saved yet, see e.g. #40544 . I can have a closer look on weekend if you specify here what is needed, or contact me on Mattermost, but not today.

@HLeithner
Copy link
Member

the thing you mentioned @richard67 was a simple str_replace on a json object what shouldn't be done ;-)
but I would suggest a php script for migartion if it's more complex or you sql json functions for the manupulation.
But keeping the b/c code would be for ever, since you have to save the tinymce plugin to have correct values right?

@dgrammatiko
Copy link
Contributor

FWIW there should be a function on the script.php where the json of the params for extensions could:

  • change the name of the keys
  • remove keys
  • add keys
  • replace value

There's a repeating pattern, when updating some of the params need some tweaking and would be nice to have a predictable way to doing it without cryptic SQL, just plain easy PHP loops...

My 2c

@richard67
Copy link
Member

Good idea. It could be something like the updateassets thing where we have a hardcoded list for the core and extension devs who let their installer script derive from our installer script class can override its with a method using their list and then the same code.

Doing it in script.php has another advantage:

When something goes wrong with a database update for some unrelated reason and people use the fix button of the database checker, it will fix only the structure and so not run any update statements in any update SQL, and so the conversion would never happen. But when it is in script.php and called at the end of the fix like we do it with other stuff, it will be executed and all is fine.

@HLeithner
Copy link
Member

so you would like to have something like this https://laravel.com/docs/10.x/migrations

@richard67
Copy link
Member

so you would like to have something like this https://laravel.com/docs/10.x/migrations

@HLeithner If you ask me: No, that’s something completely different. We talk about the json modifications of e.g. the params column in the extensions table.

@HLeithner
Copy link
Member

It's about having migration scripts in php and not in sql files, that's the basic point, laravel also includes the complete abstraction package for db engines and rollback and so on. But the basic thing would be to use php instead of sql files for migration.

@dgrammatiko
Copy link
Contributor

It's about having migration scripts in php and not in sql files, that's the basic point

Laravel is doing it right!

Maybe before we reach the migration state of Laravel a simple function for the params is a good idea to test the waters. On the other hand a way to upgrade/downgrade the db from PHP would be awesome

@HLeithner
Copy link
Member

On the other hand a way to upgrade/downgrade the db from PHP would be awesome

And unrealistic ;-) when you break the db upgrade why should the downgrade work then^^ and if it is for up and downgrade between joomla versions I'm pretty sure we don't have the resources for this. But yes I like the idea of downgrade ;-)

@dgrammatiko
Copy link
Contributor

Just to recap here:

@richard67
Copy link
Member

There should be another PR that fixes the existing params of the plugin when upgrading and revert the runtime patches

@dgrammatiko and other readers: Question is if we shall add it to the postflight method here
https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L897-L898
so it's only executed when updating from 4.4 to 5.x,
or if we add it to the update method here
https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L80
so it's executed on every update, also from 5.x to 5.y, just for the case it might have failed when having updated from 4.4 once.

I think the postflight method would be better.

Other opinions?

@dgrammatiko
Copy link
Contributor

No strong opinions here: post flight should be fine (I guess this is not an abstract version that would be possible to modify any extension params, if it is then in the regular update cycle would be more helpful)

@richard67
Copy link
Member

No strong opinions here: post flight should be fine (I guess this is not an abstract version that would be possible to modify any extension params, if it is then in the regular update cycle would be more helpful)

@dgrammatiko The TinyMCE configuration migration is a very specific thing to that plugin because it's very tied on the configuration structure. You can have an arbitrary number of tool bars and menus and sets and so on. An abstract thing which covers that would be too complicated, and we update the editors to new majors only when we have a new major, that's why I tend to handle it with the postflight. I agree that it could be useful to have a more general method for easier migrations and that could happen with minor updates, too, so that would be called in the update method.

@dgrammatiko
Copy link
Contributor

Actually my idea above #40622 (comment) will be handy for couple more extensions for v5: the templates. So, if we assume that 5.3 will be available before the v5 RC (@HLeithner someone really needs to ping the Bootstrap team for their timeline) then both the templates (and all their related styles, and their children styles) need some adjustments to incorporate the -rgb CSS Variables (in 5.2 I did some voodoo but the usage was not extensive). You could check the Muta template to get an idea of the changes needed. Anyways, specific code that handles specific extension is also fine, just a heads up that tinyMCE won't be the only extension (also apart from the templates and the BS5.3 there's Codemirror that needs a major upgrade, which might also need some tweaks of the params)

@richard67
Copy link
Member

@dgrammatiko I agree with your previous comment. As said, the TinyMCE plugin and maybe the Codemirror might be a special case, while the template stuff could be handled in a more general way since the structure of the params cannot be modified by the user, while for the TinyMCE you can have an individual configuration with a different number of sets and toolbars and menus.

@richard67
Copy link
Member

Am working on the migration on update from 4.4 to 5.x in script.php. Not sure yet if I will have it ready this weekend.

@dgrammatiko
Copy link
Contributor

Nice

So there are 3 things here:

  • Remove these plugins
        // Version 6 unload removed plugins
        $plugins = array_filter($plugins, function ($plugin) {
            return !in_array($plugin, ['hr', 'paste', 'print']);
        });
  • Rename these buttons
$toolbar = str_replace(['fontselect', 'fontsizeselect', 'formatselect', 'styleselect'], ['fontfamily', 'fontsize', 'blocks', 'styles'], $toolbar);
  • Rename these menus
$menubar = str_replace(['fontformats', 'fontsizes', 'blockformats', 'formats'], ['fontfamily', 'fontsize', 'blocks', 'styles'], $menubar);

@richard67
Copy link
Member

@dgrammatiko Thanks ... the renaming arrays I had already found, the plugins I might have missed.

@brianteeman
Copy link
Contributor Author

image

@HLeithner
Copy link
Member

If we remove the template plugin then we have to fork it or replace it with our own version, the advanced template is a payed only plugin iirc

@dgrammatiko
Copy link
Contributor

we have to fork it or replace it with our own version

Yup

@richard67
Copy link
Member

Migration of the editor plugin parameters when updating from 4.4.x to 5.0.y has been added to Dimitris’ PR #40626

@richard67
Copy link
Member

@brianteeman Can we consider this issue being fixed since PR #40626 has been merged, which includes also the migration of the parameters on update? Or is there something remaining to be done?

@richard67
Copy link
Member

P.S.: And if something remaining, is it still a release blocker?

@dgrammatiko
Copy link
Contributor

@brianteeman @richard67 there's a PR for the Brocken source view: #40679

@richard67
Copy link
Member

@brianteeman @richard67 there's a PR for the Brocken source view: #40679

@dgrammatiko Yes, I just saw. Thanks for the PR. Anything else remaining to be fixed?

@dgrammatiko
Copy link
Contributor

I've asked Harald if he wants to expose the code mirror options to the TinyMCE Sets (ie https://github.com/joomla/joomla-cms/pull/40679/files#diff-6bdf64a4a47bdcc37efb60d1257a5ade08b98727ae6978661ac36e8a50de1ccbR12-R31) If so, some fields have to be introduced in the tinybuilder and also your code needs to fill the existing params (+ the installation sql files)

@brianteeman
Copy link
Contributor Author

Not a release blocker but the following cosmetic issues remain

  1. Seperators
    The editor no longer displays a | as a seperator between groups of buttons. That's fine but the mockup editor in the plugin still displays the |

  2. Rounded corners
    The editor now has rounded courners. Looks odd as the rest of atum doesnt.

@dgrammatiko
Copy link
Contributor

@brianteeman for the separators use this in the Atum SCSS:

.tox-toolbar__group:not(:last-of-type) {
  border-inline-end: 1px solid var(--dark);
}

Result:
Screenshot 2023-08-11 at 18 32 35

For the rounded corners, use:

.tox-tinymce {
  border-radius: var(--border-radius);
}

Result:
Screenshot 2023-08-11 at 18 34 58

@dgrammatiko dgrammatiko mentioned this issue Sep 27, 2023
2 tasks
@brianteeman
Copy link
Contributor Author

closed see #41945

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants