joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.74k stars 3.64k forks source link

[5.0] Tinymce v6 #40622

Closed brianteeman closed 1 year ago

brianteeman commented 1 year ago

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 commented 1 year ago

You're right. Also it seems the code committed into 5.0 with https://github.com/joomla/joomla-cms/pull/37633 is missing parts of https://github.com/Digital-Peak/joomla-cms/pull/21

This issue deserves a Release Blocker tag!

HLeithner commented 1 year ago

Would you be so nice and create a pr @dgrammatiko

dgrammatiko commented 1 year ago

@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 commented 1 year ago

I mean the missing part from the pr you mentioned?

dgrammatiko commented 1 year ago

With https://github.com/joomla/joomla-cms/pull/40626 I tried to patch both the missing parts and introduce some runtime fixes for toolbar/menus.

HLeithner commented 1 year ago

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

dgrammatiko commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

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 commented 1 year ago

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 commented 1 year ago

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

richard67 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Just to recap here:

richard67 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Actually my idea above https://github.com/joomla/joomla-cms/issues/40622#issuecomment-1555869217 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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

Nice

So there are 3 things here:

richard67 commented 1 year ago

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

brianteeman commented 1 year ago

image

HLeithner commented 1 year ago

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 commented 1 year ago

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

Yup

richard67 commented 1 year ago

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

richard67 commented 1 year ago

@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 commented 1 year ago

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

dgrammatiko commented 1 year ago

@brianteeman @richard67 there's a PR for the Brocken source view: https://github.com/joomla/joomla-cms/pull/40679

richard67 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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
brianteeman commented 1 year ago

closed see #41945