? PHP 8.x ? Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
11 Jan 2023

Summary of Changes

Adds a check when the title is rendered in the toolbar if the app has the property JComponentTitle.

We are going basically back to the beginning cdd4173#diff-5f6eb93d25275fcc88e719c1c9e8df1de7be12c716b4632404256471aa953ad4L41.

Defines the missing property JComponentTitle in the WebApplication class. After this is upmerged to 4.3, it will be deprecated.

Testing Instructions

Open the back end with PHP 8.2.

Actual result BEFORE applying this Pull Request

The wollowing warning is shown:
`Creation of dynamic property Joomla\CMS\Application\AdministratorApplication::$JComponentTitle is deprecated in libraries/src/Toolbar/ToolbarHelper.php on line 52

Expected result AFTER applying this Pull Request

No warning.

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:

  • No documentation changes for manual.joomla.org needed

avatar joomla-cms-bot joomla-cms-bot - change - 11 Jan 2023
Category Libraries
avatar laoneo laoneo - open - 11 Jan 2023
avatar laoneo laoneo - change - 11 Jan 2023
Status New Pending
avatar laoneo laoneo - change - 11 Jan 2023
Labels Added: PHP 8.x
avatar laoneo laoneo - change - 11 Jan 2023
Title
Test if an app has the property JComponentTitle
[4.2] PHP8.2 Test if an app has the property JComponentTitle
avatar laoneo laoneo - edited - 11 Jan 2023
avatar joomla-cms-bot joomla-cms-bot - change - 11 Jan 2023
Category Libraries Modules Administration Libraries
avatar laoneo laoneo - change - 11 Jan 2023
Labels Added: ?
avatar laoneo laoneo - change - 11 Jan 2023
The description was changed
avatar laoneo laoneo - edited - 11 Jan 2023
avatar joomdonation
joomdonation - comment - 11 Jan 2023

With the change in this PR, JComponentTitle won't be set to $app, so there is backward incompatible change here in case there is an extension (like mod_title in our core code) want to access to this value via $app->JComponentTitle . Also, I'm not sure if using $app->set to store random data is a good change.

For this, maybe we should add JComponentTitle as a public property to CMSApplication instead (ToolbarHelper::title could be used on both from frontend and backend)

avatar laoneo
laoneo - comment - 12 Jan 2023

This property was never defined, nor documented, so I wouldn't count it as public API, therefore no BC break.

It was added as a property just for the sake of micro optimization and was before in the internal storage of the app, see my linked commit. So we are going basically back to where we were at the beginning.

avatar joomdonation
joomdonation - comment - 12 Jan 2023

This property was never defined, nor documented, so I wouldn't count it as public API, therefore no BC break

I'm unsure about this, so I won't record my test result, sorry. Even if that's accepted, why do we have to use the isset command in the code below on both files modified in this PR? That never returns true if the property is not defined.

if (isset($app->JComponentTitle)) {
       
}
avatar laoneo
laoneo - comment - 12 Jan 2023

Because it is a hack, you should not define a custom property in a class which doesn't support it. So I'm glad PHP 8.2 did report that. This is an abuse and is bad code practice, I would remove it completely, but you never know what for apps do exist into the wild, so I'm leaving the variable assignment.

I would even go that far and deprecated it in 4.3 and remove in 6.0.

avatar HLeithner
HLeithner - comment - 12 Jan 2023

I found this on github

https://github.com/valeriekebz/WebsiteKEBS/blob/bc01497de0e49e3fe89a2b7b7c7af4a9159e450c/administrator/components/com_breezingforms/breezingforms.php#L58-L62

if this is normal for breezingforms I would expect we break this component right?

avatar HLeithner
HLeithner - comment - 12 Jan 2023

here is another one:
https://github.com/hooNam/Solidres/blob/8b21829e18284680732a66c3e6cf0dbcde4e0b49/com_solidres/frontend/com_solidres/helpers/toolbar.php#L37-L39

adding this variable to WebApplication and deprecate it with j4.3 and remove it in 6 would be ok, but we need a bzw. layer for it right?

Also I would go more specific with an own g/setTitle() function in the WebApplication instead of a generic set/get.

Or maybe moving this variable to the htmldocument? would this make more sense?

avatar laoneo
laoneo - comment - 12 Jan 2023

I found this on github

https://github.com/valeriekebz/WebsiteKEBS/blob/bc01497de0e49e3fe89a2b7b7c7af4a9159e450c/administrator/components/com_breezingforms/breezingforms.php#L58-L62

if this is normal for breezingforms I would expect we break this component right?

Nope, because breezing forms is defining that variable (will also result in a deprecate warning) which then takes precedence here over the value from the app data store.

The same applies for solidres. So from BC perspective we are covered, except when a module is using that variable only with core. Then it is not available, but again this is undocumented and not even close to a public API.

Also I would go more specific with an own g/setTitle() function in the WebApplication instead of a generic set/get.

This is not the title of the application, it is really only a html string which gets then printed in the title module. So for now I think the closest would probably be the toolbar itself.

avatar joomla-cms-bot joomla-cms-bot - change - 12 Jan 2023
Category Libraries Modules Administration Libraries
1d348ab 12 Jan 2023 avatar laoneo cs
avatar laoneo laoneo - change - 12 Jan 2023
The description was changed
avatar laoneo laoneo - edited - 12 Jan 2023
avatar laoneo laoneo - change - 12 Jan 2023
The description was changed
avatar laoneo laoneo - edited - 12 Jan 2023
avatar laoneo
laoneo - comment - 12 Jan 2023

Changed my mind and just added the missing property to the WebApplication class. Once upmerged to 4.3 we can then deprecate it.

@joomdonation and @carlitorweb would you mind to give it a test?

avatar laoneo laoneo - change - 12 Jan 2023
Title
[4.2] PHP8.2 Test if an app has the property JComponentTitle
[4.2] PHP8.2 Define the JComponentTitle property in WebApplication class
avatar laoneo laoneo - edited - 12 Jan 2023
avatar carlitorweb carlitorweb - test_item - 12 Jan 2023 - Tested successfully
avatar carlitorweb
carlitorweb - comment - 12 Jan 2023

I have tested this item successfully on 9dee2a2


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

avatar joomdonation joomdonation - test_item - 13 Jan 2023 - Tested successfully
avatar joomdonation
joomdonation - comment - 13 Jan 2023

I have tested this item successfully on 9dee2a2


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

avatar joomdonation joomdonation - change - 13 Jan 2023
Status Pending Ready to Commit
avatar joomdonation
joomdonation - comment - 13 Jan 2023

RTC. Thanks !


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

avatar roland-d roland-d - change - 14 Jan 2023
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2023-01-14 10:32:16
Closed_By roland-d
Labels Added: ?
avatar roland-d roland-d - close - 14 Jan 2023
avatar roland-d roland-d - merge - 14 Jan 2023
avatar roland-d
roland-d - comment - 14 Jan 2023

Thank you

Add a Comment

Login with GitHub to post a comment