? PHP 8.x ? Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
11 Jan 2023

Summary of Changes

Allows dynamic properties on log entries so different formatters can add custom properties.

Testing Instructions

Open the article form on the PHP 8.2 on the front end with debug enabled.

Actual result BEFORE applying this Pull Request

The following warning is shown:
Creation of dynamic property Joomla\CMS\Log\LogEntry::$clientIP is deprecated

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 alikon alikon - test_item - 11 Jan 2023 - Tested successfully
avatar alikon
alikon - comment - 11 Jan 2023

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.

avatar joomdonation
joomdonation - comment - 11 Jan 2023
avatar alikon
alikon - comment - 11 Jan 2023
avatar laoneo
laoneo - comment - 11 Jan 2023

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.

avatar joomdonation
joomdonation - comment - 11 Jan 2023

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.

avatar joomdonation
joomdonation - comment - 11 Jan 2023

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.

avatar laoneo
laoneo - comment - 11 Jan 2023

The AllowDynamicProperties attribute should also work in PHP 9. There is no indication in the RFC, that this attribute will not work.

avatar joomdonation
joomdonation - comment - 11 Jan 2023

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

avatar laoneo
laoneo - comment - 11 Jan 2023

It will be a fatal error only when the AllowDynamicProperties attribute is not there.

avatar joomdonation
joomdonation - comment - 11 Jan 2023

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 !

avatar carlitorweb
carlitorweb - comment - 11 Jan 2023

I like the approch of make the class LogEntry extends \stdClass. In my opinion this is a more stable solution than add the attribute

avatar carlitorweb carlitorweb - test_item - 11 Jan 2023 - Tested successfully
avatar carlitorweb
carlitorweb - comment - 11 Jan 2023

I have tested this item successfully on 1c8a2c3


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

avatar Quy Quy - change - 12 Jan 2023
Status Pending Ready to Commit
Labels Added: ? PHP 8.x
avatar Quy
Quy - comment - 12 Jan 2023

RTC


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

avatar laoneo laoneo - change - 12 Jan 2023
Labels Added: ?
avatar HLeithner HLeithner - close - 12 Jan 2023
avatar HLeithner HLeithner - merge - 12 Jan 2023
avatar HLeithner HLeithner - change - 12 Jan 2023
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
avatar HLeithner
HLeithner - comment - 12 Jan 2023

I think that's a bad idea and we should find a better way but for now it's ok.
thanks

Add a Comment

Login with GitHub to post a comment