User tests: Successful: Unsuccessful:
Pull Request for Issue #41164 .
This commit changes all usages of JComponentHelper to \Joomla\CMS\Component\ComponentHelper. Additionally an exception is thrown when JComponehtHelper is used as filter but is not resolvable.
@richard67 we need a migration script for the database entries... at least for the plugins and the custom fields. Any idea what's the best way is for this?
Open Article add html save it.
HTML format get removed
HTML format still here
Please select:
Documentation link for docs.joomla.org:
No documentation changes for docs.joomla.org needed
Pull Request link for manual.joomla.org: Needs documentation that JComponentHelper is not resolved anymore
No documentation changes for manual.joomla.org needed
Category | ⇒ | Administration com_banners com_categories com_contact com_content com_messages com_modules com_newsfeeds com_tags Front End com_config SQL Installation Postgresql Libraries Plugins |
Status | New | ⇒ | Pending |
I have tested this item ✅ successfully on d0a2509
tested the editor with the backward compatibility plugin disabled and the formatting was kept
@HLeithner A migration function could be called in the "postflight" method in script.php similar to here:
It might need to modify the version check before that so it runs also when updating from a 5.0.0-alpha, i.e. anything lower than 5.0.0-beta1: https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L2198-L2200
The method function be a new private function similar to the one called in the example above https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L2217
can you help me writing this function please?
I would suggest to make a filter class, instead.
Example like for filter="safehtml"
joomla-cms/libraries/src/Form/Filter/SafehtmlFilter.php
Lines 43 to 51 in e2170c8
That will call Joomla\CMS\Component\ComponentHelper::filterText
internally:
public function filter($element, $value ...) {
return Joomla\CMS\Component\ComponentHelper::filterText($value);
}
I actually agree with @Fedik here. I think given this is a very common pattern in core making lots of people hardcode php callbacks is asking for b/c breaks (because extensions will be using this too). Making it a static name will be more maintainable going forwards.
have you looked at the loadType
how complicate it is to do the same thing then just setting the real name of the filter class+function.
It also uses J-prefix for classes, I'm not even sure which namespaces it considers, it might look only for Joomla core namespace? maybe I'm wrong. But this construct to just load one of our 5 filters is completely over engineered, if we allow such thing like shortcut to a filter object method then I think hardcoding that 5 classes makes more sense then expecting any 3rd party dev to reverse engineer this function...
never the less I'm happy for a shortcut for simple text filterText, but I would deprecate/remove this class autoloading voodoo
@HLeithner A migration function could be called in the "postflight" method in script.php similar to here:
https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L2202-L2205
It might need to modify the version check before that so it runs also when updating from a 5.0.0-alpha, i.e. anything lower than 5.0.0-beta1: https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L2198-L2200
The method function be a new private function similar to the one called in the example above https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L2217can you help me writing this function please?
@HLeithner Don‘t expect any help or advise from me because when I give some you obviously ignore it.
ok no problem thanks
In reading through the comments, would you like me to test this now or wait until more changes are made?
Yes please test it (with new installatons for now only, updates without b/c plugin are not handled yet)
@bayareajenn the code works - the question is if it is an appropriate solution
I have tested this item
Tested with b/c plugin enabled and disabled in articles, categories, and custom modules. Works in all cases. Thanks :)
@HLeithner A migration function could be called in the "postflight" method in script.php similar to here:
https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L2202-L2205
It might need to modify the version check before that so it runs also when updating from a 5.0.0-alpha, i.e. anything lower than 5.0.0-beta1: https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L2198-L2200
The method function be a new private function similar to the one called in the example above https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L2217can you help me writing this function please?
@HLeithner What needs to be migrated?
Update: Ah, I just see the difference in the base.sql. Just params of 2 plugins in the extensions table. Or is there more?
yes, at least com_fields needs to be updated JComponentHelper::filterText -> to the namespaced version
yes, at least com_fields needs to be updated JComponentHelper::filterText -> to the namespaced version
Will see if I can provide something tomorrow.
thanks
Labels |
Added:
bug
PR-5.0-dev
|
@HLeithner As the replacement would be a complete parameter value, it could be done easily with an update SQL script and string replacement. But that is not really nice with json strings, even if it would be safe, so the other way would be a conversion method in script.php like I suggested above. What do you prefer? Easy but theoretical risky way in SQL? Or safer but maybe a bit over engineered way in PHP?
Thats a good question... We could do it with SQL json functions...?
I'm happy with both
Thats a good question... We could do it with SQL json functions...?
I'm happy with both
I'm not familiar with json functions in SQL.
@HLeithner I've just made a new installation on a MySQL 8.0 database with the branch of this PR and then immediately after the installation I've checked the parameters in database. They lose the backslash, so for the editor field they are e.g.:
{"buttons":0,"width":"100%","height":"250px","filter":"JoomlaCMSComponentComponentHelper::filterText"}
The form shows "No" as selected filter for that field.
If I change the selection in the form to "Text" and save the form, the result in database is with double backslashes:
{"buttons":0,"width":"100%","height":"250px","filter":"\\Joomla\\CMS\\Component\\ComponentHelper::filterText"}
So we have to use double backslashes in the base.sql scripts for the params
's filter
values:
(0, 'plg_fields_editor', 'plugin', 'editor', 'fields', 0, 1, 1, 0, 1, '', '{"buttons":0,"width":"100%","height":"250px","filter":"\\Joomla\\CMS\\Component\\ComponentHelper::filterText"}', '', 4, 0),
I haven't tested PostgreSQL yet, but I expect it to be the same.
The same applies to PostgreSQL 14.9 and MariaDB 10.6, I've just tested.
The XML files are ok, no need for double backslashes there.
I've also experimented with an update SQL statement for an update script. On MySQL 8.0 and MariaDB 10.6, the following works with double backslashes:
UPDATE `#__extensions`
SET `params` = JSON_REPLACE(`params`, '$.filter' , '\\Joomla\\CMS\\Component\\ComponentHelper::filterText')
WHERE `type` = 'plugin'
AND `folder` = 'fields'
AND `params` <> ''
AND JSON_EXTRACT(`params`, '$.filter') = 'JComponentHelper::filterText';
This would change it for all field plugins which have a filter property equal to "JComponentHelper::filterText" in their params, which would also include 3rd party field plugins.
Should we limit this to the editor and textarea plugins with an additional AND element in ('editor', 'textarea')
so that it's limited to the core only?
For PostgreSQL this could be used:
UPDATE "#__extensions"
SET "params" = jsonb_set("params"::jsonb, '{filter}' , '"\\Joomla\\CMS\\Component\\ComponentHelper::filterText"')
WHERE "type" = 'plugin'
AND "folder" = 'fields'
AND "params" <> ''
AND "params"::jsonb->>'filter' = 'JComponentHelper::filterText';
@HLeithner I've created PR #41417 with my changes suggested above for the base.sql scripts and an update SQL for migrating the parameter.
The update SQL currently would change it for all field plugins which have a filter property equal to "JComponentHelper::filterText" in their params, which would also include 3rd party field plugins.
Should we limit this to the editor and textarea plugins with an additional AND element in ('editor', 'textarea')
so that it's limited to the core only?
I wouldn't limit it, because this would also convert 3rd party fields... Thinking about this might be a problem... Maybe you are right and we limit it to (relevant) core fields only
I wouldn't limit it, because this would also convert 3rd party fields... Thinking about this might be a problem... Maybe you are right and we limit it to (relevant) core fields only
@HLeithner Relevant core fields plugins are only 'editor', 'text' and 'textarea', is that right?
I wouldn't limit it, because this would also convert 3rd party fields... Thinking about this might be a problem... Maybe you are right and we limit it to (relevant) core fields only
@HLeithner Relevant core fields plugins are only 'editor', 'text' and 'textarea', is that right?
I've updated my PR to limit the migration to the 3 named plugins.
@HLeithner You wrote in the description of this PR:
@richard67 we need a migration script for the database entries... at least for the plugins and the custom fields. Any idea what's the best way is for this?
For the plugins we have my PR for you.
But what needs to be done for custom fields?
Ah, I see ... the fieldparams
column of the #__fields
table might need to be migrated, too. Will check for details.
Status | Pending | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-08-22 06:44:32 |
Closed_By | ⇒ | laoneo |
Merging this for now to have the description fields accepting HTML content again. Thanks @richard67 for your patience on this.
This pr doesn't prevent to add a more sophisticated solution in the future, as @Fedik suggested.
@HLeithner A migration function could be called in the "postflight" method in script.php similar to here:
https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L2202-L2205
It might need to modify the version check before that so it runs also when updating from a 5.0.0-alpha, i.e. anything lower than 5.0.0-beta1: https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L2198-L2200
The method function be a new private function similar to the one called in the example above https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L2217