User tests: Successful: Unsuccessful:
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.
Open the back end with PHP 8.2.
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
No warning.
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
Category | ⇒ | Libraries |
Status | New | ⇒ | Pending |
Labels |
Added:
PHP 8.x
|
Title |
|
Category | Libraries | ⇒ | Modules Administration Libraries |
Labels |
Added:
?
|
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.
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)) {
}
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.
I found this on github
if this is normal for breezingforms I would expect we break this component right?
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?
I found this on github
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.
Category | Libraries Modules Administration | ⇒ | Libraries |
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?
Title |
|
I have tested this item
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC. Thanks !
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:
?
|
Thank you
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)