bug PR-5.0-dev Pending

User tests: Successful: Unsuccessful:

avatar HLeithner
HLeithner
26 Jul 2023

Pull Request for Issue #41164 .

Summary of Changes

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?

Testing Instructions

Open Article add html save it.

Actual result BEFORE applying this Pull Request

HTML format get removed

Expected result AFTER applying this Pull Request

HTML format still here

Link to documentations

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

avatar joomla-cms-bot joomla-cms-bot - change - 26 Jul 2023
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
avatar HLeithner HLeithner - open - 26 Jul 2023
avatar HLeithner HLeithner - change - 26 Jul 2023
Status New Pending
avatar richard67
richard67 - comment - 26 Jul 2023

@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

avatar brianteeman brianteeman - test_item - 26 Jul 2023 - Tested successfully
avatar brianteeman
brianteeman - comment - 26 Jul 2023

I have tested this item ✅ successfully on d0a2509

tested the editor with the backward compatibility plugin disabled and the formatting was kept


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41259.

avatar HLeithner
HLeithner - comment - 27 Jul 2023

@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

can you help me writing this function please?

avatar Fedik
Fedik - comment - 27 Jul 2023

I would suggest to make a filter class, instead.
Example like for filter="safehtml"

public function filter(\SimpleXMLElement $element, $value, $group = null, Registry $input = null, Form $form = null)
{
return InputFilter::getInstance(
[],
[],
InputFilter::ONLY_BLOCK_DEFINED_TAGS,
InputFilter::ONLY_BLOCK_DEFINED_ATTRIBUTES
)->clean($value, 'html');
}

That will call Joomla\CMS\Component\ComponentHelper::filterText internally:

public function filter($element, $value ...) {
  return Joomla\CMS\Component\ComponentHelper::filterText($value);
}
avatar wilsonge
wilsonge - comment - 27 Jul 2023

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.

avatar HLeithner
HLeithner - comment - 27 Jul 2023

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

avatar richard67
richard67 - comment - 27 Jul 2023

@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

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

avatar HLeithner
HLeithner - comment - 27 Jul 2023

ok no problem thanks

avatar bayareajenn
bayareajenn - comment - 28 Jul 2023

In reading through the comments, would you like me to test this now or wait until more changes are made?

avatar HLeithner
HLeithner - comment - 28 Jul 2023

Yes please test it (with new installatons for now only, updates without b/c plugin are not handled yet)

avatar brianteeman
brianteeman - comment - 28 Jul 2023

@bayareajenn the code works - the question is if it is an appropriate solution

avatar bayareajenn bayareajenn - test_item - 28 Jul 2023 - Tested successfully
avatar bayareajenn
bayareajenn - comment - 28 Jul 2023

I have tested this item successfully on d0a2509

Tested with b/c plugin enabled and disabled in articles, categories, and custom modules. Works in all cases. Thanks :)


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41259.

avatar richard67
richard67 - comment - 20 Aug 2023

@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

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

avatar HLeithner
HLeithner - comment - 20 Aug 2023

yes, at least com_fields needs to be updated JComponentHelper::filterText -> to the namespaced version

avatar richard67
richard67 - comment - 20 Aug 2023

yes, at least com_fields needs to be updated JComponentHelper::filterText -> to the namespaced version

Will see if I can provide something tomorrow.

avatar HLeithner
HLeithner - comment - 20 Aug 2023

thanks

avatar richard67 richard67 - change - 20 Aug 2023
Labels Added: bug PR-5.0-dev
avatar richard67
richard67 - comment - 20 Aug 2023

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

avatar HLeithner
HLeithner - comment - 20 Aug 2023

Thats a good question... We could do it with SQL json functions...?

I'm happy with both

avatar richard67
richard67 - comment - 20 Aug 2023

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.

avatar richard67
richard67 - comment - 21 Aug 2023

@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';
avatar richard67
richard67 - comment - 21 Aug 2023

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

avatar HLeithner
HLeithner - comment - 21 Aug 2023

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

avatar richard67
richard67 - comment - 21 Aug 2023

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?

avatar richard67
richard67 - comment - 21 Aug 2023

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.

avatar richard67
richard67 - comment - 21 Aug 2023

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

avatar richard67
richard67 - comment - 21 Aug 2023

Ah, I see ... the fieldparams column of the #__fields table might need to be migrated, too. Will check for details.

avatar richard67
richard67 - comment - 21 Aug 2023

I've updated my PR #41417 by the migration of the custom field's fieldparams on update. I haven't tested it yet, but it should work.

avatar laoneo laoneo - change - 22 Aug 2023
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2023-08-22 06:44:32
Closed_By laoneo
avatar laoneo laoneo - close - 22 Aug 2023
avatar laoneo laoneo - merge - 22 Aug 2023
avatar laoneo
laoneo - comment - 22 Aug 2023

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.

Add a Comment

Login with GitHub to post a comment