User tests: Successful: Unsuccessful:
Allows dynamic properties on log entries so different formatters can add custom properties.
Open the article form on the PHP 8.2 on the front end with debug enabled.
The following warning is shown:
Creation of dynamic property Joomla\CMS\Log\LogEntry::$clientIP is deprecated
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 |
Shouldn't we add the missing properties instead? By scanning the code, there are two missing properties:
should be enough based on https://php.watch/versions/8.2/AllowDynamicProperties
First I did this, then I came to the conclusion that, every formater can add it's own custom properties. That's why I made it that way. The text logger is a good example. It adds specific properties which are only used by that specific logger. Others can do the same. That's why the LogClass should allow dynamic properties.
should be enough based on https://php.watch/versions/8.2/AllowDynamicProperties
Enough for not but will result on fatal error on PHP 9. I think we should take this chance to fix the code properly instead of hiding the warnings for now and suddenly seeing fatal when PHP 9 releases.
then I came to the conclusion that, every formatter can add it's own custom properties
Hard to discuss without seeing the real code. If we really have to allow define dynamic property like that, maybe make the class LogEntry extends \stdClass could be an option. I will leave this to someone else to decide.
The AllowDynamicProperties attribute should also work in PHP 9. There is no indication in the RFC, that this attribute will not work.
Base on various sources I read , it will be fatal error on PHP 9:
The deprecation notice is emitted on PHP 8.2 and later. In PHP 9.0, dynamic properties will result in a fatal error.
And remember that it won't be a fatal error until PHP 9.0, so there's plenty of time to deal with it
That's reason I want to add missing properties when it's possible instead of using AllowDynamicProperties
It will be a fatal error only when the AllowDynamicProperties attribute is not there.
It will be a fatal error only when the AllowDynamicProperties attribute is not there
Ah, OK. Read the RFC again, I can confirm that. Thanks !
I like the approch of make the class LogEntry extends \stdClass. In my opinion this is a more stable solution than add the attribute
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
Labels |
Added:
?
PHP 8.x
|
RTC
Labels |
Added:
?
|
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-01-12 15:49:19 |
Closed_By | ⇒ | HLeithner |
I think that's a bad idea and we should find a better way but for now it's ok.
thanks
I have tested this item✅ successfully on 1c8a2c3
code review
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39599.