No Code Attached Yet bug
avatar brianteeman
brianteeman
18 May 2023

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.

avatar brianteeman brianteeman - open - 18 May 2023
avatar joomla-cms-bot joomla-cms-bot - change - 18 May 2023
Labels Added: No Code Attached Yet
avatar joomla-cms-bot joomla-cms-bot - labeled - 18 May 2023
avatar dgrammatiko
dgrammatiko - comment - 18 May 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!

avatar HLeithner
HLeithner - comment - 19 May 2023

Would you be so nice and create a pr @dgrammatiko

avatar dgrammatiko
dgrammatiko - comment - 19 May 2023

@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

avatar HLeithner
HLeithner - comment - 19 May 2023

I mean the missing part from the pr you mentioned?

avatar dgrammatiko
dgrammatiko - comment - 19 May 2023

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

avatar HLeithner
HLeithner - comment - 19 May 2023

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

avatar dgrammatiko
dgrammatiko - comment - 19 May 2023

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

avatar dgrammatiko
dgrammatiko - comment - 19 May 2023

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

avatar richard67
richard67 - comment - 19 May 2023

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.

avatar HLeithner
HLeithner - comment - 20 May 2023

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?

avatar dgrammatiko
dgrammatiko - comment - 20 May 2023

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

avatar richard67
richard67 - comment - 20 May 2023

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.

avatar HLeithner
HLeithner - comment - 20 May 2023

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

avatar richard67
richard67 - comment - 20 May 2023

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.

avatar HLeithner
HLeithner - comment - 20 May 2023

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.

avatar dgrammatiko
dgrammatiko - comment - 20 May 2023

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

avatar HLeithner
HLeithner - comment - 20 May 2023

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 ;-)

avatar dgrammatiko
dgrammatiko - comment - 20 May 2023

Just to recap here:

  • #40626 fixes new installations (correct params)
  • #40626 fixes B/C with runtime code (which is not optimal/preferable)
  • There should be another PR that fixes the existing params of the plugin when upgrading and revert the runtime patches
avatar richard67
richard67 - comment - 20 May 2023

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?

avatar dgrammatiko
dgrammatiko - comment - 20 May 2023

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)

avatar richard67
richard67 - comment - 20 May 2023

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.

avatar dgrammatiko
dgrammatiko - comment - 20 May 2023

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)

avatar richard67
richard67 - comment - 20 May 2023

@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.

avatar richard67
richard67 - comment - 20 May 2023

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.

avatar dgrammatiko
dgrammatiko - comment - 20 May 2023

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);
avatar richard67
richard67 - comment - 20 May 2023

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

avatar brianteeman
brianteeman - comment - 20 May 2023

image

avatar HLeithner
HLeithner - comment - 20 May 2023

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

avatar dgrammatiko
dgrammatiko - comment - 20 May 2023

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

Yup

avatar richard67
richard67 - comment - 21 May 2023

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

avatar richard67 richard67 - labeled - 21 May 2023
avatar richard67 richard67 - change - 21 May 2023
Labels Added: bug
avatar wilsonge wilsonge - change - 23 May 2023
Labels Added: Release Blocker
avatar wilsonge wilsonge - labeled - 23 May 2023
avatar richard67
richard67 - comment - 29 May 2023

@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?

avatar richard67
richard67 - comment - 29 May 2023

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

avatar dgrammatiko
dgrammatiko - comment - 29 May 2023

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

avatar richard67
richard67 - comment - 29 May 2023

@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?

avatar dgrammatiko
dgrammatiko - comment - 29 May 2023

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)

avatar brianteeman
brianteeman - comment - 29 May 2023

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.

avatar richard67 richard67 - change - 29 May 2023
Labels Removed: Release Blocker
avatar richard67 richard67 - unlabeled - 29 May 2023
avatar wilsonge wilsonge - change - 7 Jun 2023
Labels Added: Release Blocker
avatar wilsonge wilsonge - labeled - 7 Jun 2023
avatar wilsonge wilsonge - change - 7 Jun 2023
Labels Removed: Release Blocker
avatar wilsonge wilsonge - unlabeled - 7 Jun 2023
avatar dgrammatiko
dgrammatiko - comment - 11 Aug 2023

@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

avatar brianteeman brianteeman - change - 27 Sep 2023
Status New Closed
Closed_Date 0000-00-00 00:00:00 2023-09-27 11:33:45
Closed_By brianteeman
avatar brianteeman
brianteeman - comment - 27 Sep 2023

closed see #41945

avatar brianteeman brianteeman - close - 27 Sep 2023

Add a Comment

Login with GitHub to post a comment