[CMS PR 40626] Add migration on update

[richard67]

Pull Request for joomla#40626 .

Summary of Changes

This PR adds the migration of the TinyMCE editor plugin's parameters (column params of the record for that plugin in table #__extensions) when updating the CMS from 4.4 to 5 and so updating TinyMCE from 5 to 6.

@dgrammatiko Should we add this to your PR, or should I create a separate one in the CMS repo? And could you advise on that plugin thing? I see we have also external plugins, and maybe we should not touch these?

[dgrammatiko]

Let's ask Harald how he wants to proceeds here

@HLeithner ?

[HLeithner]

First, please make it smart but self explaining and document each step, it's hard to read for example I don't understand why you only migrate toolbar1 and toolbar2 but not toolbar0. Which after looking deeper into the config array is understand able.

It would be nicer to have comments what happens when why something like.

// Migrate toolbar set 0 to 2
// If we have a menu entry in the toolbar set we have to rename the following keys:
'fontformats' => 'fontfamily'
...

// if the functions are used in the toolbar1 or 2 we also have to replace the keys.

The rest looks ok. using the extensiontable object doesn't make sense here I would expect.

Having this in the same pr is fine for me.

Thanks for your work.

[richard67]

@HLeithner TinyMCE allows to have “toolbar” if it is one only, or “toolbar1”, “toolbar2” and so one if more, but no “toolbar0”. The same applies to “menu”. At least that’s how I understand their documentation for version 6.

The reason why we use only “menu” and “toolbar1” and “toolbar2” is because in the plugin code we read only these parameters.

If you want I can change this PR here to cater for the more general case so we don’t have to change the code here when we change it in the plugin to handle more menus.

Question: Where does it need to change array keys? I did not see the need when reading the plugin code and Dimitris PR.

And does anyone know what to do with obsolete tinymce plugins?

Last question: If we have the migration on update, do we really need the “b/c compatibility” code in Dimitris’ PR?

[HLeithner]

Ok it's not a array, I thought it's a key with configuration options on it, didn't looked to deep into the json string.

It is ok how it is, only comments would be great to understand why we do what.

[richard67]

@brianteeman Maybe you can help here and answer some of the questions in my previous comment?

Or someone with more knowledge than I have about the tinymce takes it and implements it based on my PR here as a start.

[richard67]

It is ok how it is, only comments would be great to understand why we do what.

That would require the coder (me) to know why he is doing what. I’m not sure if this is always the case. :-) (joking)

[brianteeman]

iirc the multiple toolbars is a legacy from j3 and tinymce4. There used to be multiple rows of toolbars but they changed it so that there was just one row that wrapped (or expanded on the ... )

[richard67]

iirc the multiple toolbars is a legacy from j3 and tinymce4. There used to be multiple rows of toolbars but they changed it so that there was just one row that wrapped (or expanded on the ... )

Hmm, if that’s the case we maybe should merge toolbar1 and toolbar2 into toolbar?

On the other hand when quick reading their docs for v6 it seems they still allow to have toolbar1, toolbar2, toolbar3 and the same for menus.

The migration should cover every possible configuration of the user, everything which we allow to do in the plugin configuration in our backend.

Up to now I have only checked the json which we have for a new installation. I haven’t checked yet the possible user configurations. This could be time consuming, so any help would be appreciated.

[brianteeman]

Hmm, if that’s the case we maybe should merge toolbar1 and toolbar2 into toolbar?

From memory that is what we are doing in code - concatenating the toolbars into a single toolbar

[richard67]

Hmm, if that’s the case we maybe should merge toolbar1 and toolbar2 into toolbar?

From memory that is what we are doing in code - concatenating the toolbars into a single toolbar

Yes, but always on the fly, so we still have 2 toolbars in the configuration params (of which "toolbar2" might be never used, I have to test this).

Question is if we should change our code so we have only "toolbar" in the configuration params and let the migration do that merge of the toolbars once on update from 4.4 to 5.x.

[HLeithner]

since it seems it's not longer possible to have a second toolbar like we do it in the plugin configuration we can also remove it and migrate them to one entry.

[richard67]

since it seems it's not longer possible to have a second toolbar like we do it in the plugin configuration we can also remove it and migrate them to one entry.

Ok, here the link to the docs I had in mind: https://www.tiny.cloud/docs/tinymce/6/toolbar-configuration-options

There is a section using-multiple-toolbars. It seems that we just don't support that.

For menus I've found this: https://www.tiny.cloud/docs/tinymce/6/menus-configuration-options/ . It seems for menus we can be sure there is only one.

Currently this PR here works with the code of the CMS PR joomla#40626 . So maybe I should add a few comments and then make it ready for review so Dimitris can merge it into the CMS PR.

If we then decide to move migrations from on the fly to the update migration here, like e.g. merging "toolbar1" and "toolbar2" into "toolbar", it would need to change the migration code in script.php and the code of the plugin's DisplayTrait. So I think we should have both together in the same CMS PR.

Do you all agree?

[HLeithner]

ok the reason why we have only one toolbar is because tinymce 5.0 removed it and reintroduced them in 5.0.9

TinyMCE 5.0.9 adds back the ability to include multiple toolbars in the editor, similar to TinyMCE 4.x. Multiple toolbars can be configured by using the toolbar array or toolbar1, toolbar2, etc… configuration options.

https://www.tiny.cloud/docs/release-notes/release-notes509/

would be cool if we could support both multiple toolbars and sliding (it's xor)
https://www.tiny.cloud/docs/configure/editor-appearance/#toolbar_mode

So I would keep it in 2 toolbars for the moment (except someone likes to make it configure able in the plugin config) and add the option for multi toolbar or the other modes (we already have the option for sliding, scrolling, ...)

[richard67]

I've added some comments and changed a misleading variable name. @HLeithner Are you ok with it?

Before I set this PR to ready for review, i.e. remove draft status, I have to test if it works when building an update package with my branch and updating a 4.4-dev with that.

[richard67]

@HLeithner Currently my test with updating to a 5.0 which contains the changes here fails because the following PR hasn't been merged up from 4.4-dev into 5.0-dev yet: joomla#40569 . Could you do an upmerge so @dgrammatiko can update his PR after that so we get usable update packages?

[HLeithner]

Will upmerge later

[richard67]

@HLeithner If that helps: I've made a branch with an upmerge of current 4.4-dev and conflicts resolved, see https://github.com/joomla/joomla-cms/compare/5.0-dev...richard67:5.0-dev-test-upmerge-2023-05-21?expand=1 . If you want, I can make a PR for you in the CMS repo.

Update: Unfortunately some of the files from the upmerge still contain __DEPLOY_VERSION__ because in 4.4-dev there hasn't been made a version bump yet. We have to be careful to do an upmerge after they have made a bump and before the 5.0.0 alpha 1 so we don't get different @from versions in 4.4-dev and 5.0-dev.

I'm also not sure about the changes in the package-lock.json file. Maybe that should be reverted in my upmerge branch.

No, they seem to be ok.

[dgrammatiko]

@richard67 there's an update (pending to approval) for the templates. The key is changed from templates to jtemplates (to get rid of the warning) joomla@f93cf64

[richard67]

@richard67 there's an update (pending to approval) for the templates. The key is changed from templates to jtemplates (to get rid of the warning) joomla@f93cf64

@dgrammatiko Which PR is that? The linked commit doesn't show that. And I don't see anything with "templates" in the configuration in the params column of the extensions table for that, so it might be something which does not have any impact on this PR here.

[dgrammatiko]

joomla#40626

[richard67]

@dgrammatiko As that commit hasn't changed anything in the params column of the extensions table, i.e. no change in base.sql, there is nothing to be adapted here in my PR as far as I can see.

[richard67]

I think this PR here is ready. I have just tested with an update package which contains the changes from here and the results of an upmerge from 4.4-dev to fix other update issues.

[dgrammatiko]

@richard67 don't commit yet the template changes

[richard67]

@dgrammatiko Do we really have that templates thing in both the menu and the toolbar buttons?

[dgrammatiko]

On the default configuration it's only on the toolbar2, but someone could add it to toolbar1 when editing the plugin (?). In sort I'm not sure

[richard67]

On the default configuration it's only on the toolbar2, but someone could add it to toolbar1 when editing the plugin (?). In sort I'm not sure

@dgrammatiko That's not the question. The question is if it is also in the menu. If it is in both the menu and any toolbar it needs to be changed a both placers as you suggested, but if it is only any of the toolbar buttons but not a menu entry it does not to be added to the replacements for the menu.

[richard67]

Now I need to go off my desk for a while. Will check later tonight again.

[dgrammatiko]

Actually, the plugin is still named template, so all these should be ignored

[richard67]

Actually, the plugin is still named template, so all these should be ignored

@dgrammatiko Then mark them as resolved.

[richard67]

Actually, the plugin is still named template, so all these should be ignored

@dgrammatiko I still don't get it. Shall I apply your suggestions to replace it by jtemplate, or not? And are you really sure it applies also to the menu and not only to the toolbar buttons?

[richard67]

@dgrammatiko Done.

[richard67]

@dgrammatiko If you merge this PR here it will be part of your PR for the CMS. I’m not sure if you have noticed that already.

[dgrammatiko]

Merged it. I was waiting Harald...

[richard67]

Well he can continue review in you PR.

[dgrammatiko]

I've removed the runtime patches on the PR, so I guess we could continue there.

Thank you for this PR! 🙌